-
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
|
||
if err != nil { | ||
f.logger.Errorf("failed to write: %v", err) | ||
msg = err.Error() |
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.
what's the purpose of this line?
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.
it captures the error message in a string , so it can be passed along for logging or response handling
|
||
var msg string | ||
|
||
st := statusErr |
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.
Is this being used anywhere?
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 variable is used only for logging, not for functional logic.
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.
Should we have a separate logger for each integration we do?
bucket *storage.BucketHandle | ||
} | ||
|
||
type gcsClient interface { |
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.
what's the advantage of creating this interface? Shouldn't GCSFile be implementing a common CloudStorage interface, if not a generic FileStorage interface? We should typically be having a single FileStorage interface which would allow developers to switch between storage options without changing anything in their code.
@coolwednesday thoughts?
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 GCS SDK (cloud.google.com/go/storage) doesn’t provide a common interface. By defining a FileSystem
abstraction with methods like Create, Remove, Open, and Rename etc
—aligned with the S3 implementation—we ensure consistency across providers. This lets developers use the same API regardless of whether they initialize S3 or GCS, e.g.:
app.AddFileStore(s3.New(&s3.Config{...}))
//or
app.AddFileStore(gcs.New(&gcs.Config{...}))
// now can use the unified FileSystem API
file, _ := ctx.File.Create("my_file.txt")
ctx.File.Rename("old_name.txt", "new_name.txt")
ctx.File.Remove("my_dir")
ctx.File.Open()
This ensures developers can use the same API across providers, making it easy to initialize either S3 or GCS and interact with files in a uniform way.
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.
We shouldn't have another interface defined for GCS, it should implement the existing "CloudStorage" interface that S3 is implementing.
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.
You’re right that ideally we should have a common CloudStorage interface which both S3 and GCS implement.
However, currently S3 itself doesn’t implement a common CloudStorage interface — it defines its own s3Client interface around the AWS SDK (github.com/aws/aws-sdk-go-v2/service/s3)
.
So for now, I referred to the existing S3 interface design and created a similar interface around the GCS SDK (cloud.google.com/go/storage)
for GCS to keep consistency.
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.
@akshat-kumar-singhal After examining the existing implementations, I agree that a common CloudStorage interface would be ideal architecturally. However, I observe that the S3 implementation currently follows the same pattern - it defines its own s3Client
interface rather than implementing a shared abstraction.
For consistency with the existing pattern and to avoid blocking this PR, I suggest we merge the current GCS implementation with a TODO comment indicating future refactoring plans:
// TODO: Future improvement - Refactor both S3 and GCS implementations to use a
// common CloudStorageClient interface for better abstraction. This implementation
// currently follows the same pattern as S3 to maintain consistency with existing code.
This allows us to ship the GCS functionality now while documenting our intention to improve the architecture in a dedicated follow-up PR.
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.
bucket *storage.BucketHandle | ||
} | ||
|
||
type gcsClient interface { |
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.
@akshat-kumar-singhal After examining the existing implementations, I agree that a common CloudStorage interface would be ideal architecturally. However, I observe that the S3 implementation currently follows the same pattern - it defines its own s3Client
interface rather than implementing a shared abstraction.
For consistency with the existing pattern and to avoid blocking this PR, I suggest we merge the current GCS implementation with a TODO comment indicating future refactoring plans:
// TODO: Future improvement - Refactor both S3 and GCS implementations to use a
// common CloudStorageClient interface for better abstraction. This implementation
// currently follows the same pattern as S3 to maintain consistency with existing code.
This allows us to ship the GCS functionality now while documenting our intention to improve the architecture in a dedicated follow-up PR.
pkg/gofr/datasource/file/gcs/go.mod
Outdated
@@ -0,0 +1,66 @@ | |||
module gofr.dev/pkg/gofr/datasource/file/gcs | |||
|
|||
go 1.24.0 |
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 use latest go version i.e. go 1.25
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.
Why are these errors exported? If not using them outside of the directory can we please unexport them?
"strings" | ||
"time" | ||
|
||
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.
Why we need an alias of same name for this import??
var ( | ||
// errNotPointer is returned when Read method is called with a non-pointer argument. | ||
errStringNotPointer = errors.New("input should be a pointer to a string") | ||
ErrOutOfRange = errors.New("out of range") |
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.
Why is this error exported?
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..! |
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