-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support gcs #278
feat: support gcs #278
Conversation
Wow, thanks @OldPanda for the great contribution :) Long time no see~~ |
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #278 +/- ##
==========================================
- Coverage 90.89% 90.76% -0.14%
==========================================
Files 77 78 +1
Lines 3648 3693 +45
==========================================
+ Hits 3316 3352 +36
- Misses 326 332 +6
- Partials 6 9 +3
☔ View full report in Codecov by Sentry. |
Long time no see @wey-gu ! I'm glad to have a chance to contribute to NebulaGraph again! |
pkg/source/gcs.go
Outdated
|
||
func (s *gcsSource) Open() error { | ||
if s.c.GCS.Endpoint != "" { | ||
err := os.Setenv("STORAGE_EMULATOR_HOST", s.c.GCS.Endpoint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about use WithEndpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WithEndpoint
looks like a cleaner way to specify an endpoint for a GCS connection. However, their official docs introduces the env var only. Let me see if it's feasible to use WithEndpoint
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OldPanda Hi, it's WithEndpoint
work fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't bootstrap a separate emulator to test the change, but make test
works well. Is it good enough as a http server was ran by the testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, our team has tested it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for help!
pkg/source/gcs.go
Outdated
} | ||
defer client.Close() | ||
|
||
obj := client.Bucket(s.c.GCS.Bucket).Object(s.c.GCS.Key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about .Object(strings.TrimLeft(s.c.GCS.Key, "/"))
?
Maybe someone add /
in front?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I'll update accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What type of PR is this?
What problem(s) does this PR solve?
Issue(s) number:
#279
Description:
To allow users to import csv files from GCS.
How do you solve it?
Special notes for your reviewer, ex. impact of this fix, design document, etc: