-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 TestAccStorageSignedUrl_accTest #5200
Fix TestAccStorageSignedUrl_accTest #5200
Conversation
Co-authored-by: upodroid <[email protected]>
Co-authored-by: upodroid <[email protected]>
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. Thanks for your contribution! A human will be with you soon. @melinath, please review this PR or find an appropriate assignee. |
I removed the header because:
https://cloud.google.com/storage/docs/json_api/v1/objects#resource TestAccStorageSignedUrl_accTest Test Run
|
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.
I agree that this looks like it probably never should have been passing, since the generation is server-generated. Maybe the API finally started validating this? Or maybe x-goog-test used to cause the generation to be 1 and no longer does?
I think there is value to making sure that the datasource is correctly passing headers to the API... but I don't think this test ever did that. Even if we were able to get the correct generation, that wouldn't prove that we were passing headers in, since we would get a successful result regardless of whether the header was getting passed.
Really, we should probably be testing that if we pass a generation match header that is incorrect (like, say, 1) we get a precondition failed error. That would let us verify that we're actually passing the headers correctly. Would you be down to switch the test around to check that instead?
If the idea is to test headers being set, then |
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.
Okay, that's fine too. You'll need to update the if-generation-match
on line 128 to if-metageneration-match as well. (Currently if-generation-match)
my bad, fixed |
/gcbrun |
It looks like this test is skipped in VCR - running a special build to check it: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaMmUpstream/205953 |
* allow disabling service accounts Co-authored-by: upodroid <[email protected]> * fix typos * revert warning change * remove dynamically generated header from test Co-authored-by: upodroid <[email protected]> * reinstance the test with a diff header * fix typo * fix fmt
Fix: hashicorp/terraform-provider-google#9990
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)