-
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 a new field desired_state
to manage CertificateAuthority state
#5934
Conversation
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. @c2thorn, please review this PR or find an appropriate assignee. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 135 insertions(+), 4 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccDataprocCluster_withImageVersion|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthorityIsStaged |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
mmv1/third_party/terraform/tests/resource_privateca_certificate_authority_test.go
Show resolved
Hide resolved
@gfxcc asking for a second opinion from the team as this is fairly unusual behavior. May not be worth addressing my previous comment until I hear back |
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.
After discussion, we think that state-controlling behavior is best represented as a desired_state
field.
is_staged
would not be able to accommodate new states in the future, and there is precedence for this type of field in compute instance: https://github.com/hashicorp/terraform-provider-google/blob/main/google/resource_compute_instance.go#L740
The field would be an optional field that enables the CA if unset, and controls the state when explicitly set.
It would need a lot of custom code to get working:
- Customizediff - would error on invalid state transitions
- customflatten - If
desired_state
is set, the current state would be read. (see https://github.com/hashicorp/terraform-provider-google/blob/main/google/resource_compute_instance.go#L1361). If unset, leave unset. - customexpand - should always return empty as we do not want to send the field to the API.
This field would provide a better representation of the resource, wouldn't be a breaking change (CA is still enabled if unset), but is fairly complex. @gfxcc let me know what you think.
@c2thorn That sounds a great idea. Will push a new revision later. |
6361ecc
to
97ea5d9
Compare
is_staged
to create a CertificateAuthority in STAGED
statedesired_state
to manage CertificateAuthority state
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 13 files changed, 270 insertions(+), 4 deletions(-)) |
97ea5d9
to
9aa697a
Compare
9aa697a
to
6f2eb18
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 347 insertions(+), 21 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccFirebaserulesRelease_BasicRelease|TestAccPrivatecaCertificateAuthority_rootCaManageDesiredState|TestAccPrivatecaCertificateAuthority_rootCaIsEnabledByDefault|TestAccPrivatecaCertificateAuthority_rootCaCreatedInStaged|TestAccPrivatecaCertificateAuthority_subordinateCaCreatedInAwaitingUserActivation |
Tests passed during RECORDING mode: All tests passed |
mmv1/templates/terraform/post_create/privateca_certificate_authority.go.erb
Outdated
Show resolved
Hide resolved
mmv1/templates/terraform/pre_create/privateca_certificate_authority.go.erb
Outdated
Show resolved
Hide resolved
…hority.go.erb Co-authored-by: Cameron Thornton <camthornton@google.com>
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 342 insertions(+), 21 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccFirebaserulesRelease_BasicRelease|TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic |
eaa948e
to
5c1c218
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 360 insertions(+), 21 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccFirebaserulesRelease_BasicRelease|TestAccBigQueryDataTable_bigtable |
Apologies for the delay @gfxcc, LGTM |
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)