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

add send_secondary_ip_range_if_empty #11410

Merged

Conversation

c2thorn
Copy link
Member

@c2thorn c2thorn commented Aug 9, 2024

part of hashicorp/terraform-provider-google#12824 and hashicorp/terraform-provider-google#17881

adds a virtual field to replace the need to set secondary_ip_range = [] to remove secondary_ip_range explicitly. The virtual field in combination with GetRawConfig().GetAttr("secondary_ip_range") can let us detect when the user has intent to set to an empty list vs intending to let the backend manage the field.

Normally, when removing an Optional+Computed field from config, Terraform will not produce a diff. The sendSecondaryIpRangeIfEmptyDiff checks if the new flag is set and then compares the config value with the state value using GetRawConfig() and GetOk respectively.

The actual update call is handled in a post_update custom code block.

This logic works with and without SchemaConfigModeAttr, allowing an upgrade path in 5.x before removing SchemaConfigModeAttr from secondary_ip_range in 6.0

Release Note Template for Downstream PRs (will be copied)

compute: setting `google_compute_subnetwork.secondary_ip_range = []` to explicitly set a list of empty objects is deprecated and will produce an error in the upcoming major release. Use `send_secondary_ip_range_if_empty` while removing `secondary_ip_range` from config instead.
compute: added `send_secondary_ip_range_if_empty` to `google_compute_subnetwork`

@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.

google provider: Diff ( 4 files changed, 304 insertions(+), 10 deletions(-))
google-beta provider: Diff ( 4 files changed, 304 insertions(+), 10 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 29 insertions(+))

1 similar comment
@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.

google provider: Diff ( 4 files changed, 304 insertions(+), 10 deletions(-))
google-beta provider: Diff ( 4 files changed, 304 insertions(+), 10 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 29 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 980
Passed tests: 907
Skipped tests: 72
Affected tests: 1

Click here to see the affected service packages
  • compute

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccComputeSubnetwork_secondaryIpRanges_sendEmpty

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

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

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


$\textcolor{green}{\textsf{All tests passed!}}$

View the build log or the debug log for each test

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 980
Passed tests: 907
Skipped tests: 72
Affected tests: 1

Click here to see the affected service packages
  • compute

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccComputeSubnetwork_secondaryIpRanges_sendEmpty

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

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

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


$\textcolor{green}{\textsf{All tests passed!}}$

View the build log or the debug log for each test

mmv1/products/compute/Subnetwork.yaml Show resolved Hide resolved
@@ -0,0 +1,72 @@
if v, ok := d.GetOk("send_secondary_ip_range_if_empty"); ok && v.(bool) {
if sv, ok := d.GetOk("secondary_ip_range"); ok {
configValue := d.GetRawConfig().GetAttr("secondary_ip_range").AsValueSlice()
Copy link
Member

Choose a reason for hiding this comment

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

What case does this handle, if we've presumably set a known zero value for the field in the CustomizeDiff? Theoretically wouldn't we always get the correct value from d.Get?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is necessary when we remove ConfigModeAttr. After removing it, d.GetChange/d.HasChange loses the diff from the plan, so this ended up necessary to actually trigger the update. It works the same for with and without ConfigModeAttr

Copy link
Member

Choose a reason for hiding this comment

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

d.GetChange/d.HasChange loses the diff from the plan

Just to make sure I have this right- terraform plan displays a diff, but those functions- the "safe" functions in the SDK- report that it did not?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes this is a local run without ConfigModeAttr and some added debug logs: gpaste/5449355903631360

setting secondary_ip_range to newly empty line from the customDiff appears twice as expected

paste/5449355903631360#l=399 are the print logs I added. Here's the code

For good measure, I even added another debug log for if the previous MMv1-generated update code is even triggering. It does not appear

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@c2thorn c2thorn force-pushed the subnetwork-secondary-ip-range branch from bb20711 to 4cef0f3 Compare August 12, 2024 15:59
@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.

google provider: Diff ( 4 files changed, 308 insertions(+), 8 deletions(-))
google-beta provider: Diff ( 4 files changed, 308 insertions(+), 8 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 31 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 980
Passed tests: 908
Skipped tests: 72
Affected tests: 0

Click here to see the affected service packages
  • compute

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log

Co-authored-by: Riley Karson <rileykarson@google.com>
@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.

google provider: Diff ( 4 files changed, 309 insertions(+), 8 deletions(-))
google-beta provider: Diff ( 4 files changed, 309 insertions(+), 8 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 32 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 980
Passed tests: 908
Skipped tests: 72
Affected tests: 0

Click here to see the affected service packages
  • compute

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log

@c2thorn c2thorn merged commit 4472fff into GoogleCloudPlatform:main Aug 13, 2024
14 checks passed
@c2thorn c2thorn deleted the subnetwork-secondary-ip-range branch August 13, 2024 21:10
BBBmau pushed a commit to bschaatsbergen/magic-modules that referenced this pull request Aug 21, 2024
Co-authored-by: Riley Karson <rileykarson@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants