-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 an optional provider level label "goog-terraform-provisioned" #9808
Add an optional provider level label "goog-terraform-provisioned" #9808
Conversation
Hello! I am a robot. It looks like you are a: @trodge, 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. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 12 files changed, 1384 insertions(+), 30 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packagesall service packages are affected Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccCloudRunV2Service_cloudrunv2ServiceAttributionLabel|TestAccComputeAddress_withAttributionRemoved|TestAccComputeAddress_withProactiveAttributionSetOnUpdate|TestAccComputeAddress_withProactiveAttribution|TestAccComputeAddress_withCreationOnlyAttribution|TestAccComputeAddress_withCreationOnlyAttributionSetOnUpdate|TestAccDataSourceGoogleServiceAccountAccessToken_basic |
Rerun these tests in REPLAYING mode to catch issues
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 12 files changed, 1411 insertions(+), 30 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packagesall service packages are affected Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccClouddeployTarget_withCreationOnlyAttribution|TestAccCloudRunService_withCreationOnlyAttribution|TestAccComputeInstance_creationOnlyAttributionLabel|TestAccComputeDisk_attributionLabelOnCreation |
Rerun these tests in REPLAYING mode to catch issues
|
LGTM. |
mmv1/third_party/terraform/fwprovider/framework_provider.go.erb
Outdated
Show resolved
Hide resolved
Do we recommend the users to use |
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
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 12 files changed, 1411 insertions(+), 30 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packagesall service packages are affected
|
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
Thanks so much for the review!
Tagging @rileykarson for an answer... |
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 from a usability perspective, comments/suggestions inline
re: the comment above, We could add a warning to the documentation that this will include immutable resources where the change will cause a diff for PROACTIVE.
mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown
Outdated
Show resolved
Hide resolved
…nce.html.markdown Co-authored-by: Riley Karson <rileykarson@google.com>
…nce.html.markdown Co-authored-by: Riley Karson <rileykarson@google.com>
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 12 files changed, 1411 insertions(+), 30 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packagesall service packages are affected
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 12 files changed, 1408 insertions(+), 30 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packagesall service packages are affected 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 testsTestAccComputeDisk_attributionLabelOnCreation |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 12 files changed, 1408 insertions(+), 30 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packagesall service packages are affected 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 testsTestAccDataprocClusterIamPolicy |
Rerun these tests in REPLAYING mode to catch issues
|
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
…ogleCloudPlatform#9808) * Add an optional provider level label indicating a resource was provisioned by Terraform * Fix TestAccComputeAddress_withAttributionRemoved to reflect actual behavior * Fix a bad merge * Skip tests that change provider config when running under VCR * Fix some tabs that should have been spaces * Update mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown Co-authored-by: Riley Karson <rileykarson@google.com> * Update mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown Co-authored-by: Riley Karson <rileykarson@google.com> * Change skip_terraform_attribution_label to add_terraform_attribution_label * Fix a test that was missed in the change to add_terraform_attribution_label --------- Co-authored-by: Riley Karson <rileykarson@google.com>
…ogleCloudPlatform#9808) * Add an optional provider level label indicating a resource was provisioned by Terraform * Fix TestAccComputeAddress_withAttributionRemoved to reflect actual behavior * Fix a bad merge * Skip tests that change provider config when running under VCR * Fix some tabs that should have been spaces * Update mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown Co-authored-by: Riley Karson <rileykarson@google.com> * Update mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown Co-authored-by: Riley Karson <rileykarson@google.com> * Change skip_terraform_attribution_label to add_terraform_attribution_label * Fix a test that was missed in the change to add_terraform_attribution_label --------- Co-authored-by: Riley Karson <rileykarson@google.com>
…ogleCloudPlatform#9808) * Add an optional provider level label indicating a resource was provisioned by Terraform * Fix TestAccComputeAddress_withAttributionRemoved to reflect actual behavior * Fix a bad merge * Skip tests that change provider config when running under VCR * Fix some tabs that should have been spaces * Update mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown Co-authored-by: Riley Karson <rileykarson@google.com> * Update mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown Co-authored-by: Riley Karson <rileykarson@google.com> * Change skip_terraform_attribution_label to add_terraform_attribution_label * Fix a test that was missed in the change to add_terraform_attribution_label --------- Co-authored-by: Riley Karson <rileykarson@google.com>
…ogleCloudPlatform#9808) * Add an optional provider level label indicating a resource was provisioned by Terraform * Fix TestAccComputeAddress_withAttributionRemoved to reflect actual behavior * Fix a bad merge * Skip tests that change provider config when running under VCR * Fix some tabs that should have been spaces * Update mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown Co-authored-by: Riley Karson <rileykarson@google.com> * Update mmv1/third_party/terraform/website/docs/guides/provider_reference.html.markdown Co-authored-by: Riley Karson <rileykarson@google.com> * Change skip_terraform_attribution_label to add_terraform_attribution_label * Fix a test that was missed in the change to add_terraform_attribution_label --------- Co-authored-by: Riley Karson <rileykarson@google.com>
The intention of this change is to make it easy to identify GCP resources that are managed by Terraform when using other clients like Cloud Console. Overall, this builds on and behaves much the same as provider default labels from #8670, but instead of free form labels it adds a single specific structured label,
goog-terraform-provisioned = "true"
when enabled. That is:Is roughly equivalent to:
The change is documented in
provider_reference.html.markdown
. In short, this is a no-op change for users that do not opt in to the feature. For users that do opt in (by settingskip_terraform_attribution_label = false
in the provider configuration), there are two modes of operation configured with the optionalterraform_attribution_label_addition_strategy
field:goog-terraform-provisioned
label will only be added to newly created resources. This is the default value.terraform apply
runs.Tests are included for a handful of resources, including MMv1, DCL, and handwritten resources.
I'm creating this as a draft PR to gather early feedback (and because as I'm writing this description I thought of some tests changes to make 🤔)
Release Note Template for Downstream PRs (will be copied)