Fix google_dns_managed_zone update #12934

shuyama1 · 2025-01-31T17:42:26Z

fixes b/372460202

Release Note Template for Downstream PRs (will be copied)

dns: fixed a bug where `google_dns_managed_zone` is unable to update with `service_directory_config` specified

modular-magician · 2025-01-31T17:52:01Z

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-beta provider: Diff ( 1 file changed, 124 insertions(+), 16 deletions(-))

modular-magician · 2025-01-31T17:53:28Z

Tests analytics

Total tests: 0
Passed tests: 0
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

🔴 Errors occurred during REPLAYING mode. Please fix them to complete your PR.

View the build log

modular-magician · 2025-01-31T18:29:29Z

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-beta provider: Diff ( 1 file changed, 124 insertions(+), 16 deletions(-))

modular-magician · 2025-01-31T18:31:20Z

Tests analytics

Total tests: 0
Passed tests: 0
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

🔴 Errors occurred during REPLAYING mode. Please fix them to complete your PR.

View the build log

modular-magician · 2025-01-31T18:54:33Z

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-beta provider: Diff ( 1 file changed, 108 insertions(+))

modular-magician · 2025-01-31T18:58:36Z

Tests analytics

Total tests: 48
Passed tests: 46
Skipped tests: 1
Affected tests: 1

Click here to see the affected service packages

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

Get to know how VCR tests work

modular-magician · 2025-01-31T18:59:49Z

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

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

shuyama1 · 2025-01-31T19:02:45Z

This failure is expected prior to the fix.

modular-magician · 2025-01-31T19:14:47Z

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-beta provider: Diff ( 2 files changed, 109 insertions(+), 1 deletion(-))
terraform-google-conversion: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2025-01-31T19:18:42Z

Tests analytics

Total tests: 48
Passed tests: 45
Skipped tests: 1
Affected tests: 2

Click here to see the affected service packages

Action taken

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

Get to know how VCR tests work

modular-magician · 2025-01-31T19:20:17Z

