-
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
Add data source for retrieving multiple GCS buckets from a project #10444
Add data source for retrieving multiple GCS buckets from a project #10444
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @BBBmau, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
7be1c28
to
cd71348
Compare
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataSourceGoogleStorageBuckets_basic |
|
cd71348
to
f4e5a0e
Compare
mmv1/third_party/terraform/services/storage/data_source_google_storage_buckets.go
Outdated
Show resolved
Hide resolved
f4e5a0e
to
aec48ca
Compare
aec48ca
to
2c5dfef
Compare
@BBBmau |
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataSourceGoogleStorageBuckets_basic |
|
@BBBmau
|
Took a look and it could be something to do with the ordering of buckets, this is the output from the error message:
looking at the API responses both buckets are using the same name:
|
Thank you for taking a look and sharing the information. I got a similar error when I was testing in my own project. It originated in that the test project wasn't clean and contained already buckets, which was why the response contained unexpected results. Cleaning up the test project from all buckets resolved the issue in my case. Maybe, the test project from VCR-Test contains buckets that compromise the results. Would you mind to clean up the test project and re-run the pipeline? |
func TestAccDataSourceGoogleStorageBuckets_basic(t *testing.T) { | ||
t.Parallel() | ||
|
||
project := envvar.GetTestProjectFromEnv() |
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.
looking more into this, this is one of those cases where we'd want to also create a project as part of the test so that the environment stays consistent. This makes it so that only the two buckets that are created here will only exist for this project that's also created as part of running the test.
This is done in other resources, an example of this is found here:
magic-modules/mmv1/third_party/terraform/services/storage/resource_storage_bucket_test.go.erb
Lines 2226 to 2231 in 5c9241e
resource "google_project" "acceptance" { | |
name = "tf-test-%{random_suffix}" | |
project_id = "tf-test-%{random_suffix}" | |
org_id = "%{organization}" | |
billing_account = "%{billing_account}" | |
} |
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.
This is a really good approach.
Thank you very much!
I'd like to implement it, but I'm restricted to a single project and cannot run the tests locally 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.
I followed your suggestion and I've extended the test case with the deployment of it's own project to have a clean test environment.
Please, have a look on it.
2c5dfef
to
ca86d41
Compare
ca86d41
to
65b5483
Compare
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataSourceGoogleStorageBuckets_basic |
|
65b5483
to
a169166
Compare
a169166
to
f2044e1
Compare
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataSourceGoogleStorageBuckets_basic |
|
f2044e1
to
dd32a16
Compare
@BBBmau I've fixed a typo and the acceptance test with the included project succeeded with my development environment. |
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataSourceGoogleStorageBuckets_basic |
|
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.
All looks great, Thanks for the contribution!
Thanks for your patience and the review! |
Adds a new data source "data_google_storage_buckets", allowing to retrieve multiple GCS buckets from a project.
Fixes: hashicorp/terraform-provider-google#17845
Release Note Template for Downstream PRs (will be copied)