Skip to content
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

Add GA support for confidentialInstanceType #10887

Merged
merged 2 commits into from
Jun 25, 2024

Conversation

deeglaze
Copy link
Contributor

@deeglaze deeglaze commented Jun 5, 2024

Finalizes hashicorp/terraform-provider-google#14768
Fixes b/345255719

compute: added `confidential_instance_config.confidential_instance_type` field to `google_compute_instance`,  `google_compute_instance_template`, and `google_compute_region_instance_template` resources

@deeglaze
Copy link
Contributor Author

deeglaze commented Jun 5, 2024

/cc @NickElliot

Copy link

github-actions bot commented Jun 5, 2024

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.

@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 ( 10 files changed, 356 insertions(+), 17 deletions(-))
google-beta provider: Diff ( 3 files changed, 3 insertions(+), 3 deletions(-))

Errors

google provider:

  • The diff processor failed to build. This is usually due to the downstream provider failing to compile.

Copy link

github-actions bot commented Jun 7, 2024

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

@deeglaze deeglaze marked this pull request as draft June 7, 2024 17:54
@deeglaze
Copy link
Contributor Author

deeglaze commented Jun 7, 2024

Changed to draft since the GA json change hasn't propagated to the Golang derived API package yet.

@c2thorn
Copy link
Member

c2thorn commented Jun 10, 2024

Changed to draft since the GA json change hasn't propagated to the Golang derived API package yet.

please ping me when this is ready for review

@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 ( 10 files changed, 356 insertions(+), 17 deletions(-))
google-beta provider: Diff ( 3 files changed, 3 insertions(+), 3 deletions(-))

Errors

google provider:

  • The diff processor failed to build. This is usually due to the downstream provider failing to compile.

@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 ( 12 files changed, 359 insertions(+), 20 deletions(-))
google-beta provider: Diff ( 5 files changed, 6 insertions(+), 6 deletions(-))

@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 ( 12 files changed, 402 insertions(+), 63 deletions(-))
google-beta provider: Diff ( 5 files changed, 49 insertions(+), 49 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 6871
Passed tests: 3435
Skipped tests: 376
Affected tests: 1

Click here to see the affected service packages

All service packages are affected

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

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 ( 12 files changed, 402 insertions(+), 63 deletions(-))
google-beta provider: Diff ( 5 files changed, 49 insertions(+), 49 deletions(-))

@modular-magician
Copy link
Collaborator

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy[Error message] [Debug log]

$\textcolor{red}{\textsf{Errors occurred during RECORDING mode. Please fix them to complete your PR.}}$

View the build log or the debug log for each test

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 6871
Passed tests: 3435
Skipped tests: 376
Affected tests: 1

Click here to see the affected service packages

All service packages are affected

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

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy[Error message] [Debug log]

$\textcolor{red}{\textsf{Errors occurred during RECORDING mode. Please fix them to complete your PR.}}$

View the build log or the debug log for each test

@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 ( 12 files changed, 402 insertions(+), 63 deletions(-))
google-beta provider: Diff ( 5 files changed, 49 insertions(+), 49 deletions(-))

@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 ( 12 files changed, 403 insertions(+), 63 deletions(-))
google-beta provider: Diff ( 5 files changed, 49 insertions(+), 49 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 6870
Passed tests: 3434
Skipped tests: 376
Affected tests: 2

Click here to see the affected service packages

All service packages are affected

Action taken

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

Get to know how VCR tests work

@deeglaze deeglaze marked this pull request as ready for review June 24, 2024 23:55
@deeglaze
Copy link
Contributor Author

@c2thorn the TestReadPlannedAssetsCoverage failure appears to be unrelated. The rest is good to go I think.

@modular-magician
Copy link
Collaborator

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccAppEngineFlexibleAppVersion_update[Error message] [Debug log]
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy[Error message] [Debug log]

$\textcolor{red}{\textsf{Errors occurred during RECORDING mode. Please fix them to complete your PR.}}$

View the build log or the debug log for each test

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 6870
Passed tests: 3434
Skipped tests: 376
Affected tests: 2

Click here to see the affected service packages

All service packages are affected

Action taken

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

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccAppEngineFlexibleAppVersion_update[Error message] [Debug log]
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy[Error message] [Debug log]

$\textcolor{red}{\textsf{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.

can you try rebasing/pulling this branch to after #11037. This should fix the unit test failure

@@ -28,18 +28,18 @@ require (
github.com/sirupsen/logrus v1.8.1
go4.org/netipx v0.0.0-20231129151722-fdeea329fbba
golang.org/x/exp v0.0.0-20240409090435-93d18d7e34b8
golang.org/x/net v0.25.0
golang.org/x/oauth2 v0.20.0
google.golang.org/api v0.180.0
Copy link
Member

Choose a reason for hiding this comment

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

is this upgrade needed to support the field in GA?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is. The ConfidentialInstanceType field was added to the GA API very recently.

@@ -1025,13 +1025,6 @@ be from 0 to 999,999,999 inclusive.`,
Description: `The Confidential VM config being used by the instance. on_host_maintenance has to be set to TERMINATE or this will fail to create.`,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
<% if version == "ga" -%>
"enable_confidential_compute": {
Copy link
Member

Choose a reason for hiding this comment

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

even if this field has been made obsolete, removing fields are a breaking change as users may be using it in TF schema. You can deprecate the field and follow up with removal by following https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/make-a-breaking-change/#add-deprecations-and-warnings-to-the-main-branch-of-magic-modules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not removing it. The else condition is just becoming active. Is the Required: True what needs to remain until a major release can fully deprecate it? I've added a deprecation message in another commit at least.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, I misread the block. Got it!
No, it is not a breaking change to remove a requirement (only the reverse)

@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 ( 12 files changed, 403 insertions(+), 63 deletions(-))
google-beta provider: Diff ( 5 files changed, 49 insertions(+), 49 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3815
Passed tests: 3436
Skipped tests: 376
Affected tests: 3

Click here to see the affected service packages

All service packages are affected

Action taken

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

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccBigQueryRoutine_bigQueryRoutineRemoteFunction[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccAppEngineFlexibleAppVersion_update[Error message] [Debug log]
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy[Error message] [Debug log]

$\textcolor{red}{\textsf{Errors occurred during RECORDING mode. Please fix them to complete your PR.}}$

View the build log or the debug log for each test

@c2thorn
Copy link
Member

c2thorn commented Jun 25, 2024

This is a very version-specific change, so I'm going to run tests in our separate GA environment as well just to be sure. If everything's good I'll come back and merge

@deeglaze
Copy link
Contributor Author

Thanks @c2thorn!

@c2thorn
Copy link
Member

c2thorn commented Jun 25, 2024

Tests look good, failures were not relevant. LGTM

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.

3 participants