🟢 Tests passed during RECORDING mode:
TestAccDNSManagedZone_dnsManagedZoneServiceDirectoryExample [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

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

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

modular-magician · 2025-01-31T19:45:42Z

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-beta provider: Diff ( 2 files changed, 109 insertions(+), 1 deletion(-))
terraform-google-conversion: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2025-01-31T19:49:25Z

Tests analytics

Total tests: 48
Passed tests: 46
Skipped tests: 1
Affected tests: 1

Click here to see the affected service packages

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

Get to know how VCR tests work

modular-magician · 2025-01-31T19:51:14Z

🟢 Tests passed during RECORDING mode:
TestAccDNSManagedZone_dnsManagedZoneUpdateWithServiceDirectory [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

🟢 All tests passed!

View the build log or the debug log for each test

shuyama1 · 2025-02-04T20:04:50Z

mmv1/templates/terraform/custom_expand/sd_full_url.tmpl

@@ -4,7 +4,7 @@ func expand{{$.GetPrefix}}{{$.TitlelizeProperty}}(v interface{}, d tpgresource.T
  } else if strings.HasPrefix(v.(string), "https://") {
    return v, nil
  }
-  url, err := tpgresource.ReplaceVars(d, config, "{{"{{"}}ServiceDirectoryBasePath{{"}}"}}" + v.(string))
+  url, err := tpgresource.ReplaceVars(d, config, "https://servicedirectory.googleapis.com/v1/" + v.(string))


Confirmed by service team member @erikmej-code, https://servicedirectory.googleapis.com/v1/ is always used even if v1beta is explicitly sent

can we comment that in the code itself?

modular-magician · 2025-02-04T22:35: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.

google-beta provider: Diff ( 2 files changed, 110 insertions(+), 1 deletion(-))
terraform-google-conversion: Diff ( 1 file changed, 2 insertions(+), 1 deletion(-))

modular-magician · 2025-02-04T22:39:09Z

Tests analytics

Total tests: 48
Passed tests: 47
Skipped tests: 1
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

modular-magician · 2025-02-05T18:18:14Z

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-beta provider: Diff ( 2 files changed, 110 insertions(+), 1 deletion(-))
terraform-google-conversion: Diff ( 1 file changed, 2 insertions(+), 1 deletion(-))

modular-magician · 2025-02-05T18:22:16Z

Tests analytics

Total tests: 48
Passed tests: 47
Skipped tests: 1
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

rileykarson · 2025-02-05T21:40:14Z

mmv1/templates/terraform/custom_expand/sd_full_url.tmpl

@@ -4,7 +4,8 @@ func expand{{$.GetPrefix}}{{$.TitlelizeProperty}}(v interface{}, d tpgresource.T
  } else if strings.HasPrefix(v.(string), "https://") {
    return v, nil
  }
-  url, err := tpgresource.ReplaceVars(d, config, "{{"{{"}}ServiceDirectoryBasePath{{"}}"}}" + v.(string))
+  // v1 is the only version in use 
+  url, err := tpgresource.ReplaceVars(d, config, "https://servicedirectory.googleapis.com/v1/" + v.(string))


Could we substitute v1 for beta specifically or confirm this service isn't in alternate universes rather than providing a fixed URL here? Fixed URLs are mostly unsafe to use in checks nowadays, unfortunately!

Good point. It's necessary to do a substitution after the fact so we capture BasePath changes that have been applied before this code runs.

I believe substituting v1 for beta should be working in this case, I assume you mean something similarly to https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/tpgresource/self_link_helpers.go#L95. Yeah, that's safer than using fixed URLs

This approach works in GCP! I meant a substring replacement of v1 for beta rather than a fixed URL prefix like we're currently doing*

Sure, I'll revert this change and create a new one

Thanks! Sorry 😅

@c2thorn we may want to add a lint rule against fixed GCP URLs appearing in the provider outside of the specific spots they're safe to appear

Issue: hashicorp/terraform-provider-google#21266

This reverts commit 70b92ac.

modular-magician added the service/cloud-dns label Jan 31, 2025

shuyama1 force-pushed the fix_dns_update branch from 1c319ff to f045cd3 Compare January 31, 2025 18:19

shuyama1 force-pushed the fix_dns_update branch from f045cd3 to 5edae78 Compare January 31, 2025 18:44

shuyama1 mentioned this pull request Jan 31, 2025

Promoting namespace to the GA provider #12644

Open

shuyama1 commented Feb 4, 2025

View reviewed changes

shuyama1 requested a review from c2thorn February 4, 2025 20:04

c2thorn approved these changes Feb 4, 2025

View reviewed changes

shuyama1 force-pushed the fix_dns_update branch from fa3e0cb to 9fd8288 Compare February 4, 2025 22:28

shuyama1 added 4 commits February 5, 2025 10:07

added tests, expect tests to fail

65525ac

add the fix

e0c24a0

add tf-test prefix to resource names

b4ce70b

added comment

020aef6

shuyama1 force-pushed the fix_dns_update branch from 9fd8288 to 020aef6 Compare February 5, 2025 18:08

shuyama1 added this pull request to the merge queue Feb 5, 2025

Merged via the queue into GoogleCloudPlatform:main with commit 70b92ac Feb 5, 2025
20 checks passed

rileykarson reviewed Feb 5, 2025

View reviewed changes

shuyama1 added a commit that referenced this pull request Feb 5, 2025

Revert "Fix google_dns_managed_zone update (#12934)"

8bfeeb0

This reverts commit 70b92ac.

shuyama1 mentioned this pull request Feb 5, 2025

Revert "Fix google_dns_managed_zone update" #12976

Merged

shuyama1 mentioned this pull request Feb 5, 2025

Fix google_dns_managed_zone update #12978

Merged

Dawid212 pushed a commit to Dawid212/magic-modules that referenced this pull request Feb 12, 2025

Fix google_dns_managed_zone update (GoogleCloudPlatform#12934)

ea78db5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix google_dns_managed_zone update #12934

Fix google_dns_managed_zone update #12934

shuyama1 commented Jan 31, 2025 •

edited

Loading

modular-magician commented Jan 31, 2025

modular-magician commented Jan 31, 2025

modular-magician commented Jan 31, 2025

modular-magician commented Jan 31, 2025

modular-magician commented Jan 31, 2025

modular-magician commented Jan 31, 2025

modular-magician commented Jan 31, 2025

shuyama1 commented Jan 31, 2025

modular-magician commented Jan 31, 2025

modular-magician commented Jan 31, 2025

modular-magician commented Jan 31, 2025

modular-magician commented Jan 31, 2025

modular-magician commented Jan 31, 2025

modular-magician commented Jan 31, 2025

shuyama1 Feb 4, 2025

c2thorn Feb 4, 2025

modular-magician commented Feb 4, 2025

modular-magician commented Feb 4, 2025

modular-magician commented Feb 5, 2025

modular-magician commented Feb 5, 2025

rileykarson Feb 5, 2025

c2thorn Feb 5, 2025

shuyama1 Feb 5, 2025

rileykarson Feb 5, 2025

shuyama1 Feb 5, 2025

rileykarson Feb 5, 2025

rileykarson Feb 5, 2025

Fix google_dns_managed_zone update #12934

Fix google_dns_managed_zone update #12934

Conversation

shuyama1 commented Jan 31, 2025 • edited Loading

modular-magician commented Jan 31, 2025

Diff report

modular-magician commented Jan 31, 2025

Tests analytics

modular-magician commented Jan 31, 2025

Diff report

modular-magician commented Jan 31, 2025

Tests analytics

modular-magician commented Jan 31, 2025

Diff report

modular-magician commented Jan 31, 2025

Tests analytics

Action taken

modular-magician commented Jan 31, 2025

shuyama1 commented Jan 31, 2025

modular-magician commented Jan 31, 2025

Diff report

modular-magician commented Jan 31, 2025

Tests analytics

Action taken

modular-magician commented Jan 31, 2025

modular-magician commented Jan 31, 2025

Diff report

modular-magician commented Jan 31, 2025

Tests analytics

Action taken

modular-magician commented Jan 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Feb 4, 2025

Diff report

modular-magician commented Feb 4, 2025

Tests analytics

modular-magician commented Feb 5, 2025

Diff report

modular-magician commented Feb 5, 2025

Tests analytics

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuyama1 commented Jan 31, 2025 •

edited

Loading