-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
test(storage): add conformance tests for endpoints on SignURLs #9346
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, a few small questions. Wait to merge your fix before merging this one obviously.
storage/conformance_test.go
Outdated
@@ -172,7 +174,24 @@ func TestSigningV4Conformance(t *testing.T) { | |||
style = BucketBoundHostname(tc.BucketBoundHostname) | |||
} | |||
|
|||
gotURL, err := SignedURL(tc.Bucket, tc.Object, &SignedURLOptions{ | |||
if tc.EmulatorHostname != "" { | |||
t.Setenv("STORAGE_EMULATOR_HOST", tc.EmulatorHostname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of t.Setenv; wondering if we should be using it elsewhere since it's a relatively recent addition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there are likely various areas it could be used. I remember trying to use it a while back but we were still supporting previous versions.
@@ -172,7 +174,24 @@ func TestSigningV4Conformance(t *testing.T) { | |||
style = BucketBoundHostname(tc.BucketBoundHostname) | |||
} | |||
|
|||
gotURL, err := SignedURL(tc.Bucket, tc.Object, &SignedURLOptions{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity: are other languages which don't have an equivalent of our BucketHandle.SignedURL
able to handle this correctly? Do they need to add more signed URL options so the user can update the configuration manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clients that don't already support it, the guidance is for languages to add a method somewhere below the client level to handle this (and eventually provide users with client creds re-use).
opts = append(opts, option.WithUniverseDomain(tc.UniverseDomain)) | ||
} | ||
|
||
c, err := NewClient(context.Background(), opts...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this prone to be impacted by other env options that the user may have set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... that's a good point. The reason I'm using it here is to mimic a flow with the endpoint, universe domain and emulator env var setting; to make sure that the way it's handled during client creation produces correct hostnames passed through to SignURLs.
I removed the conditional around STORAGE_EMULATOR_HOST which for sure would impact the test - now we are always explicitly setting to the empty string if the test doesn't set an emulator host. There may be others, but none come to mind, and I'm not sure how to test this adequately without creating the client this way.
Pulls in googleapis/conformance-tests#89 and changes tests to set options accordingly.
Note: requires #9347 (tests will fail in the interim)