Skip to content
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

Don't send update request if updatemask is empty for resources with terraform_labels field #9154

Merged
merged 4 commits into from
Oct 4, 2023

Conversation

zli82016
Copy link
Member

@zli82016 zli82016 commented Oct 3, 2023

Fixes hashicorp/terraform-provider-google#16091

If the updateMask is empty, it means that no update is needed. The update request is not needed to be sent.

Release Note Template for Downstream PRs (will be copied)

provider: fixed the bug that update request is sent to services when updateMask is empty

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Breaking Change Detection Failed

The breaking change detector crashed during execution. This is usually due to the downstream provider(s) failing to compile. Please investigate or follow up with your reviewer.

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 59 files changed, 1261 insertions(+), 1031 deletions(-))
Terraform Beta: Diff ( 76 files changed, 1629 insertions(+), 1346 deletions(-))

@zli82016 zli82016 force-pushed the labels-fix-terraform-labels branch from ea7e7b5 to e1da22c Compare October 3, 2023 18:16
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 60 files changed, 1277 insertions(+), 1043 deletions(-))
Terraform Beta: Diff ( 77 files changed, 1645 insertions(+), 1358 deletions(-))

@zli82016 zli82016 closed this Oct 3, 2023
@zli82016 zli82016 reopened this Oct 3, 2023
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 60 files changed, 1277 insertions(+), 1043 deletions(-))
Terraform Beta: Diff ( 77 files changed, 1645 insertions(+), 1358 deletions(-))

@zli82016 zli82016 force-pushed the labels-fix-terraform-labels branch from e1da22c to 39220d2 Compare October 3, 2023 18:42
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 59 files changed, 1271 insertions(+), 1037 deletions(-))
Terraform Beta: Diff ( 76 files changed, 1639 insertions(+), 1352 deletions(-))

@zli82016 zli82016 changed the title Don't send update request if updatmask is empty when resource has ter… Don't send update request if updatemask is empty for resources having terraform_labels field Oct 3, 2023
@zli82016 zli82016 changed the title Don't send update request if updatemask is empty for resources having terraform_labels field Don't send update request if updatemask is empty for resources with terraform_labels field Oct 3, 2023
@zli82016
Copy link
Member Author

zli82016 commented Oct 3, 2023

It looks like there is a bug in terraform-google-conversion-build-and-unit-tests. It is still checking the previous revision of this PR.

mmv1/templates/terraform/resource.erb Outdated Show resolved Hide resolved
mmv1/templates/terraform/resource.erb Outdated Show resolved Hide resolved
@c2thorn
Copy link
Member

c2thorn commented Oct 3, 2023

@rileykarson
Copy link
Member

It's still accurate. The problem is that we're calling Terraform's Update method which eventually sends a spurious Patch/Post to the API. This change will guard against sending that Patch/Post.

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3113
Passed tests 2793
Skipped tests: 315
Affected tests: 5

Action taken

Found 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccContainerNodePool_withKubeletConfig|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerCluster_withAddons|TestAccDataSourceGoogleServiceAccountAccessToken_basic|TestAccDataSourceGoogleServiceAccountJwt

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataSourceGoogleServiceAccountAccessToken_basic[Debug log]
TestAccDataSourceGoogleServiceAccountJwt[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerNodePool_withKubeletConfig[Error message] [Debug log]
TestAccContainerNodePool_withUpgradeSettings[Error message] [Debug log]
TestAccContainerCluster_withAddons[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Breaking Change Detection Failed

The breaking change detector crashed during execution. This is usually due to the downstream provider(s) failing to compile. Please investigate or follow up with your reviewer.

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 158 files changed, 3151 insertions(+), 2550 deletions(-))
Terraform Beta: Diff ( 198 files changed, 4001 insertions(+), 3271 deletions(-))

@rileykarson
Copy link
Member

Looks like the build failing is just a case of

res, err = transport_tpg.SendRequest(transport_tpg.SendRequestOptions{
assuming res would already exist. You can declare res or change it to instantiate in that line by making it :=

Looking into addressgroups to see if that transformation is safe

@zli82016 zli82016 force-pushed the labels-fix-terraform-labels branch from 48937a7 to d141415 Compare October 3, 2023 21:34
@zli82016
Copy link
Member Author

zli82016 commented Oct 3, 2023

Looks like the build failing is just a case of

res, err = transport_tpg.SendRequest(transport_tpg.SendRequestOptions{

assuming res would already exist. You can declare res or change it to instantiate in that line by making it :=
Looking into addressgroups to see if that transformation is safe

Yeah, made the change.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 158 files changed, 3153 insertions(+), 2552 deletions(-))
Terraform Beta: Diff ( 198 files changed, 4003 insertions(+), 3273 deletions(-))

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 158 files changed, 3153 insertions(+), 2552 deletions(-))
Terraform Beta: Diff ( 198 files changed, 4003 insertions(+), 3273 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3116
Passed tests 2797
Skipped tests: 315
Affected tests: 4

Action taken

Found 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccBigQueryDataTable_bigtable|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerCluster_withAddons|TestAccContainerNodePool_withKubeletConfig

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccBigQueryDataTable_bigtable[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerNodePool_withUpgradeSettings[Error message] [Debug log]
TestAccContainerCluster_withAddons[Error message] [Debug log]
TestAccContainerNodePool_withKubeletConfig[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

@@ -0,0 +1 @@
project = billingProject
Copy link
Member Author

@zli82016 zli82016 Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this line is before getting billingProject in update and delete method in the generated file and isn't working as expected
https://github.com/modular-magician/terraform-provider-google-beta/blob/be49b211522d9e2b34b84ff66268f8a9f8be5bf0/google-beta/services/networksecurity/resource_network_security_address_group.go#L366

Is it safe to move the pre_update just from line 776 to line 794 before the line of sendRequest in template file resource.erb? (The same action for pre_delete)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Let's make these "" in that case. Can you file an issue to fix the inconsistency in where pre_ methods are? That should happen in a dedicated PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I will file an issue.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 159 files changed, 3153 insertions(+), 2567 deletions(-))
Terraform Beta: Diff ( 199 files changed, 4003 insertions(+), 3288 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3117
Passed tests 2799
Skipped tests: 315
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccContainerCluster_withAddons|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerNodePool_withKubeletConfig

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerCluster_withAddons[Error message] [Debug log]
TestAccContainerNodePool_withUpgradeSettings[Error message] [Debug log]
TestAccContainerNodePool_withKubeletConfig[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

@zli82016 zli82016 merged commit c653104 into main Oct 4, 2023
6 checks passed
@zli82016 zli82016 deleted the labels-fix-terraform-labels branch October 4, 2023 02:54
trodge pushed a commit to trodge/magic-modules that referenced this pull request Oct 5, 2023
…erraform_labels field (GoogleCloudPlatform#9154)

* Don't send update request if updatmask is empty when resource has terraform_labels

* Remove the function check field_specific_update_methods

* Set project to billing project for google_network_security_address_group

* Change back of project for resource google_network_security_address_group
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error updating {{resource}}: The update_mask in the Update{{resource}}Request must be set
4 participants