Add `advanced_machine_features` to GCE Instance Templates #4850

upodroid · 2021-06-07T13:46:47Z

Part of hashicorp/terraform-provider-google#9251

Raised instance template separately as it is easier to work with. There is a bug in the instances update method.

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)

compute: added `advanced_machine_features` fields to `google_compute_instance_template`

Co-authored-by: upodroid <[email protected]>

modular-magician · 2021-06-07T13:48:06Z

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

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@ScottSuarez, please review this PR or find an appropriate assignee.

modular-magician · 2021-06-07T13:55:37Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 114 insertions(+))
Terraform Beta: Diff ( 5 files changed, 115 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 23 insertions(+))

modular-magician · 2021-06-07T13:55:49Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=190965"

modular-magician · 2021-06-07T14:05:33Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 118 insertions(+))
Terraform Beta: Diff ( 4 files changed, 118 insertions(+))
TF Conversion: Diff ( 1 file changed, 23 insertions(+))

modular-magician · 2021-06-07T14:05:53Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=190967"

ScottSuarez · 2021-06-10T23:06:17Z

getting a build failure in tpc build process

# github.com/GoogleCloudPlatform/terraform-google-conversion/google [github.com/GoogleCloudPlatform/terraform-google-conversion/google.test]
google/compute_instance_helpers.go:356:3: unknown field 'ThreadsPerCore' in struct literal of type "google.golang.org/api/compute/v0.beta".AdvancedMachineFeatures
google/compute_instance_helpers.go:367:58: AdvancedMachineFeatures.ThreadsPerCore undefined (type *"google.golang.org/api/compute/v0.beta".AdvancedMachineFeatures has no field or method ThreadsPerCore)
FAIL	github.com/GoogleCloudPlatform/terraform-google-conversion/google [build failed]
FAIL

upodroid · 2021-06-10T23:17:07Z

Maybe enable dependabot on that repo?

ScottSuarez · 2021-06-11T17:16:45Z

/gcbrun - updated the deps

modular-magician · 2021-06-11T17:16:59Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=191771"

modular-magician · 2021-06-11T17:26:20Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 118 insertions(+))
Terraform Beta: Diff ( 4 files changed, 118 insertions(+))
TF Conversion: Diff ( 1 file changed, 23 insertions(+))

modular-magician · 2021-06-11T17:26:41Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=191772"

modular-magician · 2021-06-11T18:50:46Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccInstanceTemplateDatasource_filter_mostRecent|TestAccComputeBackendService_withSecurityPolicy|TestAccComputeInstanceTemplate_AdvancedMachineFeatures|TestAccComputeSecurityPolicy_basic|TestAccComputeSecurityPolicy_withRule|TestAccComputeSecurityPolicy_withRuleExpr|TestAccComputeSecurityPolicy_update You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=191774"

ScottSuarez · 2021-06-11T21:15:16Z

/gcbrun

modular-magician · 2021-06-11T21:15:29Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=191795"

modular-magician · 2021-06-11T21:24:20Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 118 insertions(+))
Terraform Beta: Diff ( 4 files changed, 118 insertions(+))
TF Conversion: Diff ( 1 file changed, 23 insertions(+))

modular-magician · 2021-06-11T21:24:39Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=191798"

ScottSuarez · 2021-06-11T23:24:52Z

/gcbrun - fixed downstream images

modular-magician · 2021-06-11T23:25:06Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=191807"

modular-magician · 2021-06-11T23:34:39Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 118 insertions(+))
Terraform Beta: Diff ( 5 files changed, 119 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 23 insertions(+))

modular-magician · 2021-06-11T23:34:47Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=191809"

rileykarson · 2021-06-25T16:52:00Z

mmv1/third_party/terraform/resources/resource_compute_instance_template.go.erb

