Skip to content
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 Resource v1 SCC Findings Export to BQ Folder Config #11587

Conversation

vijaykanthm
Copy link
Contributor

@vijaykanthm vijaykanthm commented Aug 29, 2024

Related to http://b/356159589.

Bug Description: hashicorp/terraform-provider-google#18848

Adding new template for V1 SCC Findings Exports to Big Query Folder Config
https://cloud.google.com/security-command-center/docs/reference/rest/v1/folders.bigQueryExports

Release Note Template for Downstream PRs (will be copied)

`google_scc_folder_scc_big_query_export`

@github-actions github-actions bot requested a review from melinath August 29, 2024 22:55
Copy link

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@melinath, 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.

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

mmv1/products/securitycenter/FolderSccBigQueryExports.yaml Outdated Show resolved Hide resolved
A Cloud Security Command Center (Cloud SCC) Big Query Export Config.
It represents exporting Security Command Center data, including assets, findings, and security marks
using gcloud scc bqexports
~> **Note:** In order to use Cloud SCC resources, your organization must be enrolled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand what you are trying to say here, the example link which you have provided for notification Config doesn't have a newline. I have used the document preview tool for the existing description in my code and it is the same as that of the notification config description.

image

Copy link
Member

@melinath melinath Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes - sorry for being unclear. Both resources are incorrect. The link was to a resource that looks wrong. A warning note is supposed to display like this:

Screenshot 2024-09-11 at 3 05 07 PM

Source: https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/bigquery_dataset

An informational note should display like this:

Screenshot 2024-09-11 at 3 06 53 PM

Source: https://registry.terraform.io/providers/hashicorp/google/latest/docs/data-sources/compute_instance_template

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for providing clarity on this. Addressed.

mmv1/products/securitycenter/FolderSccBigQueryExports.yaml Outdated Show resolved Hide resolved
mmv1/products/securitycenter/FolderSccBigQueryExports.yaml Outdated Show resolved Hide resolved
mmv1/products/securitycenter/FolderSccBigQueryExports.yaml Outdated Show resolved Hide resolved
mmv1/products/securitycenter/FolderSccBigQueryExports.yaml Outdated Show resolved Hide resolved
mmv1/products/securitycenter/FolderSccBigQueryExports.yaml Outdated Show resolved Hide resolved
mmv1/products/securitycenter/FolderSccBigQueryExports.yaml Outdated Show resolved Hide resolved
melinath

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@melinath

This comment was marked as outdated.

PreCheck: func() { acctest.AccTestPreCheck(t) },
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t),
ExternalProviders: map[string]resource.ExternalProvider{
"random": {},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: random provider isn't used

Suggested change
"random": {},

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed.

"github.com/hashicorp/terraform-provider-google/google/envvar"
)

func TestAccSecurityCenterFolderBigQueryExportConfig_basic(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func TestAccSecurityCenterFolderBigQueryExportConfig_basic(t *testing.T) {
func TestAccSecurityCenterFolderBigQueryExportConfig_update(t *testing.T) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed.

@melinath
Copy link
Member

Also clarified the change request on #11587 (comment)

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 5 files changed, 926 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 5 files changed, 926 insertions(+), 2 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 89 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 17
Passed tests: 16
Skipped tests: 0
Affected tests: 1

Click here to see the affected service packages
  • securitycenter

Action taken

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
  • TestAccSecurityCenterFolderBigQueryExportConfig_update

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccSecurityCenterFolderBigQueryExportConfig_update[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$

View the build log or the debug log for each test

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 5 files changed, 926 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 5 files changed, 926 insertions(+), 2 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 89 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 17
Passed tests: 17
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages
  • securitycenter

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log

Comment on lines 16 to 17
dataset_id := "tf_test_" + randomSuffix
dataset_id2 := "tf_test_" + randomSuffix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle this works, but right now the values here are both the same, so it won't actually trigger a change to the dataset resource.

Suggested change
dataset_id := "tf_test_" + randomSuffix
dataset_id2 := "tf_test_" + randomSuffix
dataset_id := "tf_test_" + randomSuffix
dataset_id2 := "tf_test_" + acctest.RandString(t, 10)

Or alternatively:

Suggested change
dataset_id := "tf_test_" + randomSuffix
dataset_id2 := "tf_test_" + randomSuffix
dataset_id := "tf_test_" + randomSuffix
dataset_id2 := dataset_id + "2"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 5 files changed, 926 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 5 files changed, 926 insertions(+), 2 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 89 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 17
Passed tests: 16
Skipped tests: 0
Affected tests: 1

Click here to see the affected service packages
  • securitycenter

Action taken

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
  • TestAccSecurityCenterFolderBigQueryExportConfig_update

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccSecurityCenterFolderBigQueryExportConfig_update[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$

View the build log or the debug log for each test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants