Make billing budget filter credit_types and subaccounts updatable #7035

zli82016 · 2022-12-22T23:09:52Z

Fixes https://b.corp.google.com/issues/242945866

Make fields credit_types and subaccounts updatable

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

billingbudget: Make fields `credit_types` and `subaccounts` updatable for `google_billing_budget`

modular-magician · 2022-12-22T23:24:57Z

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

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

field-oc-to-c - reference
field-oc-to-c - reference

If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An override-breaking-change label can be added to allow merging.

Diff report

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

Terraform GA: Diff ( 4 files changed, 83 insertions(+), 11 deletions(-))
Terraform Beta: Diff ( 4 files changed, 83 insertions(+), 11 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-12-23T00:14:44Z

Tests analytics

Total tests: 2381
Passed tests 2126
Skipped tests: 248
Failed tests: 7

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease|TestAccBillingSubaccount_renameOnDestroy|TestAccContainerCluster_withInvalidGatewayApiConfigChannel|TestAccBillingBudget_billingBudgetUpdate|TestAccBillingBudget_billingBudgetFilterExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccBillingBudget_billingFilterSubaccounts

modular-magician · 2022-12-23T00:15:57Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccBillingSubaccount_renameOnDestroy[Debug log]
TestAccContainerCluster_withInvalidGatewayApiConfigChannel[Debug log]
TestAccBillingBudget_billingBudgetUpdate[Debug log]
TestAccBillingBudget_billingBudgetFilterExample[Debug log]
TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample[Debug log]
TestAccBillingBudget_billingFilterSubaccounts[Debug log]

All tests passed
View the build log or the debug log for each test

trodge

The breaking change detection is complaining because we're removing O+C from budget_filter.credit_types and budget_filter.subaccounts, but I can't figure out why these fields needed to be O+C in the first place. The overrides were added in #4273 but the test that was failing, TestAccBillingBudget_billingBudgetBasicExample, seemed to only require budget_filter and budget_filter.credit_types_treatment be O+C.

trodge · 2023-01-03T17:09:54Z

mmv1/products/billingbudget/terraform.yaml

@@ -93,8 +93,6 @@ overrides: !ruby/object:Overrides::ResourceOverrides
        custom_flatten: 'templates/terraform/custom_flatten/object_to_bool.go.erb'
      amount.specifiedAmount.currencyCode: !ruby/object:Overrides::Terraform::PropertyOverride
        default_from_api: true
-      budgetFilter.creditTypes: !ruby/object:Overrides::Terraform::PropertyOverride


Do these overrides need to be removed to make the field updatable?

Without removing the overrides, the test testAccBillingBudget_billingBudgetCustomPeriodUpdate failed. https://github.com/GoogleCloudPlatform/magic-modules/pull/7035/files#diff-a5c0fe03fa3b54255bdca47bd1415a24c2de3ec1715a457db27a558bd0e1b530R114.

The error is "Credit types treatment of INCLUDE_ALL_CREDITS or EXCLUDE_ALL_CREDITS requires empty list of credit_types".

The reason is that creditTypes is sent to API because of the computed override even the user does not set that field in config.

I checked the response from API and the API does not return default values for the two fields. These two fields do not behave like computed fields.

This should be able to be fixed while retaining the computed property. Even though this is a mistake these changes will break end users.

@ScottSuarez , do you have an idea how to fix it while retaining the computed property?

ScottSuarez · 2023-01-03T18:45:41Z

Even if a value does not need to be O+C.. A transition from O+C to O is breaking as terraform state is stored for the previously O+C field. So any subsequent apply will try to resolve the diff by removing any previous value.

zli82016 · 2023-01-03T18:53:05Z

Is this change allowed to merge to main branch with the override-breaking-change label? Or it needs to wait for a major release?

zli82016 · 2023-01-03T21:39:18Z

I tested with the config https://paste.googleplex.com/6534593615233024 and current provider. The config contains the field credit_types, which is one field in the PR from O+C -> O. And then build the new provider and do terraform plan. The plan is empty.

In this special case, is it not a breaking change?

trodge · 2023-01-03T22:51:47Z

I tested with the config https://paste.googleplex.com/6534593615233024 and current provider. The config contains the field credit_types, which is one field in the PR from O+C -> O. And then build the new provider and do terraform plan. The plan is empty.

In this special case, is it not a breaking change?

I think the case we want to test is one where budget_filters is specified but credit_types and subaccounts are not.

zli82016 · 2023-01-03T23:08:26Z

I tested with the config https://paste.googleplex.com/6534593615233024 and current provider. The config contains the field credit_types, which is one field in the PR from O+C -> O. And then build the new provider and do terraform plan. The plan is empty.
In this special case, is it not a breaking change?

I think the case we want to test is one where budget_filters is specified but credit_types and subaccounts are not.

I also tested this case that credit_types and subaccounts are not set. The plan is also empty with the new provider.

ScottSuarez · 2023-01-06T18:15:28Z

I tested with the config paste.googleplex.com/6534593615233024 and current provider. The config contains the field credit_types, which is one field in the PR from O+C -> O. And then build the new provider and do terraform plan. The plan is empty.
In this special case, is it not a breaking change?

I think the case we want to test is one where budget_filters is specified but credit_types and subaccounts are not.

I also tested this case that credit_types and subaccounts are not set. The plan is also empty with the new provider.

I can confirm. Also tested this scenario

zli82016 · 2023-01-10T17:58:25Z

In this PR, C + O of fields credit_types and subaccounts are changed to O. I tested the following cases with built provider and do not see diff for terraform plan.

Without lifecycle.ignore_changes
- With the fields credit_types and subaccounts (config: https://paste.googleplex.com/6522104118771712)
- Without the fields credit_types and subaccounts (config: https://paste.googleplex.com/6561475010232320)
With lifecycle.ignore_changes
- With the fields credit_types and subaccounts (config: https://paste.googleplex.com/4878558781505536)

It looks like it is because the fields don't behave like computed and don't have default values in API side, the changes from C + O -> C is not breaking.

zli82016 · 2023-01-11T18:37:26Z

Talked with @ScottSuarez , as the change from C + O to O is a breaking change, we will keep Computed property now. To remove the fields credit_types and subaccounts in the config, the user has to set the fields to be an empty array in the config as the workaround.

Created a bug hashicorp/terraform-provider-google#13456 and we need to remove Computed for these fields in the next major release.

modular-magician · 2023-01-11T18:55:23Z

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 ( 4 files changed, 139 insertions(+), 12 deletions(-))
Terraform Beta: Diff ( 4 files changed, 139 insertions(+), 12 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2023-01-11T19:15:49Z

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 ( 4 files changed, 139 insertions(+), 12 deletions(-))
Terraform Beta: Diff ( 4 files changed, 139 insertions(+), 12 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2023-01-11T20:05:41Z

Tests analytics

Total tests: 2404
Passed tests 2150
Skipped tests: 251
Failed tests: 3

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccContainerCluster_withInvalidGatewayApiConfigChannel|TestAccBillingBudget_billingFilterSubaccounts|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample

modular-magician · 2023-01-11T20:08:23Z

Tests passed during RECORDING mode:
TestAccContainerCluster_withInvalidGatewayApiConfigChannel[Debug log]
TestAccBillingBudget_billingFilterSubaccounts[Debug log]

Tests failed during RECORDING mode:
TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

trodge

LGTM

zli82016 requested review from a team and trodge and removed request for a team December 22, 2022 23:11

trodge reviewed Jan 3, 2023

View reviewed changes

trodge requested a review from ScottSuarez January 3, 2023 17:24

ScottSuarez mentioned this pull request Jan 3, 2023

Fix issue where breaking change message was corrupted. #7069

Merged

zli82016 force-pushed the bug-242945866 branch from 4c664f5 to 5e84578 Compare January 11, 2023 18:40

Make billing budget filter credit_types and subaccounts updatable

886d117

zli82016 force-pushed the bug-242945866 branch from 5e84578 to 886d117 Compare January 11, 2023 19:02

zli82016 requested a review from trodge January 11, 2023 23:51

trodge reviewed Jan 12, 2023

View reviewed changes

ScottSuarez approved these changes Jan 12, 2023

View reviewed changes

ScottSuarez merged commit 1372e1d into main Jan 12, 2023

ScottSuarez deleted the bug-242945866 branch January 12, 2023 01:23

modular-magician mentioned this pull request Jan 12, 2023

Make billing budget filter credit_types and subaccounts updatable hashicorp/terraform-provider-google-beta#5087

Merged

5 tasks

This was referenced Jan 12, 2023

Make billing budget filter credit_types and subaccounts updatable hashicorp/terraform-provider-google#13466

Merged

Make billing budget filter credit_types and subaccounts updatable GoogleCloudPlatform/terraform-validator#1278

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make billing budget filter credit_types and subaccounts updatable #7035

Make billing budget filter credit_types and subaccounts updatable #7035

zli82016 commented Dec 22, 2022

modular-magician commented Dec 22, 2022

modular-magician commented Dec 23, 2022

modular-magician commented Dec 23, 2022

trodge left a comment

trodge Jan 3, 2023

zli82016 Jan 3, 2023 •

edited

Loading

zli82016 Jan 3, 2023

ScottSuarez Jan 3, 2023

zli82016 Jan 10, 2023

ScottSuarez commented Jan 3, 2023

zli82016 commented Jan 3, 2023

zli82016 commented Jan 3, 2023

trodge commented Jan 3, 2023

zli82016 commented Jan 3, 2023

ScottSuarez commented Jan 6, 2023

zli82016 commented Jan 10, 2023 •

edited

Loading

zli82016 commented Jan 11, 2023 •

edited

Loading

modular-magician commented Jan 11, 2023

modular-magician commented Jan 11, 2023

modular-magician commented Jan 11, 2023

modular-magician commented Jan 11, 2023

trodge left a comment

Make billing budget filter credit_types and subaccounts updatable #7035

Make billing budget filter credit_types and subaccounts updatable #7035

Conversation

zli82016 commented Dec 22, 2022

modular-magician commented Dec 22, 2022

Breaking Change(s) Detected

Diff report

modular-magician commented Dec 23, 2022

Tests analytics

Action taken

modular-magician commented Dec 23, 2022

trodge left a comment

Choose a reason for hiding this comment

trodge Jan 3, 2023

Choose a reason for hiding this comment

zli82016 Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

zli82016 Jan 3, 2023

Choose a reason for hiding this comment

ScottSuarez Jan 3, 2023

Choose a reason for hiding this comment

zli82016 Jan 10, 2023

Choose a reason for hiding this comment

ScottSuarez commented Jan 3, 2023

zli82016 commented Jan 3, 2023

zli82016 commented Jan 3, 2023

trodge commented Jan 3, 2023

zli82016 commented Jan 3, 2023

ScottSuarez commented Jan 6, 2023

zli82016 commented Jan 10, 2023 • edited Loading

zli82016 commented Jan 11, 2023 • edited Loading

modular-magician commented Jan 11, 2023

Diff report

modular-magician commented Jan 11, 2023

Diff report

modular-magician commented Jan 11, 2023

Tests analytics

Action taken

modular-magician commented Jan 11, 2023

trodge left a comment

Choose a reason for hiding this comment

zli82016 Jan 3, 2023 •

edited

Loading

zli82016 commented Jan 10, 2023 •

edited

Loading

zli82016 commented Jan 11, 2023 •

edited

Loading