-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Shared reservation #5547
Shared reservation #5547
Conversation
Merge branch 'sharedReservation' of https://github.com/simamGit/magic-modules into sharedReservation
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. @melinath, please review this PR or find an appropriate assignee. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 4 files changed, 297 insertions(+), 40 deletions(-)) |
/gcbrun |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 257 insertions(+)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccApigeeEnvironmentIamMemberGenerated|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeInstance_reservationAffinities|TestAccComputeReservation_sharedReservationBasicExample|TestAccComputeReservation_reservationBasicExample|TestAccComputeReservation_update|TestAccContainerNodePool_withInvalidUpgradeSettings You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=232210 |
@melinath Could you also add me as a reviewer for this pull request? (I'm new to this process, so please feel free to let me know if this is not needed). |
@paul-hoang I can't add you as a reviewer, but I think you may be able to provide a review regardless. |
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.
It looks like TestAccComputeReservation_sharedReservationBasicExample
is failing with the following error:
Error: Error creating Reservation: googleapi: Error 412: Constraint constraints/compute.sharedReservationsOwnerProjects violated for project projects/<project-number>
Shared reservation can not be created in this project for your organization., conditionNotMet
Please ensure that this test passes locally & then re-request review.
ran reservation tests successfully before PR. I can locally run both reservation and sharedreservation tests successfully. using these two commands: TF_LOG=DEBUG make testacc TEST=./google TESTARGS='-run=TestAccComputeReservation_reservationBasicExample' but it is also set the environment variables for these tests. specially project play key role for these tests. I set these three EVN variables: GOOGLE_APPLICATION_CREDENTIALS, GOOGLE_PROJECT , GOOGLE_REGION so the question here is which environment these tests are running and how can i set them on test time? |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 291 insertions(+), 31 deletions(-)) |
/gcbrun |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 4 files changed, 260 insertions(+)) |
Hi Stephen, to create project and set org policy the user needs to have these roles: Organization Administrator, Organization Policy Administrator and Project Creator. also we need to set these environment variables It is passing successfully in my local machine but all these new changes are for Terraform environment. hopefully this time it can pass in Terrafom Environment. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 285 insertions(+), 18 deletions(-)) |
/gcbrun |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 4 files changed, 267 insertions(+)) |
Tests were added that did not run in TeamCity:
|
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccApigeeEnvironmentIamBindingGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccCloudFunctionsFunction_vpcConnector|TestAccComposerEnvironment_composerV1MasterAuthNetworksUpdate|TestAccComposerEnvironment_composerV2MasterAuthNetworksUpdate|TestAccComputeInstance_reservationAffinities|TestAccComputeReservation_update|TestAccContainerNodePool_withInvalidUpgradeSettings You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=235482 |
I'm running the new test manually in Beta since it skips vcr: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaMmUpstream/235485 |
Thanks, it seems the main tests sharedReservation and reservation are passing ( i do not see them in failing list). most of the tests which are failing does not look related. I am going to check the TestAccComputeInstance_reservationAffinities, TestAccComputeReservation_update to see if their failing is due to my recent changes. |
Thanks for pointing these out as potential issues; I've started a new run in Teamcity for just these tests as well: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaMmUpstream/235487 |
I ran locally the reservation related tests: |
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.
@simamGit +1 it looks like TestAccComputeReservation_
is passing in CI as well. However, TestAccComputeInstance_reservationAffinities
seems to be failing with a permadiff:
Terraform will perform the following actions:
# google_compute_reservation.reservation must be replaced
-/+ resource "google_compute_reservation" "reservation" {
+ commitment = (known after apply)
~ creation_timestamp = "2021-12-23T11:32:06.647-08:00" -> (known after apply)
~ id = "projects/project-id/zones/us-central1-a/reservations/tf-test-resaffinity-et2yep0tb1" -> (known after apply)
name = "tf-test-resaffinity-et2yep0tb1"
~ project = "project-id" -> (known after apply)
~ self_link = "https://www.googleapis.com/compute/v1/projects/project-id/zones/us-central1-a/reservations/tf-test-resaffinity-et2yep0tb1" -> (known after apply)
specific_reservation_required = true
~ status = "READY" -> (known after apply)
~ zone = "https://www.googleapis.com/compute/v1/projects/project-id/zones/us-central1-a" -> "us-central1-a"
- share_settings {
- share_type = "LOCAL" -> null
}
~ specific_reservation {
count = 1
~ in_use_count = 0 -> (known after apply)
~ instance_properties {
machine_type = "n1-standard-1"
+ min_cpu_platform = (known after apply)
}
}
}
The relevant part is:
- share_settings {
- share_type = "LOCAL" -> null
}
This means that the API returned a value for this field that Terraform didn't expect, so Terraform is trying to set it to null
. I believe you can work around this by adding default_from_api
to shareSettings and shareType in terraform.yaml - that will make Terraform accept whatever the API returns.
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 262 insertions(+), 4 deletions(-)) |
It looks like there's an issue on the API side; please re-request review once that's been resolved. I think everything here should be ready to go once that's finished. https://google.aip.dev/cloud/2510 |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 8 files changed, 281 insertions(+), 22 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 221 insertions(+), 5 deletions(-)) |
Beta test run for just |
/gcbrun |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 8 files changed, 243 insertions(+), 27 deletions(-)) |
Per discussion, we're moving forward with ignore_read rather than fixing the API issue. ignore_read doesn't seem to work as expected inside the map object, so it has to be set to ignore reads on all of share settings. |
Trying a beta run again - the last one failed for unrelated reasons: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaGoogleProject/242816 |
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 LGTM as long as the tests pass.
LGTM. |
Tests were added that did not run in TeamCity:
|
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccApigeeEnvironmentIamBindingGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccComputeReservation_reservationBasicExample|TestAccComputeReservation_update|TestAccContainerCluster_withDNSConfig|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccPrivatecaCertificate_privatecaCertificateConfigExample|TestAccPrivatecaCertificate_privatecaCertificateNoAuthorityExample You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=242828 |
new beta test run for I was running the other one in the wrong place which is why it was failing 🤦 |
* Adding shareSetting to create SherdReservations. * Add ShareSetting to create sharedReservation. * fixing the update test * fixing sharedReservation test * adding shareType local to instance test. * fixing shareType issue. * fixing shareType issue. * fix ShareType issue. * fix shareType issue. * unde recent changes. * provide digit validation for project_number. * set ignore_read=true for shareSettings to enable project_id.
* Adding shareSetting to create SherdReservations. * Add ShareSetting to create sharedReservation. * fixing the update test * fixing sharedReservation test * adding shareType local to instance test. * fixing shareType issue. * fixing shareType issue. * fix ShareType issue. * fix shareType issue. * unde recent changes. * provide digit validation for project_number. * set ignore_read=true for shareSettings to enable project_id.
* Adding shareSetting to create SherdReservations. * Add ShareSetting to create sharedReservation. * fixing the update test * fixing sharedReservation test * adding shareType local to instance test. * fixing shareType issue. * fixing shareType issue. * fix ShareType issue. * fix shareType issue. * unde recent changes. * provide digit validation for project_number. * set ignore_read=true for shareSettings to enable project_id.
* Adding shareSetting to create SherdReservations. * Add ShareSetting to create sharedReservation. * fixing the update test * fixing sharedReservation test * adding shareType local to instance test. * fixing shareType issue. * fixing shareType issue. * fix ShareType issue. * fix shareType issue. * unde recent changes. * provide digit validation for project_number. * set ignore_read=true for shareSettings to enable project_id.
Added shareSetting to enable SharedReservation creation for compute.reservation.
ShareSetting includes two properties:
-shareType: LOCAL and SPECIFIC_PROJECTS
-project_map: a map of projectId and projectConfig.
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)