-
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
Infer bigtable zone from provider #4392
Infer bigtable zone from provider #4392
Conversation
Fork Update
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. @ScottSuarez, please review this PR or find an appropriate assignee. |
/gcbrun |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=166692" |
/gcbrun failed; trying again. |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=166911" |
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 have a couple of questions about the implementation here. I was also wondering if you'd be open to adding a test to make sure that this works as expected. (current tests all seem to explicitly set a zone.)
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccInstanceTemplateDatasource_filter|TestAccInstanceTemplateDatasource_filter_mostRecent|TestAccDataSourceStorageBucketObjectContent_Basic|TestAccCloudRunServiceIamBindingGenerated|TestAccCloudRunServiceIamMemberGenerated|TestAccCloudRunServiceIamPolicyGenerated|TestAccActiveDirectoryDomainTrust_activeDirectoryDomainTrustBasicExample|TestAccApigeeOrganization_apigeeOrganizationCloudFullTestExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccCloudRunService_cloudRunServiceBasicExample|TestAccCloudRunDomainMapping_cloudRunDomainMappingBasicExample|TestAccCloudRunService_cloudRunServiceSqlExample|TestAccCloudRunService_cloudRunServiceNoauthExample|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupCloudrunExample|TestAccContainerCluster_withPrivateClusterConfigMissingCidrBlock You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=166925" |
So instead of writing another test, I removed the zone from |
/gcbrun |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=166934" |
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.
It looks like in CI there are two failures related to BigTable:
TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample
TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample
They're both failing with Error: Computed attribute cannot be set
with regard to the zone attribute, which seems a bit odd since there are a number of other tests that hardcode a zone for a google_bigtable_instance.cluster
without issue.
Hmm, I'll take a closer look. Can you look in to why the bigtable client doesn't log the requests when TF_LOG is set to debug? |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccInstanceTemplateDatasource_filter|TestAccInstanceTemplateDatasource_filter_mostRecent|TestAccActiveDirectoryDomainTrust_activeDirectoryDomainTrustBasicExample|TestAccApigeeOrganization_apigeeOrganizationCloudFullTestExample|TestAccApigeeOrganization_apigeeOrganizationCloudBasicTestExample|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccCloudRunService_cloudRunServiceMultipleEnvironmentVariablesExample You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=167025" |
It looks like the other tests (like I'll check on the TF_LOG question :-) |
It is fixed now, both of the tests are passing. |
/gcbrun |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=167120" |
@@ -190,7 +190,6 @@ resource "google_bigtable_instance" "instance" { | |||
name = "%s" | |||
cluster { | |||
cluster_id = "%s" | |||
zone = "us-central1-b" |
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 posterity: this was removed to make sure the case of defaulting to the provider value works. There are other tests that create explicitly in us-central1-b.
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.
LGTM pending passing tests. Re: TF_LOG issues, could they be related to hashicorp/terraform-plugin-sdk#88 (comment)?
I'm definitely seeing output from API requests on other tests locally, so it's not the issue I linked to above. That's odd. Thanks for catching this - I'll make a ticket. |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccActiveDirectoryDomainTrust_activeDirectoryDomainTrustBasicExample|TestAccApigeeOrganization_apigeeOrganizationCloudBasicTestExample|TestAccApigeeOrganization_apigeeOrganizationCloudFullTestExample|TestAccCloudRunService_cloudRunServiceMultipleEnvironmentVariablesExample You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=167124" |
@melinath
Part of #4377
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)