-
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
Add resources for Cloud Logging default settings #9409
Conversation
Hello! I am a robot. It looks like you are a: @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. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Breaking Change Detection FailedThe breaking change detector crashed during execution. This is usually due to the downstream provider(s) failing to compile. Please investigate or follow up with your reviewer. Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 10 files changed, 1867 insertions(+), 2 deletions(-)) |
Hi @zachberger, |
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 ( 5 files changed, 1008 insertions(+), 2 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_logging_folder_settings" "primary" {
disable_default_sink = # value needed
folder = # value needed
kms_key_name = # value needed
kms_service_account_id = # value needed
storage_location = # value needed
}
Resource: resource "google_logging_organization_settings" "primary" {
disable_default_sink = # value needed
kms_key_name = # value needed
organization = # value needed
storage_location = # value needed
}
|
Tests analyticsTotal 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. Terraform GA: Diff ( 7 files changed, 1272 insertions(+), 2 deletions(-)) |
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 testsTestAccLoggingOrganizationSettings_loggingOrganizationSettingsExample|TestAccLoggingFolderSettings_loggingFolderSettingsExample |
|
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 ( 7 files changed, 1272 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccLoggingFolderSettings_loggingFolderSettingsExample|TestAccLoggingOrganizationSettings_loggingOrganizationSettingsExample|TestAccDataSourceGoogleServiceAccountJwt |
Rerun these tests in REPLAYING mode to catch issues
|
0834a51
to
5728d2c
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. Terraform GA: Diff ( 7 files changed, 1276 insertions(+), 2 deletions(-)) |
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. Terraform GA: Diff ( 7 files changed, 1276 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataprocClusterIamPolicy|TestAccLoggingOrganizationSettings_loggingOrganizationSettingsExample|TestAccLoggingFolderSettings_loggingFolderSettingsExample |
Rerun these tests in REPLAYING mode to catch issues
|
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 ( 11 files changed, 1662 insertions(+), 2 deletions(-)) |
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, 1844 insertions(+), 2 deletions(-)) |
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, 1744 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccHealthcareDatasetIamPolicy|TestAccLoggingFolderSettings_datasource|TestAccLoggingProjectSettings_datasource|TestAccLoggingOrganizationSettings_loggingOrganizationSettingsExample|TestAccLoggingFolderSettings_loggingFolderSettingsExample |
Rerun these tests in REPLAYING mode to catch issues
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.
|
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, 1744 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccLoggingOrganizationSettings_loggingOrganizationSettingsExample|TestAccLoggingFolderSettings_loggingFolderSettingsExample|TestAccLoggingProjectSettings_datasource|TestAccDataSourceGoogleServiceAccountIdToken_impersonation |
|
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, 1744 insertions(+), 2 deletions(-)) |
Rerun these tests in REPLAYING mode to catch issues
|
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 ( 9 files changed, 1624 insertions(+), 2 deletions(-)) |
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 testsTestAccLoggingOrganizationSettings_loggingOrganizationSettingsOnlyDisableDefaultExample|TestAccLoggingOrganizationSettings_loggingOrganizationSettingsOnlyLocationExample |
Rerun these tests in REPLAYING mode to catch issues
|
Is there any way to remove |
No, you would have to implement the test as an Looking at what you have, you might want to consider dropping some of the variants you have, to reduce the number of tests and eliminate the chance for conflicting tests. For example:
|
FWIW, everything besides those remaining tests LGTM at this point |
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 ( 8 files changed, 1181 insertions(+), 2 deletions(-)) |
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 ( 8 files changed, 1181 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataprocClusterIamPolicy|TestAccLoggingOrganizationSettings_update|TestAccLoggingFolderSettings_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.
Few remaining comments, but code LGTM and tests look good too. Main blocker is verifying that the updateMask isn't doing something unintended like clearing existing fields that are not set by the user.
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.
Whoops, forgot to submit the comments in the previous review
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 ( 8 files changed, 1181 insertions(+), 2 deletions(-)) |
I did a manual tests of this:
|
Tests analyticsTotal tests:
|
Thanks @roaks3! |
…#9409) * Add folder and organization setting resources * Try GetTestOrgTargetFromEnv instead of GetTestOrgFromEnv. * Use correct organization for testing. * Remove key rotation to fix VCR test. * Don't specify fields matching default values. * Specify true instead of yes. * Use BootstrapKMSKeyInLocation instead of creating new keys. * Add missing quotes * Add additional examples to generate additional tests. * Remove unneeded resources from examples. * Simplify tests to be one full resource creation and one update. * Fix typo in test * Document and cleanup example.
…#9409) * Add folder and organization setting resources * Try GetTestOrgTargetFromEnv instead of GetTestOrgFromEnv. * Use correct organization for testing. * Remove key rotation to fix VCR test. * Don't specify fields matching default values. * Specify true instead of yes. * Use BootstrapKMSKeyInLocation instead of creating new keys. * Add missing quotes * Add additional examples to generate additional tests. * Remove unneeded resources from examples. * Simplify tests to be one full resource creation and one update. * Fix typo in test * Document and cleanup example.
Add resources for Cloud Logging default settings.
Fixes: hashicorp/terraform-provider-google#15827
Release Note Template for Downstream PRs (will be copied)