Skip to content

Conversation

@quartzmo
Copy link
Member

@quartzmo quartzmo commented Nov 25, 2024

This PR is intended as a replacement for #11121. It uses a new internaloption.AuthCreds helper func added to the google.golang.org/api/option package to support sourcing credentials from both old and new auth options. You can see that helper here: googleapis/google-api-go-client#2891.

@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Nov 25, 2024
quartzmo added a commit to quartzmo/google-api-go-client that referenced this pull request Nov 26, 2024
quartzmo added a commit to quartzmo/google-api-go-client that referenced this pull request Nov 26, 2024
quartzmo added a commit to googleapis/google-api-go-client that referenced this pull request Nov 26, 2024
Copy link
Contributor

@BrennaEpp BrennaEpp left a comment

Choose a reason for hiding this comment

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

In general looks good, thank you for the changes. Just a note on making sure tests pass with old auth creds sent in by users.

// Don't error out here. The user may have passed in their own HTTP
// client which does not auth with ADC or other common conventions.
c, err := transport.Creds(ctx, opts...)
c, err := internaloption.AuthCreds(ctx, opts)
Copy link
Member Author

Choose a reason for hiding this comment

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

Does internaloption.EnableNewAuthLibrary() need to be added here (or elsewhere)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so, internaloption.AuthCreds(ctx, opts) will call into the new auth library no matter what it seems. What maybe should be added is a check for the env var to disable new auth if we still need that in case we need to debug?

@quartzmo quartzmo marked this pull request as ready for review April 11, 2025 22:08
@quartzmo quartzmo requested review from a team as code owners April 11, 2025 22:08
@BrennaEpp
Copy link
Contributor

Just adding a note here, integration tests pass on the latest commit to this PR.

Copy link
Contributor

@BrennaEpp BrennaEpp left a comment

Choose a reason for hiding this comment

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

Thank you!! Sorry for the delay. Storage is frozen for the next release, so I added the do not merge label. This can be merged once v1.53.0 is released.

@BrennaEpp BrennaEpp added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels May 2, 2025
@BrennaEpp BrennaEpp enabled auto-merge (squash) May 14, 2025 08:55
@BrennaEpp BrennaEpp merged commit 3a22349 into googleapis:main May 14, 2025
10 checks passed
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants