Skip to content

Conversation

Suryakantdsa
Copy link

@Suryakantdsa Suryakantdsa commented Aug 2, 2025

Pull Request Template

Description:

  • Added GCS file provider in pkg/datasource/file/gcs .
  • Implemented Create , Remove , ReadDir , Open ,Stat and MakeDir using cloud.google.com/go/storage .
  • Simulates directories using object key prefixes .

Checklist:

  • I have formatted my code using goimport and golangci-lint.
  • All new code is covered by unit tests.
  • This PR does not decrease the overall code coverage.
  • I have reviewed the code comments and documentation for clarity.

Fixes #2013

@Suryakantdsa
Copy link
Author

hi @Umang01-hash , @coolwednesday ,

Please have a look at my PR , let me know if you notice anything that needs changes.

thanks !

@coolwednesday
Copy link
Member

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).

@Suryakantdsa
Copy link
Author

Suryakantdsa commented Aug 4, 2025

sure @coolwednesday , I will add the setup guide and documentation to help the user to use this feature.

thanks for the feedback👍

@Suryakantdsa
Copy link
Author

hey @coolwednesday ,

I have noticed that the setup details for file handling (S3,FTP/SFTP etc .) are already there in gofr/docs/advanced-guide/handling-file/page.md .

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..!

@Suryakantdsa
Copy link
Author

Hey @coolwednesday ,

Could you please take a look at this above doubt ?

@Suryakantdsa
Copy link
Author

hey @ccoVeille @Umang01-hash ,
Could you please take a look at this above doubt ?

Copy link
Member

@coolwednesday coolwednesday left a 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.

@Suryakantdsa
Copy link
Author

Ok, got it. I’m working on it and will let you know once it’s completed.
@coolwednesday thanks..!

@Suryakantdsa Suryakantdsa force-pushed the feat/gcs-file-provider branch from 8c79d6a to 7b1824a Compare August 16, 2025 19:06
@Suryakantdsa
Copy link
Author

hey @Umang01-hash , @coolwednesday ,

  • I have Updated the existing unified Handling File documentation (handling-file.md) to include GCS setup and usage (no new navigation entry added).
  • Included Docker container commands in contributing.md.

Here are some screenshots showing logs, traces, and metrics from the running cloud store.

Screenshots (click to expand) image
Screenshots (click to expand) image
Screenshots (click to expand) image
Screenshots (click to expand) image

Please let me know if you notice anything that needs changes.
Thank you so much..!

Copy link
Member

@coolwednesday coolwednesday left a 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.

Copy link
Contributor

@akshat-kumar-singhal akshat-kumar-singhal left a 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

@Suryakantdsa
Copy link
Author

Hi @Umang01-hash,

My branch was not up to date with gofr/development. I’ve rebased it and pushed the updates now.
please have a look.

thank you..!

Copy link
Member

@Umang01-hash Umang01-hash left a 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 inside examples/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.

@Suryakantdsa
Copy link
Author

Hi @Umang01-hash,
I’ve noted the required changes you mentioned. I’m working on them and will update you once they’re completed.

thanks for your time..

@Suryakantdsa
Copy link
Author

Hi @Umang01-hash,

I’ve made the required changes. Please have a look when you get a chance.

thank you..!

Copy link
Member

@Umang01-hash Umang01-hash left a 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:
Screenshot 2025-09-05 at 3 59 39 PM

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.

@Umang01-hash
Copy link
Member

@Suryakantdsa are you still working on this PR??

@Suryakantdsa
Copy link
Author

Hi @Umang01-hash,
I was unavailable earlier due to some medical urgency, but I’m back to working on this PR now

@Suryakantdsa
Copy link
Author

Hi @Umang01-hash ,

I've removed the if-else blocks by splitting the tests into separate, focused test functions .
Also ,TODO comment for future interface refactoring hasn't been added yet .
Please take a look when you get a chance.

Thanks!

Copy link
Member

@Umang01-hash Umang01-hash left a 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:

  1. The PR is panicing:
Image

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.

  1. 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.

}

var (
errNilGCSFileBody = errors.New("GCS file body is nil")
Copy link
Member

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.

"time"

"cloud.google.com/go/storage"
file "gofr.dev/pkg/gofr/datasource/file"
Copy link
Member

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 {
Copy link
Member

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 {
Copy link
Member

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.

@Suryakantdsa
Copy link
Author

Hi @Umang01-hash,

I have addressed the required changes.
Please take a look when you get a chance.

thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Google Cloud Storage (GCS) integration as File System

4 participants