-
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
Add two outputs for dlp job trigger: outputting to SCC and DataCatalog #6855
Add two outputs for dlp job trigger: outputting to SCC and DataCatalog #6855
Conversation
Oops! It looks like you're using an unknown release-note type in your changelog entries:
Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md. |
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @shuyama1, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
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.
@Wallace99 Thanks for making the change! The build failed due to
bundler: failed to load command: compiler (compiler)
RuntimeError: Field 'publish_summary_to_cscc' properties are nil!
Looks like publishSummaryToCscc
and publishFindingsToCloudDataCatalog
are added as nested object without subfields which will cause the build fail. Maybe consider adding some virtual subfields such as boolean field enable
to make sure nested object are non-empty?
Ah thanks, will update that shortly! |
Hi @shuyama1, sorry for the delay. Do you know of elsewhere in the repo I could reference for adding virtual subfields? Would some custom logic also be needed? To configure a DLP inspection job to output to SCC an empty JSON object called |
Hi @Wallace99, it seems that we may be able handle empty block schema by adding |
/gcbrun |
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 ( 3 files changed, 100 insertions(+), 6 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccLoggingBucketConfigProject_cmekSettings|TestAccDataLossPreventionJobTrigger_dlpJobTriggerUpdateExample|TestAccDataLossPreventionJobTrigger_dlpJobTriggerBigqueryRowLimitPercentageExample|TestAccDataLossPreventionJobTrigger_dlpJobTriggerBigqueryRowLimitExample|TestAccDataLossPreventionJobTrigger_dlpJobTriggerBasicExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
mmv1/templates/terraform/examples/dlp_job_trigger_bigquery_row_limit.tf.erb
Outdated
Show resolved
Hide resolved
mmv1/templates/terraform/examples/dlp_job_trigger_bigquery_row_limit_percentage.tf.erb
Outdated
Show resolved
Hide resolved
Hi @Wallace99! Would you mind also modifying the changelog a bit based on https://github.com/GoogleCloudPlatform/magic-modules/blob/main/.ci/RELEASE_NOTES_GUIDE.md? Thanks! |
Co-authored-by: Shuya Ma <87669292+shuyama1@users.noreply.github.com>
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 ( 3 files changed, 100 insertions(+), 6 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccDataLossPreventionJobTrigger_dlpJobTriggerBigqueryRowLimitExample|TestAccDataLossPreventionJobTrigger_dlpJobTriggerBigqueryRowLimitPercentageExample|TestAccLoggingBucketConfigProject_cmekSettings|TestAccFirebaserulesRelease_BasicRelease |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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.
Looks like only one of the fields in actions
can be specified. Therefore, I propose we add back exactly_one_of
and also include the newly added two fields. Plus, we need to use indexed path for exactly_one_of
(save_findings
-> inspect_job.0.actions.0.save_findings
).
Would you mind testing it locally first to make sure the test passed? It will be faster to see tests result running locally as our CI need to run the PR against the whole test suite.
The docs for the DLP rest api do say only one action can be specified, but I believe they're incorrect as I can create an inspection trigger with multiple actions both through the rest api and through the console. That's why I removed But yes, sorry I will locally test first! I've been having issues getting the tests to run locally so hoped this would be a trivial change that passed first try 😄 |
@Wallace99 Thanks for the explanation! Please let me know when you hear back from the API team, as based on the current error for the failed test, it seems that it still does not allow multiple actions to be set at the same time:
|
Hi @shuyama1, my mistake - I meant to say the DLP Python client rather than the REST api. Just now I verified that I can create an inspection job trigger with two actions using the Python client which from a quick look appears to be calling the RPCs here. I'm confused as to why then the test would be failing with the message above. I opened a support case seeking clarification, but they said that it's intentional that the REST api (and RPCs) only allow one action and that there's an internal request for this to be changed, but there's no timeline for when it will be done. I'm happy in the meantime to put back in |
@Wallace99 Thanks for the explanation. I'd propose to bring back |
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 ( 2 files changed, 94 insertions(+), 4 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccLoggingBucketConfigProject_cmekSettings|TestAccDataLossPreventionJobTrigger_dlpJobTriggerUpdateExample2|TestAccDataLossPreventionJobTrigger_dlpJobTriggerBigqueryRowLimitPercentageExample|TestAccDataLossPreventionJobTrigger_dlpJobTriggerBigqueryRowLimitExample |
Tests passed during RECORDING mode: All tests passed |
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.
LGTM! Only some nit-picks. Thank you for the work!
parent = "projects/<%= ctx[:test_env_vars]['project'] %>" | ||
description = "Description" | ||
display_name = "Displayname" | ||
|
||
triggers { | ||
schedule { | ||
recurrence_period_duration = "86400s" | ||
} | ||
} | ||
|
||
inspect_job { | ||
inspect_template_name = "fake" | ||
actions { | ||
publish_findings_to_cloud_data_catalog { | ||
} | ||
} | ||
storage_config { | ||
big_query_options { | ||
table_reference { | ||
project_id = "project" | ||
dataset_id = "dataset" | ||
table_id = "table_to_scan" | ||
} | ||
|
||
rows_limit_percent = 50 | ||
sample_method = "RANDOM_START" | ||
} | ||
} | ||
} | ||
} |
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.
nit: spacing is a bit off here - we use spaces instead of tabs for resource configuration for tests
parent = "projects/<%= ctx[:test_env_vars]['project'] %>" | ||
description = "Description" | ||
display_name = "Displayname" | ||
|
||
triggers { | ||
schedule { | ||
recurrence_period_duration = "86400s" | ||
} | ||
} | ||
|
||
inspect_job { | ||
inspect_template_name = "fake" | ||
actions { | ||
publish_summary_to_cscc { | ||
} | ||
} | ||
storage_config { | ||
big_query_options { | ||
table_reference { | ||
project_id = "project" | ||
dataset_id = "dataset" | ||
table_id = "table_to_scan" | ||
} | ||
|
||
rows_limit_percent = 50 | ||
sample_method = "RANDOM_START" | ||
} | ||
} | ||
} | ||
} |
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.
nit: spacing here
623a74e
to
a777a12
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 ( 2 files changed, 94 insertions(+), 4 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccRegionInstanceGroupManager_stateful|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample |
Tests failed during RECORDING mode: Please fix these to complete your PR |
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.
Failing tests are unrelated! Thank you for the work!
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, I just realize that we didn't include the newly added examples to the terraform.yaml
file, so no tests or doc examples is generated. Sorry for missing this earlier! I thought I saw related tests passed at some point but I guess those were only existing ones. Some examples of how to refer test/doc examples in the terraform.yaml
file at https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/products/dlp/terraform.yaml#L76
Plus, I think we can fairly skip generating these two examples in the web docs by adding skip_docs: true
in the terraform.yaml
file. Please let me know if you have any question generating the tests. 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. Terraform GA: Diff ( 3 files changed, 216 insertions(+), 4 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccDataLossPreventionJobTrigger_dlpJobTriggerSccOutputExample|TestAccDataLossPreventionJobTrigger_dlpJobTriggerDataCatalogOutputExample|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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.
Thank you!
GoogleCloudPlatform#6855) Co-authored-by: Shuya Ma <87669292+shuyama1@users.noreply.github.com> Fixes hashicorp/terraform-provider-google#11861
Add in outputting results of DLP job to SCC and DataCatalog. Also fix description of
saveFindings
block and remove unnecessary restriction on only one output.Fixes hashicorp/terraform-provider-google#11861
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)