blob/s3: Adds NewSessionFromURLParams for AWS to set profile in the URL#3008
Merged
Merged
Conversation
It is used for s3 blob to enable setting profile from the s3 URL like: s3://bucket_name?profile=main Fixes google#2865
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Contributor
Author
|
@googlebot I signed it! |
Codecov Report
@@ Coverage Diff @@
## master #3008 +/- ##
==========================================
- Coverage 69.49% 69.42% -0.08%
==========================================
Files 111 111
Lines 11675 11684 +9
==========================================
- Hits 8114 8112 -2
- Misses 2892 2903 +11
Partials 669 669
Continue to review full report at Codecov.
|
vangent
requested changes
May 27, 2021
vangent
left a comment
Contributor
There was a problem hiding this comment.
Can you add a test that uses "profile"?
Should be near here:
https://github.com/google/go-cloud/blob/master/blob/s3blob/s3blob_test.go#L293
It won't really verify the semantics, but at least verify that
- using a "profile" query parameter works.
- using a "profile" query parameter along with another query parameter works.
- using a "profile" query parameter along with an invalid query parameter fails (this will at least partly test the "rest" code path).
Contributor
Author
|
Thanks for the feedback! The tests added. |
vangent
approved these changes
May 28, 2021
|
I believe this change introduced a race This is the (partial) error message: |
Contributor
|
Thanks for reporting, sent #3101 to
fix.
…On Wed, Feb 23, 2022 at 6:19 PM ianhmarkowitz ***@***.***> wrote:
I believe this change introduced a race
This is the (partial) error message:
`WARNING: DATA RACE
Write at 0x00c0001a4d00 by goroutine 61:
gocloud.dev/blob/s3blob.(*urlSessionOpener).OpenBucketURL()
[Removed]/vendor/gocloud.dev/blob/s3blob/s3blob.go:108 +0x1e4
gocloud.dev/blob.applyPrefixParam()
[Removed]/vendor/gocloud.dev/blob/blob.go:1270 +0x210
gocloud.dev/blob.(*URLMux).OpenBucket()
[Removed]/vendor/gocloud.dev/blob/blob.go:1247 +0xd2
gocloud.dev/blob.OpenBucket()
[Removed]/vendor/gocloud.dev/blob/blob.go:1301 +0x32d
...
Previous write at 0x00c0001a4d00 by goroutine 60:
gocloud.dev/blob/s3blob.(*urlSessionOpener).OpenBucketURL()
[Removed]/vendor/gocloud.dev/blob/s3blob/s3blob.go:108 +0x1e4
gocloud.dev/blob.applyPrefixParam()
[Removed]/vendor/gocloud.dev/blob/blob.go:1270 +0x210
gocloud.dev/blob.(*URLMux).OpenBucket()
[Removed]/vendor/gocloud.dev/blob/blob.go:1247 +0xd2
gocloud.dev/blob.OpenBucket()
[Removed]/vendor/gocloud.dev/blob/blob.go:1301 +0x32d
...`
—
Reply to this email directly, view it on GitHub
<#3008 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFMCKTQNQZPLQ54O4DIRACLU4WIS7ANCNFSM45TXHWUQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It is used for s3 blob to enable setting profile from S3 URLs like: s3://bucket_name?profile=main
Fixes #2865
The first cut. I had to replace
lazySessionOpenerprivate class because after this change it is possible to open several s3 buckets using different AWS profiles, so creating a session withinit.Dodoesn't make much sense anymore.Basically, the idea is to get profile from the url and pass the rest of the parameters to
ConfigFromURLParamsafterwards.Maybe it would be a good idea to refactor those functions that parse the URL somehow, so they are not repeated in several places in this repository (i.e. https://github.com/google/go-cloud/blob/master/blob/azureblob/azureblob.go#L318 does pretty much the same thing, but it's private, and also doesn't allow multiple values for the same parameter).