-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat(securityCenter): Adding SCC's BigQueryExport resource #3847
base: main
Are you sure you want to change the base?
Conversation
Here is the summary of changes. You are about to add 5 region tags.
This comment is generated by snippet-bot.
|
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.
This PR has combined changes for samples across products. Please clean up or delete and send a new PR. The title specifies bigquery, the referenced bug specifies security center, the code includes changes for both.
Once this is done, we can proceed with a code review.
I didn't understand what you meant, can you please elaborate on the comment. The bug doesn't have all the details but the requirement is to create samples for different components which are raised in separate PR's. This one has changes only related to the Big query. |
The changes in this PR have accidentally picked up changes for BigQuery and Security Center. If this PR is meant to make changes for BigQuery samples, please remove accidentally included Security Center code. |
Thank you for the clarification. I realize the title of the PR might have been misleading. The changes are scoped to Security Center snippets, and the BigQueryExport is a resource type under the Security Center that integrates with BigQuery. I will update the PR title to reflect this and avoid further confusion. |
@@ -81,7 +81,7 @@ jobs: | |||
- run: npm test | |||
env: | |||
GCLOUD_ORGANIZATION: 1081635000895 | |||
GOOGLE_SAMPLES_PROJECT: "long-door-651" | |||
GOOGLE_SAMPLES_PROJECT: "project-a-id" |
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.
issue: Use of this project is under discussion in #3830. It is probably not usable.
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.
Addressed.
@@ -94,5 +94,6 @@ | |||
"video-intelligence", | |||
"vision/productSearch", | |||
"workflows", | |||
"workflows/invoke-private-endpoint" | |||
"workflows/invoke-private-endpoint", | |||
"security-center/snippets/v2" |
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.
issue: #3883 means we shouldn't separately add v2.
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.
Addressed.
@@ -9,7 +9,7 @@ | |||
"node": ">=16.0.0" | |||
}, | |||
"scripts": { | |||
"test": "c8 mocha -p -j 2 --recursive --timeout 6000000 system-test/v2/findings.test.js" | |||
"test": "c8 mocha -p -j 2 --recursive --timeout 6000000 system-test/v2/" |
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.
issue: The test command for security center snippets overall should run all snippet tests, not just v2.
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.
Addressed.
const organizationId = '1081635000895'; | ||
const projectId = 'project-a-id'; |
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.
question: Shouldn't these be populated from environment variables configured on the workflow?
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.
Addressed.
@@ -27,7 +27,7 @@ const exec = cmd => execSync(cmd, {encoding: 'utf8'}); | |||
|
|||
// TODO(developers): update for your own environment | |||
const organizationId = '1081635000895'; | |||
const projectId = 'long-door-651'; | |||
const projectId = 'project-a-id'; |
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.
question: Shouldn't these be populated from environment variables configured on the workflow?
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.
Addressed.
* characters or less. | ||
*/ | ||
const bigQueryExportId = | ||
'bigqueryexportid-' + uuidv1().replace(/-/g, '').substring(0, 20); |
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.
issue: Please remove the unneeded dependency on uuid
, apply the Math.random
approach from #3830
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.
Addressed.
* "folders/[folder_id]/locations/[location_id]", or | ||
* "projects/[project_id]/locations/[location_id]". | ||
*/ | ||
const parent = client.organizationLocationPath(organizationId, location); |
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.
issue: Referencing organizationId and location from outside the region tag scope is a violation of the copy-paste-runnable goal for samples.
suggestion: Minimally, any referenced variable should have a commented out variable declaration with an instruction for use on each variable.
For example,
/**
* TODO(developer): Uncomment the following line before running the sample.
*/
// const projectId = 'YOUR_PROJECT_ID';
In https://github.com/GoogleCloudPlatform/nodejs-docs-samples/blob/main/security-center/snippets/v2/createFinding.js it appears the decision was made to hard-code location to global. If that's how we're testing, hardcoding the global value without comment and adding an inline comment explaining would be better than a commented out instantiation.
See #3830 for previously discussed instances
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.
Addressed.
* + `folders/{folder}/locations/{location}/bigQueryExports/{export_id}` | ||
* + `projects/{project}locations/{location}/bigQueryExports/{export_id}` | ||
*/ | ||
const name = `organizations/${organizationId}/locations/${location}/bigQueryExports/${exportId}`; |
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.
issue: Referencing organizationId and location from outside the region tag scope is a violation of the copy-paste-runnable goal for samples.
suggestion: Minimally, any referenced variable should have a commented out variable declaration with an instruction for use on each variable.
For example,
/**
* TODO(developer): Uncomment the following line before running the sample.
*/
// const projectId = 'YOUR_PROJECT_ID';
In https://github.com/GoogleCloudPlatform/nodejs-docs-samples/blob/main/security-center/snippets/v2/createFinding.js it appears the decision was made to hard-code location to global. If that's how we're testing, hardcoding the global value without comment and adding an inline comment explaining would be better than a commented out instantiation.
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.
Addressed.
* When paginating, all other parameters provided to `ListBigQueryExports` | ||
* must match the call that provided the page token. | ||
*/ | ||
// const pageToken = 'abc123' |
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.
issue: I don't see how the commented out variables are used in the snippet below. How is this guidance meant to be used?
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.
Addressed.
updateBigQueryExportRequest, | ||
fieldMask | ||
); | ||
console.log('BigQueryExport updated successfully!: %j', response); |
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.
issue: The guideline to process the result calls for outputting specific attributes in the response.
https://googlecloudplatform.github.io/samples-style-guide/#result
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.
Addressed.
Description
Fixes #349577531
Reference to https://b.corp.google.com/issues/349577531
This PR adds v2 API Big Query Node js client samples to Create, List, Get, Delete & Update.
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
npm test
(see Testing)npm run lint
(see Style)GoogleCloudPlatform/nodejs-docs-samples
. Not a fork.