-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support behavioral changes to instance, instance template's max_run_duration feature on Beta #10683
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @SarahFrench, 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. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_instance" "primary" {
scheduling {
maintenance_interval = # value needed
}
}
Resource: resource "google_compute_instance_from_machine_image" "primary" {
scheduling {
instance_termination_action = # value needed
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
on_instance_stop_action {
discard_local_ssd = # value needed
}
preemptible = # value needed
provisioning_model = # value needed
}
}
Resource: resource "google_compute_instance_from_template" "primary" {
scheduling {
instance_termination_action = # value needed
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
on_instance_stop_action {
discard_local_ssd = # value needed
}
preemptible = # value needed
provisioning_model = # value needed
}
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeInstanceTemplate_spot_maxRunDuration_deleteTerminationAction|TestAccComputeInstanceTemplate_spot_maxRunDuration_stopTerminationAction|TestAccComputeInstance_localSsdVM_maxRunDuration_stopTerminationAction|TestAccComputeInstance_maxRunDuration_update|TestAccComputeInstance_spotVM_maxRunDuration_deleteTerminationAction|TestAccComputeInstance_standardVM_maxRunDuration_deleteTerminationAction|TestAccComputeInstance_standardVM_maxRunDuration_stopTerminationAction |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR - the new tests look good and are passing but I want to make sure this message about missing test coverage is acknowledged.
That message shows that the new on_instance_stop_action
field is also being added to the google_compute_instance_from_machine_image
resource. This is because schemas are re-used in the provider (see here for google_compute_instance_from_machine_image).
Firstly, could you please update the documentation for google_compute_instance_from_machine_image to include the new field?
And then, could you please add a basic acceptance test that tests the new field/behaviour you've added to google_compute_instance_from_machine_image
?
I'm happy for us to ignore the message's warnings about lack of test coverage on fields that your PR isn't adding.
…late, instance_template
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_instance" "primary" {
scheduling {
maintenance_interval = # value needed
}
}
Resource: resource "google_compute_instance_from_machine_image" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
Resource: resource "google_compute_instance_from_template" "primary" {
scheduling {
instance_termination_action = # value needed
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
on_instance_stop_action {
discard_local_ssd = # value needed
}
preemptible = # value needed
provisioning_model = # value needed
}
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
Non-exercised testsTests were added that are skipped in VCR:
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_instance" "primary" {
scheduling {
maintenance_interval = # value needed
}
}
Resource: resource "google_compute_instance_from_machine_image" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
Resource: resource "google_compute_instance_from_template" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
Non-exercised testsTests were added that are skipped in VCR:
|
/gcbrun |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem reported here is a panic that occurs when running the TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy/access_config_update_access_config
acceptance test:
panic: interface conversion: interface {} is []interface {}, not map[string]interface {}
goroutine 68179 [running]:
github.com/hashicorp/terraform-provider-google-beta/google-beta/services/compute.hasMaxRunDurationChanged(...)
/go/src/github.com/modular-magician/terraform-provider-google-beta/google-beta/services/compute/compute_instance_helpers.go:705
github.com/hashicorp/terraform-provider-google-beta/google-beta/services/compute.schedulingHasChangeRequiringReboot(0xc001302ca8?)
/go/src/github.com/modular-magician/terraform-provider-google-beta/google-beta/services/compute/compute_instance_helpers.go:658 +0x3ce
github.com/hashicorp/terraform-provider-google-beta/google-beta/services/compute.resourceComputeInstanceUpdate(0xc001f3f680, {0x4613dc0?, 0xc000161000?})
/go/src/github.com/modular-magician/terraform-provider-google-beta/google-beta/services/compute/resource_compute_instance.go:1890 +0x1437
The line compute_instance_helpers.go:705 in the generated provider is the beginning of hasMaxRunDurationChanged function where empty interfaces are converted to maps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the panic described in #10683 (review)
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_instance" "primary" {
scheduling {
maintenance_interval = # value needed
}
}
Resource: resource "google_compute_instance_from_machine_image" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
Resource: resource "google_compute_instance_from_template" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
Non-exercised testsTests were added that are skipped in VCR:
|
Thanks Sarah for pointing this out. I was not able to reproduce the issue locally by running the test you pointed out. It is somehow failing on its own due to an unrelated issue. But, i have pushed a change to fix this. I will take another look if the panic shows up again. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_instance" "primary" {
scheduling {
maintenance_interval = # value needed
}
}
Resource: resource "google_compute_instance_from_machine_image" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
Resource: resource "google_compute_instance_from_template" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
Non-exercised testsTests were added that are skipped in VCR:
|
This PR has been waiting for review for 2 days. Please take a look! Use the label |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_instance" "primary" {
scheduling {
maintenance_interval = # value needed
}
}
Resource: resource "google_compute_instance_from_machine_image" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
Resource: resource "google_compute_instance_from_template" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - the panic is addressed and there is test coverage for all the new fields added by this PR.
Spotted a code change in GA when this PR is meant to be beta-only changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approved but then from looking at your release note I remembered that this PR is meant to be causing changes in the beta provider only. I looked at the diffs that this PR will create in the GA provider and noticed that there are non-trivial changes being introduced by this PR in the GA provider. We want to get to a state where this PR only causes changes to documentation in the GA provider.
I've added a few suggestions in my review, but could you please also ensure that the changes to the _test.go files are also made only in the Beta provider? To achieve this you will need to convert any affected .go
files into .go.erb
files, as that'll allow the code to be templated in a conditional way based on the provider that's being generated.
When converting a .go
file to a .go.erb
file you just need to add the new extension to the filename and then add <% autogen_exception -%>
as the first line of the file (example here).
mmv1/third_party/terraform/services/compute/compute_instance_helpers.go.erb
Outdated
Show resolved
Hide resolved
@@ -656,7 +692,7 @@ func schedulingHasChangeRequiringReboot(d *schema.ResourceData) bool { | |||
oScheduling := o.([]interface{})[0].(map[string]interface{}) | |||
newScheduling := n.([]interface{})[0].(map[string]interface{}) | |||
|
|||
return hasNodeAffinitiesChanged(oScheduling, newScheduling) | |||
return hasNodeAffinitiesChanged(oScheduling, newScheduling) || hasMaxRunDurationChanged(oScheduling, newScheduling) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suggested change should mean that the changes only appear in the Beta provider
return hasNodeAffinitiesChanged(oScheduling, newScheduling) || hasMaxRunDurationChanged(oScheduling, newScheduling) | |
<% unless version == 'ga' -%> | |
return hasNodeAffinitiesChanged(oScheduling, newScheduling) || hasMaxRunDurationChanged(oScheduling, newScheduling) | |
<% else -%> | |
return hasNodeAffinitiesChanged(oScheduling, newScheduling) | |
<% end -%> |
mmv1/third_party/terraform/services/compute/resource_compute_instance.go.erb
Show resolved
Hide resolved
…compute_instance_helpers.go.erb Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>
…e/resource_compute_instance.go.erb Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_instance" "primary" {
scheduling {
maintenance_interval = # value needed
}
}
Resource: resource "google_compute_instance_from_machine_image" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
Resource: resource "google_compute_instance_from_template" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
Errors
|
1 similar comment
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_instance" "primary" {
scheduling {
maintenance_interval = # value needed
}
}
Resource: resource "google_compute_instance_from_machine_image" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
Resource: resource "google_compute_instance_from_template" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
Errors
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_instance" "primary" {
scheduling {
maintenance_interval = # value needed
}
}
Resource: resource "google_compute_instance_from_machine_image" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
Resource: resource "google_compute_instance_from_template" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
Errors
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_instance" "primary" {
scheduling {
maintenance_interval = # value needed
}
}
Resource: resource "google_compute_instance_from_machine_image" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
Resource: resource "google_compute_instance_from_template" "primary" {
scheduling {
maintenance_interval = # value needed
min_node_cpus = # value needed
on_host_maintenance = # value needed
preemptible = # value needed
}
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
Thanks Sarah for catching this. I have made changes as you suggested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the GA/Beta issue! I can see that the diffs for the GA provider still show changes in functions like testAccComputeInstanceTemplate_spot_maxRunDuration
, however those functions are unused in the GA code base so the code change isn't user-facing. Because Go tolerates unused functions and those functions were already present in the GA code before this PR, I'm happy to merge this PR
…uration feature on Beta (GoogleCloudPlatform#10683) Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>
…uration feature on Beta (GoogleCloudPlatform#10683) Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>
…uration feature on Beta (GoogleCloudPlatform#10683) Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>
Max run duration(MRD) feature support in terraform was added in #6812. There are behavioral changes in the feature which needs further changes.
See SchedulingOnInstanceStopAction section in https://pkg.go.dev/google.golang.org/api/compute/v0.beta.
Fix for hashicorp/terraform-provider-google#13005
Closes hashicorp/terraform-provider-google#13671
Release Note Template for Downstream PRs (will be copied)