-
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
Revert google_dns_record_set
to previous implementation
#5191
Revert google_dns_record_set
to previous implementation
#5191
Conversation
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccComputeManagedSslCertificate_managedSslCertificateBasicExample|TestAccDNSRecordSet_rrdatasUpdate|TestAccDNSRecordSet_basic|TestAccDNSRecordSet_Update|TestAccDNSRecordSet_changeType|TestAccDNSRecordSet_nestedNS|TestAccDNSRecordSet_quotedTXT|TestAccDNSRecordSet_uppercaseMX You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=205564 |
Tests failed during RECORDING mode: TestAccDNSRecordSet_changeType Please fix these to complete your PR |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDNSRecordSet_changeType You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=205585 |
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.
Can we add a test that replicates hashicorp/terraform-provider-google#9257?
mmv1/third_party/terraform/resources/resource_dns_record_set.go
Outdated
Show resolved
Hide resolved
return fmt.Errorf("Error creating DNS RecordSet: %s", err) | ||
} | ||
|
||
d.SetId(fmt.Sprintf("%s/%s/%s", zone, name, rType)) |
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.
Thinking out loud: The id
change is safe because we don't use it in Read
and neither did the old implementation (so users are safely able to downgrade if needed)
mmv1/third_party/terraform/resources/resource_dns_record_set.go
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/tests/resource_dns_record_set_test.go.erb
Outdated
Show resolved
Hide resolved
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDNSRecordSet_secondaryNS You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=206126 |
Fixes hashicorp/terraform-provider-google#9257
Fixes hashicorp/terraform-provider-google#9112
Fixes hashicorp/terraform-provider-google#9416
This PR reverts the resource to the original handwritten version that uses the DNS client library from the MM-generated version that used the new API.
Deviations from the old handwritten version include:
ttl
was made optional in the previous iteration, I am keeping it optional for backwards-compatibilitytype
field can be updated which is a part of the ID. This allows one of the newer tests to import correctlyRelease Note Template for Downstream PRs (will be copied)