-
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
KMS_config_first_commit #9353
KMS_config_first_commit #9353
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Hello! I am a robot. It looks like you are a: Community Contributor @NickElliot, 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. |
@NickElliot - Just signed the CLA |
/gcbrun |
The CLA check is still failing, which appears to be because the email associated with your commits is not the email associated with your CLA |
@NickElliot - Have fixed the CLA issue |
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.
Hi, the build is failing with the following:
Please use type KeyValueLabels for field labels in resource Netapp/kmsconfig (RuntimeError)
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. Terraform GA: Diff ( 13 files changed, 1156 insertions(+), 2 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_netapp_kmsconfig" "primary" {
labels = # value needed
}
|
Tests analyticsTotal tests: Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccNetappkmsconfig_kmsConfigCreateExample_Update|TestAccNetappkmsconfig_kmsConfigCreateExample |
|
@NickElliot - Can you please the test error logs for us to take a look at it? |
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.
Sorry about the delay, one error explanation and one needed change :)
kms_name = "<%= ctx[:vars]['kms_name'] %>" | ||
description="this is a test description" | ||
crypto_key_name="projects/cxo-automation-gcp/locations/us-central1/keyRings/kmsRing2/cryptoKeys/kmsCrypto111" | ||
location="usthis is a test description-central1" |
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.
Error: Error creating kmsconfig: googleapi: Error 403: Permission denied on 'locations/usthis is a test description-central1' (or it may not exist).
with google_netapp_kmsconfig.kmsConfig,
on terraform_plugin_test.tf line 2, in resource "google_netapp_kmsconfig" "kmsConfig":
2: resource "google_netapp_kmsconfig" "kmsConfig" {
appears to be a problem here! also could you update this sample to be using a cryptokey terraform resource for the crypto_key_name
resource "google_netapp_kmsconfig" "kmsConfig" { | ||
kms_name = "tf-test-kms-test%{random_suffix}" | ||
description="" | ||
crypto_key_name="projects/cxo-automation-gcp/locations/us-central1/keyRings/kmsRing2/cryptoKeys/kmsCrypto111" |
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.
same thing here! need a crypto key resource in config
@NickElliot I have updated the crypto_key_name and fixed the above 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. Terraform GA: Diff ( 13 files changed, 1200 insertions(+), 2 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_netapp_kmsconfig" "primary" {
labels = # value needed
}
|
Tests analyticsTotal tests: Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccNetappkmsconfig_kmsConfigCreateExample_Update|TestAccNetappkmsconfig_kmsConfigCreateExample |
|
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. Terraform GA: Diff ( 6 files changed, 1047 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccNetappkmsconfig_kmsConfigCreateExample_Update |
|
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.
Still experiencing the same error - I suggested removing the parallelization of the test so it should not happen until the other test is fully finished, and moving the tests back to us-central1. Sorry for the extremely prolonged review.
) | ||
|
||
func TestAccNetappkmsconfig_kmsConfigCreateExample_Update(t *testing.T) { | ||
t.Parallel() |
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.
t.Parallel() |
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.
Woot! This is a missing link I was looking for in the last weeks. We have multiple resources (Active Directory, KSMconfig, BackupVault) which are unique per region and tests are failing when ran in parallel. We worked around that so far, but this is the best way to solve that. I learned something important. Thank you!
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.
But I am still surprised the tests did fail. We ran them in two different regions, so they shouldn't impact each other. Let us incorporate your suggestions and see how it works. My local test works fine.
return acctest.Nprintf(` | ||
resource "google_kms_key_ring" "keyring" { | ||
name = "tf-test-key-ring%{random_suffix}" | ||
location = "us-west1" |
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.
location = "us-west1" | |
location = "us-central1" |
return acctest.Nprintf(` | ||
resource "google_kms_key_ring" "keyring" { | ||
name = "tf-test-key-ring%{random_suffix}" | ||
location = "us-west1" |
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.
location = "us-west1" | |
location = "us-central1" |
name = "tf-test-kms-test%{random_suffix}" | ||
description="this is a test description" | ||
crypto_key_name=google_kms_crypto_key.crypto_key.id | ||
location="us-west1" |
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.
location="us-west1" | |
location = "us-central1" |
name = "tf-test-kms-test%{random_suffix}" | ||
description="kmsconfig update" | ||
crypto_key_name=google_kms_crypto_key.crypto_key.id | ||
location="us-west1" |
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.
location="us-west1" | |
location = "us-central1" |
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. Terraform GA: Diff ( 6 files changed, 1047 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccNetappkmsconfig_kmsConfigCreateExample_Update |
Rerun these tests in REPLAYING mode to catch issues
|
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.
At last everything is good! Thank you for your contribution.
Thank you for your review. Very appreciated! |
* KMS_config_first_commit * kmscommit for author change * Updated label to KeyValueLabels * kmsconfig_update_key_ring_crypto_key_name * Fixed issues in resource_netapp_kmsconfig_test.go * removed roation_period * changed location to us-west1 * fixed double quotes * changes_with_labels * changed location to us-central1
* KMS_config_first_commit * kmscommit for author change * Updated label to KeyValueLabels * kmsconfig_update_key_ring_crypto_key_name * Fixed issues in resource_netapp_kmsconfig_test.go * removed roation_period * changed location to us-west1 * fixed double quotes * changes_with_labels * changed location to us-central1
…s in state by config order
* KMS_config_first_commit * kmscommit for author change * Updated label to KeyValueLabels * kmsconfig_update_key_ring_crypto_key_name * Fixed issues in resource_netapp_kmsconfig_test.go * removed roation_period * changed location to us-west1 * fixed double quotes * changes_with_labels * changed location to us-central1
* KMS_config_first_commit * kmscommit for author change * Updated label to KeyValueLabels * kmsconfig_update_key_ring_crypto_key_name * Fixed issues in resource_netapp_kmsconfig_test.go * removed roation_period * changed location to us-west1 * fixed double quotes * changes_with_labels * changed location to us-central1
…g order (#10572) Co-authored-by: Luca Prete <lucaprete@google.com> Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>
…s in state by config order (GoogleCloudPlatform#10572) Co-authored-by: Luca Prete <lucaprete@google.com> Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>
…s in state by config order (GoogleCloudPlatform#10572) Co-authored-by: Luca Prete <lucaprete@google.com> Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>
…s in state by config order (GoogleCloudPlatform#10572) Co-authored-by: Luca Prete <lucaprete@google.com> Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>
Fixes - hashicorp/terraform-provider-google#16355
This PR is to support provisioning kmsConfigs for Google Cloud NetApp Volumes through GCP terraform provider.