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

fix(storage): respect STORAGE_EMULATOR_HOST in signedURL #5411

Closed
wants to merge 2 commits into from

Conversation

ChrisBr
Copy link
Contributor

@ChrisBr ChrisBr commented Jan 29, 2022

Respect STORAGE_EMULATOR_HOST in signedURL.

@ChrisBr ChrisBr requested review from a team as code owners January 29, 2022 18:32
@product-auto-label product-auto-label bot added the size: xs Pull request size is extra small. label Jan 29, 2022
@ChrisBr ChrisBr force-pushed the cbruckmayer/storage-emulator branch from e9ba020 to 0bec397 Compare January 29, 2022 18:41
@codyoss codyoss changed the title Respect STORAGE_EMULATOR_HOST in signedURL fix(storage): respect STORAGE_EMULATOR_HOST in signedURL Jan 31, 2022
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Feb 1, 2022
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. A couple thoughts; let me know if you are able to fix or if you'd like some help. It would also be good to add a test case for this to TestSignedURLV4.

return os.Getenv("STORAGE_EMULATOR_HOST")
} else {
return "storage.googleapis.com"
}
}

func (s virtualHostedStyle) host(bucket string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

The path for virtual hosted should be updated as well probably?

@@ -308,7 +308,11 @@ type bucketBoundHostname struct {
}

func (s pathStyle) host(bucket string) string {
return "storage.googleapis.com"
if os.Getenv("STORAGE_EMULATOR_HOST") {
return os.Getenv("STORAGE_EMULATOR_HOST")
Copy link
Contributor

Choose a reason for hiding this comment

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

We accept STORAGE_EMULATOR_HOST both with and without a scheme, so we should strip the scheme if it is present. See https://github.com/googleapis/google-cloud-go/blob/main/storage/storage.go#L154

@BrennaEpp
Copy link
Contributor

Hi @ChrisBr, thanks for your contribution! I made a PR to your branch to incorporate the changes suggested by @tritone. Can you look that over and sign the CLA as well?

To sign the CLA, please visit https://cla.developers.google.com/. Once you've signed, click here https://cla.developers.google.com/github/rescan?pr=googleapis%2Fgoogle-cloud-go%2Fpull%2F5411&key=%01~%D4%13%80JC%15G%B8Y%92%FC%87%D9%82%D4%CE%97%DC%40&expiry=2022-03-15 to update the check

@ChrisBr
Copy link
Contributor Author

ChrisBr commented Feb 15, 2022

Sorry for the delay, will check tomorrow with the CLA 👍

@ChrisBr
Copy link
Contributor Author

ChrisBr commented Feb 22, 2022

The CLA is still under review with my employer Shopify but I will hopefully have an answer in the next couple of days. Sorry for the delay.

@ChrisBr
Copy link
Contributor Author

ChrisBr commented Feb 24, 2022

@BrennaEpp signed the CLA 👍 Thanks for your patience.

@product-auto-label product-auto-label bot added the stale: old Pull request is old and needs attention. label Mar 1, 2022
@BrennaEpp
Copy link
Contributor

These changes were merged in #5673. Thank you for your contribution!

@BrennaEpp BrennaEpp closed this Mar 14, 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: xs Pull request size is extra small. stale: old Pull request is old and needs attention.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants