-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add a new resource CmekConfig. #13419
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
base: main
Are you sure you want to change the base?
Add a new resource CmekConfig. #13419
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. Googlers: see go/terraform-auto-test-runs to set up automatic test runs. @shuyama1, 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_discovery_engine_cmek_config" "primary" {
kms_key = # value needed
kms_key_version = # value needed
location = # value needed
single_region_keys {
kms_key = # value needed
}
}
|
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_discovery_engine_cmek_config" "primary" {
kms_key = # value needed
kms_key_version = # value needed
location = # value needed
single_region_keys {
kms_key = # value needed
}
}
|
Tests analyticsTotal tests: 14 Click here to see the affected service packages
Action takenFound 14 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Tests analyticsTotal tests: 14 Click here to see the affected service packages
Action takenFound 14 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Hi reviewer, our resource
|
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_discovery_engine_cmek_config" "primary" {
kms_key = # value needed
kms_key_version = # value needed
location = # value needed
single_region_keys {
kms_key = # value needed
}
}
|
Tests analyticsTotal tests: 14 Click here to see the affected service packages
🟢 All tests passed! View the build log |
@shuyama1 This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
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 reviewer, our resource
CmekConfig
only has Update/Get/List/Delete methods but doesn't have a Create method. I have 2 questions about the implementation. Could you help take a look? Thanks!
- Per googlecloudplatform.github.io/magic-modules/develop/add-resource#add-a-resource-1, can we add a new resource leaving field
create_url
unset and only support update/get/list/delete operations?
Is this a pre-created resource that only allows update. I think you can set the create_url
and create_verb
to be the one used to update this resource. So basically creating the resource in Terraform is actually updating the resource.
Here's an example: https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/products/kms/AutokeyConfig.yaml
- Per googlecloudplatform.github.io/magic-modules/test/test#add-an-update-test, I tried to add an update test, but an update test is create-then-update. How to add an update test without create operation?
I believe after you've done question 1, this should be working fine.
I hope these answer your questions.
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_discovery_engine_cmek_config" "primary" {
kms_key = # value needed
kms_key_version = # value needed
location = # value needed
single_region_keys {
kms_key = # value needed
}
}
|
Thanks for your answers! I'm adding the create test now, but getting blocked by an ESF routing issue b/359238429#comment15. Temporarily waiting until the issue is fixed. |
Tests analyticsTotal tests: 15 Click here to see the affected service packages
🟢 All tests passed! View the build log |
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.
Would you mind adding tests for this resource following https://googlecloudplatform.github.io/magic-modules/test/test/#add-resource-tests?
Sorry if you're still working on this PR. No rush! I'm just sending out this review message to disable the review-reminder for this round. Thanks!
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_discovery_engine_cmek_config" "primary" {
kms_key = # value needed
kms_key_version = # value needed
location = # value needed
single_region_keys {
kms_key = # value needed
}
}
|
Tests analyticsTotal tests: 15 Click here to see the affected service packages
🟢 All tests passed! View the build log |
@jialei-chen I'm moving this PR to draft as it's awaiting test coverage (assuming currently blocked by some internal issues). Setting it to draft disables the review-reminder for now to avoid noise. |
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Error: Test case Debugging: Since we don't have CreateCmekConfig API, and use UpdateCmekConfig API for both create and update operation. Field "kms_key_name" is required and immutable. When updating with Solution: Per our API definition proto, the HTTP verb for update should be PATCH. But due to our API limitation (using UpdateCmekConfig API for both create and update operation), I think I should set |
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.
|
Tests analyticsTotal tests: 19 Click here to see the affected service packages
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
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
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.
|
Tests analyticsTotal tests: 19 Click here to see the affected service packages
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
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Error: I checked the latest [Error message] and [Debug log], the returned error is "Precondition check failed." Debugging: It's not informative, but I feel the possible root cause should be "Attempted to create an Astro corpus 'xxxx' that is currently being deleted. This is likely due to a race condition between create and delete pipelines. Code: FAILED_PRECONDITION". CmekConfig related operations are expensive. Deleting a CmekConfig (an operation from previous VCR test) takes long since its dependency has to be deleted first. Solution: Wait until the previous delete operation is done. Then re-trigger the VCR test again to see if it works. |
Thanks for the explanation. Assume the previous deletion should be done, triggering the VCR tests now and starting a new round of review. Thanks! |
/gcbrun to trigger tests |
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.
|
Tests analyticsTotal tests: 19 Click here to see the affected service packages
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
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
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.
|
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.
|
Tests analyticsTotal tests: 19 Click here to see the affected service packages
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
|
1 similar comment
Tests analyticsTotal tests: 19 Click here to see the affected service packages
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
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Error: Checked the lasted VCR test [Error message] and [Debug log], the error is Debugging: I don't know why kmsKey is required.
Hi @shuyama1, do you have any idea of the reported error? |
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: Checked the lasted VCR test [Error message] and [Debug log], the error is
"Field \"kms_key_name\" is a required field, but no value is found."
Debugging: I don't know why kmsKey is required.
- Field
kmsKey
is not marked withrequired: true
.- In the update test, I set
kms_key = "%{kms_key_name}"
.Hi @shuyama1, do you have any idea of the reported error?
I think I see where the issue is coming from. During the update test step, since kms_key
itself is not modified, it does not include in the request body. Therefore, the PATCH call failed with "Field \"kms_key_name\" is a required field, but no value is found."
error. (See the second PATCH call in the debug log)
You can add a custom update_encoder to always send this field even if it's not modified. (example)
Thank you! I added update_encoder as suggested. I tested locally and it worked. There are 2 checks "contributor-membership-checker" and "downstream-generation-and-test" in pending status for a long time. Except for these 2, others passed successfully, including the unit tests. https://screenshot.googleplex.com/7K27iuNotpTSzwb |
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.