compute: added `numeric_id` to `google_compute_subnetwork` #12285

wyardley · 2024-11-08T17:31:28Z

Added numeric_id to google_compute_subnetwork resource and data source.

Since Terraform uses id internally, follow the example of google_compute_network, and make numeric_id contain the id field from the API.

As mentioned in the issue below, it would be very nice if there were a simpler way to just say "map id in the API to numericId here", but guessing that's not possible?

Part of hashicorp/terraform-provider-google#20223

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.

compute: added `numeric_id` to `google_compute_subnetwork` resource and data source

github-actions · 2024-11-08T17:32:08Z

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

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

mmv1/products/compute/Subnetwork.yaml

wyardley · 2024-11-08T17:55:59Z

mmv1/third_party/terraform/services/compute/data_source_google_compute_subnetwork.go.tmpl

@@ -100,6 +105,10 @@ func dataSourceGoogleComputeSubnetworkRead(d *schema.ResourceData, meta interfac
 		return transport_tpg.HandleDataSourceNotFoundError(err, d, fmt.Sprintf("Subnetwork Not Found : %s", name), id)
 	}

+	if err := d.Set("numeric_id", strconv.Itoa(int(subnetwork.Id))); err != nil {


I am assuming this is pretty reliably an int64; not sure if this is the right place to do the conversion and / or if any more error checking is needed here, but it seems to work and tests pass.

if we mark numericId as an integer we shouldn't need to do this extra conversion unless there's something that's preventing integer to be used over string

See comment above; as best I could tell, other similar fields (in particular, the one in google_compute_network that this is based on) are implemented as strings… I could make it a number in terraform if you can confirm that’s the right thing to do, but would be nice to understand why so many of them are already implemented as strings.

Google project number, org ID, etc. also seems to be implemented as strings vs. int

magic-modules/mmv1/third_party/terraform/services/resourcemanager/resource_google_project.go

Line 104 in 8179902

Type: schema.TypeString,

modular-magician · 2024-11-08T18:35:04Z

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

modular-magician · 2024-11-08T18:54:54Z

Tests analytics

Total tests: 1061
Passed tests: 987
Skipped tests: 73
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_numericId

Get to know how VCR tests work

modular-magician · 2024-11-08T18:57:24Z

🟢 Tests passed during RECORDING mode:
TestAccComputeSubnetwork_numericId [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

wyardley · 2024-11-15T17:49:16Z

@SarahFrench @melinath either of you have any background / perspective on what things should be implemented as strings vs numbers within the provider?

melinath · 2024-11-15T18:14:45Z

in general, it should match the API type. I don't know why the other field you mentioned is a string instead.

wyardley · 2024-11-15T18:25:08Z

So then I guess the question is whether we should follow the other examples within this resource type for consistency, or update it to be a number. I’m happy to do whatever is preferred.

I can test, but is there any type conversion needed if one were interpolating the number value within a string in terraform itself?

melinath · 2024-11-15T18:27:52Z

the only reason I can think of to use a string would be if a user needed to be able to set "0" on an optional + computed field - but I would still hesitate doing that since it would be a hacky workaround, and this isn't an optional + computed field so that wouldn't apply anyway. I'd say just go with matching the API.

Added `numeric_id` to `google_compute_subnetwork` resource and data source. Since Terraform uses `id` internally, follow the example of `google_compute_network`, and make `numeric_id` contain the `id` field from the API. Part of hashicorp/terraform-provider-google#20223

wyardley · 2024-11-15T19:05:08Z

@BBBmau Updated to be an integer. Tests seem to pass, and also functionally tested against the built provider.

Might be smart to switch over google_compute_network.numeric_id to match at some point?

Add `numeric_id` to `google_compute_network` data source, matching the resource. Switch `numeric_id` to to be an integer internally within the `google_compute_network` resource for consistency with the API and with `google_compute_subnetwork`. Followup to GoogleCloudPlatform#12285 Part of hashicorp/terraform-provider-google#20223

wyardley · 2024-11-15T19:20:34Z

Created #12339 to update this for the google_compute_network resource, while also adding it to the data source for parity.

wyardley · 2024-11-15T20:48:48Z

Is it expected that the "awaiting approval" labels aren't seeming to get applied / removed?

wyardley · 2024-11-15T20:57:07Z

mmv1/third_party/terraform/services/compute/resource_compute_subnetwork_test.go.tmpl

+			{
+				Config: testAccComputeSubnetwork_numericId(cnName, subnetworkName),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestMatchResourceAttr("google_compute_subnetwork.numeric_id_test", "numeric_id", regexp.MustCompile("^\\d{16,48}$")),


There's no builtin check on type here, right? Like to assert that it's an int or that it's a string?

wyardley · 2024-11-15T20:57:45Z

mmv1/third_party/terraform/services/compute/data_source_google_compute_subnetwork.go.tmpl

@@ -100,6 +104,10 @@ func dataSourceGoogleComputeSubnetworkRead(d *schema.ResourceData, meta interfac
 		return transport_tpg.HandleDataSourceNotFoundError(err, d, fmt.Sprintf("Subnetwork Not Found : %s", name), id)
 	}

+	if err := d.Set("numeric_id", subnetwork.Id); err != nil {


I didn't seem to need to convert from int64 to int here, even though I did when I was doing string conversion. Let me know if there should be an int(subnetwork.Id) here.

modular-magician · 2024-11-18T17:35:27Z

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

modular-magician · 2024-11-18T17:54:42Z

Tests analytics

Total tests: 1065
Passed tests: 991
Skipped tests: 73
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

TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy

Get to know how VCR tests work

modular-magician · 2024-11-18T18:25:11Z

🔴 Tests failed during RECORDING mode:
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy [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

wyardley · 2024-11-18T20:43:01Z

@SarahFrench pointed out https://googlecloudplatform.github.io/magic-modules/develop/field-reference/#api_name in the docs, which might be simpler than the custom templates if it works?

I think the test failure is unrelated?

wyardley · 2024-11-20T00:10:56Z

Maybe not needed after #12351?

ScottSuarez · 2024-11-20T00:21:05Z

Maybe not needed after #12351?

Ack, yeah this can be closed. Although if you want you could add it to the datasource still @wyardley

wyardley · 2024-11-20T00:24:28Z

I will look at it again in a week or two. Opened #12339 a bit back for compute_network as well

Add `numeric_id` to `google_compute_network` data source, matching the resource. Switch `numeric_id` to to be an integer internally within the `google_compute_network` resource for consistency with the API and with `google_compute_subnetwork`. Followup to GoogleCloudPlatform#12285 Part of hashicorp/terraform-provider-google#20223

Added `subnetwork_id` to `google_compute_subnetwork` data source Picked from GoogleCloudPlatform#12285 Followup to GoogleCloudPlatform#12351 Related to hashicorp/terraform-provider-google#20223

wyardley · 2024-12-02T04:24:06Z

Ack, yeah this can be closed. Although if you want you could add it to the datasource still @wyardley

@ScottSuarez Thanks. Seems like your changes used foo.foo_id vs foo.numeric_id? If that's the convention, if a breaking change is made later to add the network ID as an integer, maybe it would make sense to make that change? or even to add google_compute_network.network_id ahead of time as an integer, and remove google_compute_network.numeric_id as a breaking change later, to make network and subnetwork consistent both in type and in naming?

I made a followup PR for adding the data source only (basically cherry-picking part of this PR): #12461

Also made hashicorp/terraform-provider-google#20530 to track some of the inconsistencies of name / type that might be breaking changes.

github-actions bot requested a review from BBBmau November 8, 2024 17:32

modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Nov 8, 2024

wyardley force-pushed the wyardley/issues_20223_pt1 branch from a5e00b1 to 3312a35 Compare November 8, 2024 17:33

wyardley commented Nov 8, 2024

View reviewed changes

mmv1/products/compute/Subnetwork.yaml Show resolved Hide resolved

wyardley mentioned this pull request Nov 8, 2024

numeric id required for tag binding is missing on many resources hashicorp/terraform-provider-google#20223

Closed

wyardley commented Nov 8, 2024

View reviewed changes

mmv1/products/compute/Subnetwork.yaml Outdated Show resolved Hide resolved

wyardley commented Nov 8, 2024

View reviewed changes

modular-magician added service/compute-vpc and removed awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests labels Nov 8, 2024

wyardley added 2 commits November 15, 2024 11:04

Switch to integer vs. string

06cf7a4

wyardley force-pushed the wyardley/issues_20223_pt1 branch from 293a0a5 to 06cf7a4 Compare November 15, 2024 19:05

wyardley mentioned this pull request Nov 15, 2024

compute: added numeric_id to google_compute_network data source #12339

Merged

wyardley commented Nov 15, 2024

View reviewed changes

ScottSuarez closed this Nov 20, 2024

wyardley deleted the wyardley/issues_20223_pt1 branch November 20, 2024 00:24

wyardley mentioned this pull request Dec 2, 2024

Standardize type and naming for numeric IDs hashicorp/terraform-provider-google#20530

Open

wyardley mentioned this pull request Dec 2, 2024

compute: added subnetwork_id to compute subnet data source #12461

Merged

This was referenced Dec 2, 2024

compute: added numeric_id to google_compute_network data source hashicorp/terraform-provider-google-beta#8821

Merged

compute: added numeric_id to google_compute_network data source hashicorp/terraform-provider-google#20548

Merged

wyardley mentioned this pull request Dec 5, 2024

compute: added network_id to google_compute_network #12504

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compute: added `numeric_id` to `google_compute_subnetwork` #12285

compute: added `numeric_id` to `google_compute_subnetwork` #12285

wyardley commented Nov 8, 2024 •

edited

Loading

github-actions bot commented Nov 8, 2024

wyardley Nov 8, 2024

BBBmau Nov 12, 2024

wyardley Nov 12, 2024 •

edited

Loading

wyardley Nov 12, 2024

modular-magician commented Nov 8, 2024

modular-magician commented Nov 8, 2024

modular-magician commented Nov 8, 2024

wyardley commented Nov 15, 2024

melinath commented Nov 15, 2024

wyardley commented Nov 15, 2024

melinath commented Nov 15, 2024

wyardley commented Nov 15, 2024 •

edited

Loading

wyardley commented Nov 15, 2024

wyardley commented Nov 15, 2024

wyardley Nov 15, 2024

wyardley Nov 15, 2024

modular-magician commented Nov 18, 2024

modular-magician commented Nov 18, 2024

modular-magician commented Nov 18, 2024

wyardley commented Nov 18, 2024

wyardley commented Nov 20, 2024

ScottSuarez commented Nov 20, 2024

wyardley commented Nov 20, 2024

wyardley commented Dec 2, 2024

compute: added numeric_id to google_compute_subnetwork #12285

compute: added numeric_id to google_compute_subnetwork #12285

Conversation

wyardley commented Nov 8, 2024 • edited Loading

github-actions bot commented Nov 8, 2024

wyardley Nov 8, 2024

Choose a reason for hiding this comment

BBBmau Nov 12, 2024

Choose a reason for hiding this comment

wyardley Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

wyardley Nov 12, 2024

Choose a reason for hiding this comment

modular-magician commented Nov 8, 2024

Diff report

modular-magician commented Nov 8, 2024

Tests analytics

Action taken

modular-magician commented Nov 8, 2024

wyardley commented Nov 15, 2024

melinath commented Nov 15, 2024

wyardley commented Nov 15, 2024

melinath commented Nov 15, 2024

wyardley commented Nov 15, 2024 • edited Loading

wyardley commented Nov 15, 2024

wyardley commented Nov 15, 2024

wyardley Nov 15, 2024

Choose a reason for hiding this comment

wyardley Nov 15, 2024

Choose a reason for hiding this comment

modular-magician commented Nov 18, 2024

Diff report

modular-magician commented Nov 18, 2024

Tests analytics

Action taken

modular-magician commented Nov 18, 2024

wyardley commented Nov 18, 2024

wyardley commented Nov 20, 2024

ScottSuarez commented Nov 20, 2024

wyardley commented Nov 20, 2024

wyardley commented Dec 2, 2024

compute: added `numeric_id` to `google_compute_subnetwork` #12285

compute: added `numeric_id` to `google_compute_subnetwork` #12285

wyardley commented Nov 8, 2024 •

edited

Loading

wyardley Nov 12, 2024 •

edited

Loading

wyardley commented Nov 15, 2024 •

edited

Loading