-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feature: ephemeral write-only support #13135
base: main
Are you sure you want to change the base?
feature: ephemeral write-only support #13135
Conversation
…TURE-BRANCH-ephemeral-write-only
…oogleCloudPlatform#12800) Co-authored-by: Zhenhua Li <[email protected]>
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_bigquery_data_transfer_config" "primary" {
sensitive_params {
secret_access_key = # value needed
secret_access_key_wo = # value needed
}
sensitive_params_wo_version = # value needed
}
|
Tests analyticsTotal tests: 4609 Click here to see the affected service packages
🔴 Tests were added that are skipped in VCR:
Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
mmv1/third_party/terraform/services/sql/resource_sql_user_test.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Riley Karson <[email protected]>
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_bigquery_data_transfer_config" "primary" {
sensitive_params {
secret_access_key = # value needed
secret_access_key_wo = # value needed
}
sensitive_params_wo_version = # value needed
}
|
TeamCity Builds testing the resources with Write-Only attributes: |
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.
Going through failing tests for now, I'll make a full run through the code tomorrow. The BQ change and a docs change didn't go through earlier dedicated review, and not sure if a full review was the expectation on the feature branch merge PR.
Breakdown by test:
- TestAccDataSourceGoogleGkeHubFeature_basic seems unrelated. I'm not entirely sure what's going wrong, but it's VCR mechanics and doesn't seem to have been touched here.
- TestAccSpannerInstance_spannerInstanceWithAutoscaling has a non-deterministic source of randomisation. I'll fix that. edit: Use controlled random suffix for TestAccSpannerInstance_spannerInstanceWithAutoscaling #13141
- TestAccAccessContextManager is likely failing due to messiness/conflicts in the environment.
- TestAccCloudbuildWorkerPool_basic is failing in nightlies
- TestAccEphemeralServiceAccountKey_basic seems like it may be related
- TestAccSecretManagerSecretVersion_secretVersionBasicWriteOnlyExample can't pass yet in VCR but did in TC
- TestAccSecretManagerSecretVersion_secretVersionWithBase64StringSecretDataWriteOnlyExample can't pass yet in VCR but did in TC
- TestAccSqlUser_password_wo was added but won't pass yet in VCR but did in TC
@BBBmau can you link successful runs for the WO tests, and investigate and provide context on or fix the TestAccEphemeralServiceAccountKey_basic
failure? edit ah, you just beat me with most of them. Thanks!
See comment inline on bigquerydatatransfer
, also, a test/test(s) should be added.
No tests ran in that one, see comment above! |
Tests analyticsTotal tests: 4610 Click here to see the affected service packages
Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_bigquery_data_transfer_config" "primary" {
sensitive_params {
secret_access_key = # value needed
secret_access_key_wo = # value needed
}
sensitive_params_wo_version = # value needed
}
|
13c786b
to
0d265d0
Compare
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_bigquery_data_transfer_config" "primary" {
sensitive_params {
secret_access_key = # value needed
secret_access_key_wo = # value needed
}
sensitive_params_wo_version = # value needed
}
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_bigquery_data_transfer_config" "primary" {
sensitive_params {
secret_access_key = # value needed
secret_access_key_wo = # value needed
}
sensitive_params_wo_version = # 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_bigquery_data_transfer_config" "primary" {
sensitive_params {
secret_access_key = # value needed
secret_access_key_wo = # value needed
}
sensitive_params_wo_version = # value needed
}
|
Tests analyticsTotal tests: 4613 Click here to see the affected service packages
Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
Tests analyticsTotal tests: 4613 Click here to see the affected service packages
Action takenFound 9 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Tests analyticsTotal tests: 4614 Click here to see the affected service packages
Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
2 similar comments
Tests analyticsTotal tests: 4614 Click here to see the affected service packages
Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
Tests analyticsTotal tests: 4614 Click here to see the affected service packages
Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
@@ -210,6 +211,12 @@ properties: | |||
**NOTE** : If you are attempting to update a parameter that cannot be updated (due to api limitations) [please force recreation of the resource](https://www.terraform.io/cli/state/taint#forcing-re-creation-of-resources). | |||
required: true | |||
custom_flatten: 'templates/terraform/custom_flatten/json_to_string_map.go.tmpl' | |||
|
|||
- name: 'sensitiveParamsWoVersion' |
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.
I'd lean more towards keeping the version field with the WO field. So we'd support sensitive_params.0.secret_access_key_wo_version
here. While params
is sent directly, sensitive_params
isn't and is structured. That'll be easier to adapt later if we do introduce a template, and will keep a single pattern for all WO + WO version field pairs.
if _, exists := d.GetOkExists("sensitive_params.0." + sp); exists { | ||
delete(params, sp) | ||
} else { | ||
params[sp] = d.Get("params." + sp) |
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.
Doesn't this change mean we'll remove values from state where we wouldn't before? This seems like a regression.
Previously the logic was:
- do a presence check
- if present and in sensitive_params, don't set in state
- if present and not in sensitive_params, persist old state value
- else record value in state (will diff from null)
The new logic is:
- delete from state if present
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.
you're correct, the regression comes from if a user decides to explicitly set a secret_access_key
into params directly instead of sensitive_params
. If a user were to do this with the current change it would delete from state (regression).
Latest commit reverts this and adds a config check of _wo
attribute with RawConfigAt
d5b47a4
There might be something wrong with how RawConfigAt works. It only works on creation. Was getting null as the result. Enforcing the check with version fixes the diff just fine but feels janky. I'll get back to you on whether this is a bug or if this is meant to be like this, just need to confirm. The new commit: 113b096
mmv1/third_party/terraform/website/docs/r/sql_user.html.markdown
Outdated
Show resolved
Hide resolved
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_bigquery_data_transfer_config" "primary" {
sensitive_params {
secret_access_key = # value needed
secret_access_key_wo = # value needed
}
sensitive_params_wo_version = # value needed
}
|
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.
Errors
|
Tests analyticsTotal tests: 0 Click here to see the affected service packages
🔴 Tests were added that are skipped in VCR:
View the build log |
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.
Errors
|
Tests analyticsTotal tests: 0 Click here to see the affected service packages
🔴 Tests were added that are skipped in VCR:
View the build log |
Tests analyticsTotal tests: 4616 Click here to see the affected service packages
Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
This PR adds support for Write-Only attributes with the release of TF 1.11
It includes the following attributes:
password_wo
field togoogle_sql_user
#13011secretAccessKeyWo
#12967secret_data_wo
ingoogle_secret_version
#12800Some PRs that were necessary to support Write-Only attributes:
raw_resource_config_validation
forvalidation.PreferWriteOnlyAttribute
support #13048mmv1
: addhashicorp/gocty/cty
in resource.go.tmpl #12974EphemeralWriteOnly
: Initial support for MMv1 Generation in schema & docs #12550Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.