-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: add GCS support as file provider (#2013) #2117
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
base: development
Are you sure you want to change the base?
feat: add GCS support as file provider (#2013) #2117
Conversation
hi @Umang01-hash , @coolwednesday , Please have a look at my PR , let me know if you notice anything that needs changes. thanks ! |
Hey! Can you add Setup details for the ease of testing and documentation for users who would want to setup the same by referring documentation (when this feature would be out). |
sure @coolwednesday , I will add the setup guide and documentation to help the user to use this feature. thanks for the feedback👍 |
hey @coolwednesday , I have noticed that the setup details for file handling (S3,FTP/SFTP etc .) are already there in Just wanted to confirm , should I add a setup instruction for GCS in the same file or would you prefer this to be in a separate file ? thanks..! |
Hey @coolwednesday , Could you please take a look at this above doubt ? |
hey @ccoVeille @Umang01-hash , |
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.
Please update the unified handling-files documentation to include a code snippet demonstrating how to add GCS support.
Additionally, make the necessary changes in the Container package so that it works like any other datasource. For reference, review PR #2076 and replicate the relevant changes.
After that, ensure traces, logs, and metrics are integrated (also refer to #2076 for implementation details).
Once implemented:
- Add screenshots showing logs, traces, and metrics from the running cloud store.
- Add setup documentation in the docs folder.
- Update the navigation.js.
- Include any Docker container commands used in contributing.md.
Ok, got it. I’m working on it and will let you know once it’s completed. |
8c79d6a
to
7b1824a
Compare
hey @Umang01-hash , @coolwednesday ,
Here are some screenshots showing logs, traces, and metrics from the running cloud store. Please let me know if you notice anything that needs changes. |
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.
The whole implementation other than this looks good to me.
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.
Overall rewrite of file package should be considered before adding more implementations like GCS etc
e6dcea0
to
22e4f09
Compare
Hi @Umang01-hash, My branch was not up to date with gofr/development. I’ve rebased it and pushed the updates now. thank you..! |
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.
- Please remove all the
go.mod
changes from all the other datasources as they are not related to scope of your PR. Also the changes insideexamples/using-add-filestore
is also not required. Please revert all of these changes. - There are no tests written for entire newly added code but only some of it. Please make sure to write tests using mocks for the newly added code (feel free to refer other datasources to see how they use mocks for testing).
- Only export errors, constants etc when needed, unexport all the ones not used outside the directory.
Hi @Umang01-hash, thanks for your time.. |
22e4f09
to
690b993
Compare
Hi @Umang01-hash, I’ve made the required changes. Please have a look when you get a chance. thank you..! |
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.
Hey @Suryakantdsa, Thankyou for making the requested changes.
After running the tests here is your current code coverage:
Please make sure that each file (except from mocks, interfaces) has atleast 75-80% code coverage.
- Also in the example you provided in the documentation, the gcs Config:
&gcs.Config{
BucketName: "my-bucket",
CredentialsJSON: readFile("gcs-credentials.json"),
ProjectID: "my-project-id",
}
Endpoint field is missing from here.
@Suryakantdsa are you still working on this PR?? |
Hi @Umang01-hash, |
…ions and fix linting issue
Hi @Umang01-hash , I've removed the if-else blocks by splitting the tests into separate, focused test functions . Thanks! |
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.
@Suryakantdsa thankyou for making the changes but still we have some flaws in the PR:
- The PR is panicing:
And the mostprobable reason is metrics is nil, and you are trying to call metrics.NewHistogram resulting in panic. You should place this NewHistorgam in Connect method instead of New.
- Also it will be better if we can add automatic retries in case for conenction to GCS failed. It has to be a background and non blocking task. We can log the user in case of connection error and retry in background and once it gets connected again we will log again and now we are good to go.
pkg/gofr/datasource/file/gcs/file.go
Outdated
} | ||
|
||
var ( | ||
errNilGCSFileBody = errors.New("GCS file body is nil") |
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.
@Suryakantdsa this comment has not been resolved yet.
pkg/gofr/datasource/file/gcs/fs.go
Outdated
"time" | ||
|
||
"cloud.google.com/go/storage" | ||
file "gofr.dev/pkg/gofr/datasource/file" |
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.
The alias file
here is not useful, please remove it
func defaultBuckets() []float64 { | ||
return []float64{0.1, 1, 10, 100, 1000} | ||
} | ||
func New(config *Config) file.FileSystemProvider { |
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.
This New
method is uncovered in the tests, can we also please add a Test method for New
f.metrics = m | ||
} | ||
} | ||
func generateCopyName(original string, count int) string { |
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.
This function is also uncovered and can be covered by adding a test easily.
…ions and fix linting issue
Hi @Umang01-hash, I have addressed the required changes. thanks ! |
Pull Request Template
Description:
pkg/datasource/file/gcs
.Create
,Remove
,ReadDir
,Open
,Stat
andMakeDir
usingcloud.google.com/go/storage
.Checklist:
goimport
andgolangci-lint
.Fixes #2013