Prioritize disk.DiskSizeGb over disk.InitializeParams.DiskSizeGb #8246

seanrmilligan · 2023-06-30T17:08:42Z

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).
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).
Generated Terraform providers, and ran make test and make lint in the generated providers to ensure it passes unit and linter tests.
Ran relevant acceptance tests using my own Google Cloud project and credentials (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read Write release notes before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

…rams.DiskSizeGb

…odules

modular-magician · 2023-06-30T17:08:59Z

Oops! It looks like you're using an unknown release-note type in your changelog entries:

REPLACEME

Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md.

modular-magician · 2023-06-30T17:09:15Z

Hello! I am a robot who works on Magic Modules PRs.

I've detected that you're a community contributor. @hao-nan-li, a repository maintainer, has been assigned to assist you and help review your changes.

❓ First time contributing? Click here for more details

Your assigned reviewer will help review your code by:

Ensuring it's backwards compatible, covers common error cases, etc.
Summarizing the change into a user-facing changelog note.
Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.

seanrmilligan · 2023-06-30T17:13:52Z

Note to self to fix the whitespace alignment of added code.

seanrmilligan · 2023-06-30T17:17:00Z

"disk_size_gb2" needs a better name, but I left it as a placeholder so we could have that conversation in the review.

modular-magician · 2023-06-30T17:23: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 ( 1 file changed, 31 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 1 file changed, 31 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_compute_instance_template (121 total tests)
Untested fields: disk.disk_size_gb2

Please add acceptance tests which include these fields.

modular-magician · 2023-06-30T17:58:53Z

The provider crashed while running the VCR tests in REPLAYING mode
Please fix it to complete your PR
View the build log

melinath

FYI tests are crashing due to google_compute_region_instance_template reusing the flatteners and expanders from google_compute_instance_template. Any additional fields would also need to be added there.

modular-magician · 2023-07-05T16:41:21Z

Oops! It looks like you're using an unknown release-note type in your changelog entries:

REPLACEME

Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md.

modular-magician · 2023-07-05T16:56:03Z

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 ( 1 file changed, 31 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 1 file changed, 31 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_compute_instance_template (121 total tests)
Untested fields: disk.disk_size_gb2

Please add acceptance tests which include these fields.

modular-magician · 2023-07-05T17:30:03Z

The provider crashed while running the VCR tests in REPLAYING mode
Please fix it to complete your PR
View the build log

…odules

modular-magician · 2023-07-05T19:27:50Z

Oops! It looks like you're using an unknown release-note type in your changelog entries:

REPLACEME

Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md.

modular-magician · 2023-07-05T19:40: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.

Terraform GA: Diff ( 1 file changed, 12 insertions(+), 5 deletions(-))
Terraform Beta: Diff ( 1 file changed, 12 insertions(+), 5 deletions(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2023-07-05T19:54:57Z

Oops! It looks like you're using an unknown release-note type in your changelog entries:

REPLACEME

Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md.

seanrmilligan · 2023-07-05T20:01:55Z

mmv1/third_party/terraform/services/compute/resource_compute_instance_template.go.erb

+		// The API does not return a disk size value for scratch disks. They are largely only one size,
+		// so we can assume that size here. Prefer disk.DiskSizeGb over the deprecated
+		// disk.InitializeParams.DiskSizeGb.
+		if disk.DiskSizeGb == 0 && disk.InitializeParams.DiskSizeGb == 0 && disk.Type == "SCRATCH" {


Possible logic bug, past and present: If the disk is not of type SCRATCH, we move past the if into the else-if and else blocks. I can put a guard here wrapping this entire section checking for SCRATCH, but it more closely emulates the existing code to not do so.

This looks correct as-is to me. If the disk type is not SCRATCH and both DiskSizeGb fields are 0 we can set disk_size_gb to 0.

modular-magician · 2023-07-05T20:08:08Z

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 ( 1 file changed, 11 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 1 file changed, 11 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

trodge · 2023-07-05T20:44:19Z

mmv1/third_party/terraform/services/compute/resource_compute_instance_template.go.erb

+		// The API does not return a disk size value for scratch disks. They are largely only one size,
+		// so we can assume that size here. Prefer disk.DiskSizeGb over the deprecated
+		// disk.InitializeParams.DiskSizeGb.
+		if disk.DiskSizeGb == 0 && disk.InitializeParams.DiskSizeGb == 0 && disk.Type == "SCRATCH" {


This looks correct as-is to me. If the disk type is not SCRATCH and both DiskSizeGb fields are 0 we can set disk_size_gb to 0.

modular-magician · 2023-07-05T21:27:01Z

Tests analytics

Total tests: 2836
Passed tests 2508
Skipped tests: 295
Affected tests: 33

Action taken

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

TestAccComputeRegionInstanceTemplate_minCpuPlatform|TestAccComputeRegionInstanceTemplate_guestAcceleratorSkip|TestAccComputeRegionInstanceTemplate_guestAccelerator|TestAccComputeRegionInstanceTemplate_secondaryAliasIpRange|TestAccComputeRegionInstanceTemplate_primaryAliasIpRange|TestAccComputeRegionInstanceTemplate_metadata_startup_script|TestAccComputeRegionInstanceTemplate_subnet_custom|TestAccComputeRegionAutoscaler_regionAutoscalerBasicExample|TestAccComputeRegionInstanceTemplate_subnet_auto|TestAccComputeRegionInstanceTemplate_regionDisks|TestAccComputeInstanceTemplate_withScratchDisk|TestAccComputeInstanceFromTemplate_012_removableFields|TestAccComputeInstanceFromTemplate_overrideScratchDisk|TestAccComputeInstanceFromTemplate_overrideAttachedDisk|TestAccComputeInstanceFromTemplate_overrideBootDisk|TestAccComputeInstanceFromRegionTemplate_basic|TestAccComputeInstanceFromTemplate_self_link_unique|TestAccComputeInstanceFromTemplate_basic|TestAccComputeInstanceTemplate_minCpuPlatform|TestAccComputeInstanceTemplate_guestAcceleratorSkip|TestAccComputeInstanceTemplate_guestAccelerator|TestAccComputeInstanceTemplate_secondaryAliasIpRange|TestAccComputeInstanceTemplate_primaryAliasIpRange|TestAccComputeInstanceTemplate_metadata_startup_script|TestAccComputeFirewallPolicyRule_multipleRules|TestAccComputeRegionInstanceTemplate_with18TbScratchDisk|TestAccComputeInstanceTemplate_subnet_custom|TestAccComputeRegionInstanceTemplate_withScratchDisk|TestAccComputeInstanceTemplate_subnet_auto|TestAccComputeInstanceTemplate_regionDisks|TestAccComputeInstanceTemplate_NetworkAttachment|TestAccComputeInstanceTemplate_disks|TestAccComputeInstanceTemplate_with18TbScratchDisk

Get to know how VCR tests work

modular-magician · 2023-07-05T23:44:44Z

Tests passed during RECORDING mode:
TestAccComputeRegionAutoscaler_regionAutoscalerBasicExample[Debug log]
TestAccComputeInstanceFromTemplate_self_link_unique[Debug log]
TestAccComputeInstanceFromTemplate_basic[Debug log]
TestAccComputeInstanceTemplate_minCpuPlatform[Debug log]
TestAccComputeInstanceTemplate_guestAcceleratorSkip[Debug log]
TestAccComputeInstanceTemplate_guestAccelerator[Debug log]
TestAccComputeInstanceTemplate_regionDisks[Debug log]
TestAccComputeInstanceTemplate_disks[Debug log]
TestAccComputeInstanceTemplate_with18TbScratchDisk[Debug log]

Rerun these tests in REPLAYING mode to catch issues

All tests passed after rerunning REPLAYING mode.

Several tests got terminated during RECORDING mode.
View the build log or the debug log for each test

…gleCloudPlatform#8246) * Add and prefer AttachedDisk.DiskSizeGb over AttachedDisk.InitializeParams.DiskSizeGb * Fix a paren out of place * Add DiskSizeGb outside InitializeParams as a separate field. * Undo unnecessary changes * use one field instead of two * use one field instead of two

seanrmilligan added 5 commits June 29, 2023 11:25

Add and prefer AttachedDisk.DiskSizeGb over AttachedDisk.InitializePa…

2d33e38

…rams.DiskSizeGb

Fix a paren out of place

6a25c78

Add DiskSizeGb outside InitializeParams as a separate field.

476c358

Undo unnecessary changes

c163069

Merge branch 'main' of https://github.com/GoogleCloudPlatform/magic-m…

1da727f

…odules

modular-magician requested a review from hao-nan-li June 30, 2023 17:09

melinath requested review from melinath and removed request for hao-nan-li June 30, 2023 18:30

melinath reviewed Jun 30, 2023

View reviewed changes

modular-magician requested a review from melinath July 5, 2023 16:41

seanrmilligan added 2 commits July 5, 2023 12:03

Merge branch 'main' of https://github.com/GoogleCloudPlatform/magic-m…

d587ef8

…odules

use one field instead of two

abc41cb

seanrmilligan force-pushed the main branch from b74ffeb to abc41cb Compare July 5, 2023 19:27

use one field instead of two

ba92425

seanrmilligan changed the title ~~Add a field to compute instance templates to support changes in the GCP API~~ Prioritize disk.DiskSizeGb over disk.InitializeParams.DiskSizeGb Jul 5, 2023

seanrmilligan commented Jul 5, 2023

View reviewed changes

rileykarson removed the request for review from melinath July 5, 2023 20:13

rileykarson requested a review from trodge July 5, 2023 20:13

trodge approved these changes Jul 5, 2023

View reviewed changes

trodge merged commit cafdd60 into GoogleCloudPlatform:main Jul 5, 2023
6 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prioritize disk.DiskSizeGb over disk.InitializeParams.DiskSizeGb #8246

Prioritize disk.DiskSizeGb over disk.InitializeParams.DiskSizeGb #8246

seanrmilligan commented Jun 30, 2023 •

edited

Loading

modular-magician commented Jun 30, 2023

modular-magician commented Jun 30, 2023

seanrmilligan commented Jun 30, 2023

seanrmilligan commented Jun 30, 2023

modular-magician commented Jun 30, 2023

modular-magician commented Jun 30, 2023

melinath left a comment

modular-magician commented Jul 5, 2023

modular-magician commented Jul 5, 2023

modular-magician commented Jul 5, 2023

modular-magician commented Jul 5, 2023

modular-magician commented Jul 5, 2023

modular-magician commented Jul 5, 2023

seanrmilligan Jul 5, 2023

trodge Jul 5, 2023

modular-magician commented Jul 5, 2023

trodge Jul 5, 2023

modular-magician commented Jul 5, 2023

modular-magician commented Jul 5, 2023

Prioritize disk.DiskSizeGb over disk.InitializeParams.DiskSizeGb #8246

Prioritize disk.DiskSizeGb over disk.InitializeParams.DiskSizeGb #8246

Conversation

seanrmilligan commented Jun 30, 2023 • edited Loading

modular-magician commented Jun 30, 2023

modular-magician commented Jun 30, 2023

seanrmilligan commented Jun 30, 2023

seanrmilligan commented Jun 30, 2023

modular-magician commented Jun 30, 2023

Diff report

Missing test report

modular-magician commented Jun 30, 2023

melinath left a comment

Choose a reason for hiding this comment

modular-magician commented Jul 5, 2023

modular-magician commented Jul 5, 2023

Diff report

Missing test report

modular-magician commented Jul 5, 2023

modular-magician commented Jul 5, 2023

modular-magician commented Jul 5, 2023

Diff report

modular-magician commented Jul 5, 2023

seanrmilligan Jul 5, 2023

Choose a reason for hiding this comment

trodge Jul 5, 2023

Choose a reason for hiding this comment

modular-magician commented Jul 5, 2023

Diff report

trodge Jul 5, 2023

Choose a reason for hiding this comment

modular-magician commented Jul 5, 2023

Tests analytics

Action taken

modular-magician commented Jul 5, 2023

Rerun these tests in REPLAYING mode to catch issues

seanrmilligan commented Jun 30, 2023 •

edited

Loading