@@ -556,11 +556,38 @@ func resourceComputeInstanceTemplate() *schema.Resource {
 						"enable_confidential_compute": {
 							Type:        schema.TypeBool,
 							Required:    true,
+							ForceNew:    true,


@ScottSuarez @upodroid: Why was this made ForceNew? That's appears to be a breaking change at first glance, and I can't see any context in this PR on why it wouldn't be in this circumstance.

It was something I forgot when I first added CVM to this resource. The resource doesn't have an update method so it is safe to sneak this change through.

Ack, thanks. If you're making similar changes in the future can you make sure to specifically call that out when you send the review? Showing that modifying the field always returns an error is quick to collect and does all we need to justify the change.

One case that gives me pause as a reviewer is when Terraform doesn't return an error. We may want to avoid the change in those cases, even if it's correct. Unfortunately not all users run terraform plan before apply, particularly after provider updates, and depending on the circumstances we may want to not make the change until a major version.

I'll bear that in mind. There isn't a fix for end users running apply carelessly and not pinning provider versions.

rileykarson · 2021-06-25T18:33:23Z

mmv1/third_party/terraform/resources/resource_compute_instance_template.go.erb

+				Type:        schema.TypeList,
+				MaxItems:    1,
+				Optional:    true,
+				Computed:    true,


@upodroid: Just curious, why did you choose to make the value Computed: true? I haven't seen the API return a default value, does it have a default that changes based on other configuration between the resource? (sorry for the drive-by comments here, there's a conversation internally about the implementation of the field)

Yeah, values were computed in google_compute_instance so I copied them over. I did the bulk of the testing with google_compute_instance

Thanks! And GCE Instance will return the value when not sent in some circumstances?

Also see hashicorp/terraform-provider-google#9436 if you're interested in some of the context here, I'm seeing @karlkfi decided to comment around the same time I did 🙂

karlkfi · 2021-06-25T18:34:33Z

mmv1/third_party/terraform/resources/resource_compute_instance_template.go.erb

+				Type:        schema.TypeList,
+				MaxItems:    1,
+				Optional:    true,
+				Computed:    true,


Does this need to be Computed=true?

It seems like this would cause undesirable behavior.

Going from

advanced_machine_features { threads_per_core = 1 enable_nested_virtualization = true }

to

// removed

would NOT trigger an update/recreate. It would just be ignored, even tho the threads_per_core default value is 0.

upodroid added 2 commits June 7, 2021 14:14

add advance_machine_features

874cd17

Co-authored-by: upodroid <[email protected]>

add advanced_machine_features

f6d483d

Co-authored-by: upodroid <[email protected]>

google-cla bot added the cla: yes label Jun 7, 2021

modular-magician requested a review from ScottSuarez June 7, 2021 13:48

upodroid mentioned this pull request Jun 7, 2021

Add advanced_machine_features to GCE Instances #4849

Merged

5 tasks

fix schema typo

2c63fa3

ScottSuarez mentioned this pull request Jun 10, 2021

upgrade google sdk v GoogleCloudPlatform/terraform-google-conversion#720

Merged

ScottSuarez approved these changes Jun 14, 2021

View reviewed changes

ScottSuarez merged commit 3701a5a into GoogleCloudPlatform:master Jun 14, 2021

modular-magician mentioned this pull request Jun 14, 2021

Add advanced_machine_features to GCE Instance Templates GoogleCloudPlatform/terraform-google-conversion#721

Merged

5 tasks

This was referenced Jun 14, 2021

Add advanced_machine_features to GCE Instance Templates hashicorp/terraform-provider-google#9363

Merged

Add advanced_machine_features to GCE Instance Templates hashicorp/terraform-provider-google-beta#3337

Merged

rileykarson reviewed Jun 25, 2021

View reviewed changes

karlkfi reviewed Jun 25, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `advanced_machine_features` to GCE Instance Templates #4850

Add `advanced_machine_features` to GCE Instance Templates #4850

upodroid commented Jun 7, 2021

modular-magician commented Jun 7, 2021

modular-magician commented Jun 7, 2021

modular-magician commented Jun 7, 2021

modular-magician commented Jun 7, 2021

modular-magician commented Jun 7, 2021

ScottSuarez commented Jun 10, 2021

upodroid commented Jun 10, 2021

ScottSuarez commented Jun 11, 2021

modular-magician commented Jun 11, 2021

modular-magician commented Jun 11, 2021

modular-magician commented Jun 11, 2021

modular-magician commented Jun 11, 2021

ScottSuarez commented Jun 11, 2021

modular-magician commented Jun 11, 2021

modular-magician commented Jun 11, 2021

modular-magician commented Jun 11, 2021

ScottSuarez commented Jun 11, 2021

modular-magician commented Jun 11, 2021

modular-magician commented Jun 11, 2021

modular-magician commented Jun 11, 2021

rileykarson Jun 25, 2021

upodroid Jun 25, 2021 •

edited

Loading

rileykarson Jun 25, 2021

upodroid Jun 25, 2021

rileykarson Jun 25, 2021

upodroid Jun 25, 2021 •

edited

Loading

rileykarson Jun 25, 2021

rileykarson Jun 25, 2021

karlkfi Jun 25, 2021

Add advanced_machine_features to GCE Instance Templates #4850

Add advanced_machine_features to GCE Instance Templates #4850

Conversation

upodroid commented Jun 7, 2021

modular-magician commented Jun 7, 2021

modular-magician commented Jun 7, 2021

Diff report:

modular-magician commented Jun 7, 2021

modular-magician commented Jun 7, 2021

Diff report:

modular-magician commented Jun 7, 2021

ScottSuarez commented Jun 10, 2021

upodroid commented Jun 10, 2021

ScottSuarez commented Jun 11, 2021

modular-magician commented Jun 11, 2021

modular-magician commented Jun 11, 2021

Diff report:

modular-magician commented Jun 11, 2021

modular-magician commented Jun 11, 2021

ScottSuarez commented Jun 11, 2021

modular-magician commented Jun 11, 2021

modular-magician commented Jun 11, 2021

Diff report:

modular-magician commented Jun 11, 2021

ScottSuarez commented Jun 11, 2021

modular-magician commented Jun 11, 2021

modular-magician commented Jun 11, 2021

Diff report:

modular-magician commented Jun 11, 2021

rileykarson Jun 25, 2021

Choose a reason for hiding this comment

upodroid Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

rileykarson Jun 25, 2021

Choose a reason for hiding this comment

upodroid Jun 25, 2021

Choose a reason for hiding this comment

rileykarson Jun 25, 2021

Choose a reason for hiding this comment

upodroid Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

rileykarson Jun 25, 2021

Choose a reason for hiding this comment

rileykarson Jun 25, 2021

Choose a reason for hiding this comment

karlkfi Jun 25, 2021

Choose a reason for hiding this comment

Add `advanced_machine_features` to GCE Instance Templates #4850

Add `advanced_machine_features` to GCE Instance Templates #4850

upodroid Jun 25, 2021 •

edited

Loading

upodroid Jun 25, 2021 •

edited

Loading