Skip to content

compute: Implement graceful switch for metadata_startup_script #12360

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

Merged

Conversation

Asiderr
Copy link
Contributor

@Asiderr Asiderr commented Nov 19, 2024

This patch checks whether a graceful switch (without ForceNew) is available between metadata_startup_script and metadata.startup-script. Graceful switch can be executed in two situations:

  1. When metadata_startup_script is created with the old value of metadata.startup-script.
  2. When metadata_startup_script is deleted and the old value remains in metadata.startup-script.

For all other changes in metadata_startup_script, isGracefulMetadataStartupSwitch sets ForceNew.

The change is covered by:
TestAccComputeInstance_metadataStartupScript_update and TestAccComputeInstance_metadataStartupScript_gracefulSwitch

closes: hashicorp/terraform-provider-google#9459

compute: made `metadata_startup_script` able to be updated via graceful switch in `google_compute_instance`

Copy link

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

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

@github-actions github-actions bot requested a review from c2thorn November 19, 2024 13:26
@Asiderr Asiderr force-pushed the metadata_startup_graceful_switch branch 2 times, most recently from 09f36dc to a98d2f0 Compare November 19, 2024 13:45
@modular-magician
Copy link
Collaborator

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

Missing test report

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

Resource: google_compute_instance_from_machine_image (12 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_compute_instance_from_machine_image" "primary" {
  metadata_startup_script = # value needed
}

Resource: google_compute_instance_from_template (20 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_compute_instance_from_template" "primary" {
  metadata_startup_script = # value needed
}

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 1064
Passed tests: 990
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
Copy link
Collaborator

🔴 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

Copy link
Member

@c2thorn c2thorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Asiderr
Did you test the Forcenew/regular update by hand?

Should we not get a new update test specifically for updating metadata_startup_script without Forcenew?

This patch checks whether a graceful switch (without ForceNew)
is available between `metadata_startup_script` and `metadata.startup-script`.
Graceful switch can be executed in two situations:
1. When `metadata_startup_script` is created with the old
value of `metadata.startup-script`.
2. When `metadata_startup_script` is deleted and the old value remains
in `metadata.startup-script`

For all other changes in `metadata_startup_script`, function sets ForceNew.

Also it adds TestAccComputeInstance_metadataStartupScript_gracefulSwitch
test that covers those changes.
Closes: hashicorp/terraform-provider-google#9459

Signed-off-by: Norbert Kamiński <[email protected]>
@Asiderr Asiderr force-pushed the metadata_startup_graceful_switch branch from a98d2f0 to 3eb8e86 Compare November 27, 2024 09:13
@github-actions github-actions bot requested a review from c2thorn November 27, 2024 09:14
@Asiderr
Copy link
Contributor Author

Asiderr commented Nov 27, 2024

Hi @c2thorn

Did you test the Forcenew/regular update by hand?

Yes, I tested all of the potential configurations and it works as expected.

Should we not get a new update test specifically for updating metadata_startup_script without Forcenew?

I've added test TestAccComputeInstance_metadataStartupScript_gracefulSwitch that covers updating metadata_startup_script without Forcenew

@modular-magician modular-magician added awaiting-approval Pull requests that need reviewer's approval to run presubmit tests and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Nov 27, 2024
@modular-magician
Copy link
Collaborator

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

Missing test report

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

Resource: google_compute_instance_from_machine_image (12 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_compute_instance_from_machine_image" "primary" {
  metadata_startup_script = # value needed
}

Resource: google_compute_instance_from_template (20 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_compute_instance_from_template" "primary" {
  metadata_startup_script = # value needed
}

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 1067
Passed tests: 993
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
  • TestAccComputeInstance_metadataStartupScript_gracefulSwitch

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

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

Copy link

github-actions bot commented Dec 2, 2024

@c2thorn This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

Copy link

github-actions bot commented Dec 4, 2024

@GoogleCloudPlatform/terraform-team @c2thorn This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

Copy link
Member

@c2thorn c2thorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks @Asiderr for the update test.
Logic looks good for the forcenew function and should be otherwise safe considering the previous logic was always ForceNew

@c2thorn c2thorn merged commit b2590d7 into GoogleCloudPlatform:main Dec 10, 2024
14 checks passed
amanMahendroo pushed a commit to amanMahendroo/magic-modules that referenced this pull request Dec 17, 2024
niharika-98 pushed a commit to niharika-98/magic-modules that referenced this pull request Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow switching from metadata_startup_script to metadata.startup-script gracefully
3 participants