Skip to content
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

chore(storage): migrate top-level methods #6433

Merged
merged 8 commits into from
Jul 27, 2022

Conversation

cojenco
Copy link
Contributor

@cojenco cojenco commented Jul 26, 2022

Migrate few top-level methods to transport-agnostic interface.

  • GetServiceAccount
  • ListBuckets
  • CreateBucket*

*Interface refactoring
Shown in the current implementation, bucketName is referenced from BucketHandle, so proposing to include bucket string as an arg for method CreateBucket()

Integration tests pass locally

@product-auto-label product-auto-label bot added size: s Pull request size is small. api: storage Issues related to the Cloud Storage API. labels Jul 26, 2022
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jul 26, 2022
@cojenco cojenco marked this pull request as ready for review July 26, 2022 22:44
@cojenco cojenco requested review from a team as code owners July 26, 2022 22:44
@@ -44,7 +44,7 @@ type storageClient interface {
// Top-level methods.

GetServiceAccount(ctx context.Context, project string, opts ...storageOption) (string, error)
CreateBucket(ctx context.Context, project string, attrs *BucketAttrs, opts ...storageOption) (*BucketAttrs, error)
CreateBucket(ctx context.Context, project, bucket string, attrs *BucketAttrs, opts ...storageOption) (*BucketAttrs, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch on this

@cojenco
Copy link
Contributor Author

cojenco commented Jul 27, 2022

@tritone @noahdietz Any thoughts on adding retryConfig to BucketIterator... this is what I've done for now

Moving to the new interface detaches referencing Client (and client.retry) in BucketIterator, like so

This stems from a failed test unable to find retryConfig from Client

@noahdietz
Copy link
Contributor

noahdietz commented Jul 27, 2022

@tritone @noahdietz Any thoughts on adding retryConfig to BucketIterator... this is what I've done for now

I think that is fine. Did you have a particular concern? I did notice though that the actual API calls still reference s.retry - should that be it.retry instead? Can't comment on something not in the scope of the diff in GH Reviews I guess :)

Moving to the new interface detaches referencing Client (and client.retry) in BucketIterator

Are we able to/should we remove the BucketIterator client *Client field as a result? Haven't looked to see if it is referenced anywhere else.

@cojenco
Copy link
Contributor Author

cojenco commented Jul 27, 2022

Are we able to/should we remove the BucketIterator client *Client field as a result? Haven't looked to see if it is referenced anywhere else.

Great callout, Noah, client *Client field should be removed from BucketIterator. That helped me discover a todo to remove the no-longer used fetch method, which is the other only place referencing client field :)

Also spoke to Chris and realized my original question is more of a test issue. The call is retrieving retryConfig with no concerns, so removing the now-outdated test.

@cojenco cojenco merged commit 3a14187 into googleapis:storage-refactor Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants