feat(security-center): Add Resource v2 API Assets Security Marks Samples #3916

vijaykanthm · 2024-11-04T20:13:47Z

Description

This PR adds v2 API Assets Security Marks Node js client samples to Add Security Marks, Delete Security Marks, Add Delete Security Marks.

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

Checklist

conventional-commit-lint-gcf · 2024-11-04T20:13:53Z

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

snippet-bot · 2024-11-04T20:35:21Z

Here is the summary of changes.

You are about to add 3 region tags.

security-center/snippets/v2/addDeleteSecurityMarks.js:21, tag securitycenter_add_delete_security_marks_v2
security-center/snippets/v2/addSecurityMarks.js:21, tag securitycenter_add_security_marks_v2
security-center/snippets/v2/deleteAssetsSecurityMarks.js:21, tag securitycenter_delete_security_marks_v2

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

…form/nodejs-docs-samples into security-marks-assets-v2 merge remote to local

feywind

This generally looks fine, maybe just check on that exec() vs execSync() thing. Let's see if we can get someone closer to docs/samples to approve the rest of the way.

feywind · 2024-11-13T20:15:09Z

security-center/snippets/system-test/v2/assetSecurityMarks.test.js

+      parent: client.organizationPath(organizationId),
+    });
+    const randomAsset =
+      assetResults[Math.floor(Math.random() * assetResults.length)].asset;


Seeing random() in a test makes me worry that it's going to end up being flaky, but I guess if all the items are more or less equivalent, should be okay.

Refactored it.

feywind · 2024-11-13T20:16:32Z

security-center/snippets/system-test/v2/assetSecurityMarks.test.js

+
+  it('client can add and delete security marks', () => {
+    // Ensure marks are set.
+    exec(`node v2/addSecurityMarks.js ${data.assetName}`);


Should these be execSync()?

Addressed. Other tests were using the exec() method so I have used the same but I have updated it.

iennae · 2024-11-16T00:12:15Z

@gemini review

code-review-assist-experimental

This pull request adds samples for the Security Center v2 API's Asset Security Marks in Node.js. The new samples demonstrate adding, deleting, and adding/deleting security marks. The changes also include updates to the system tests to incorporate these new functionalities. The implementation looks good overall, but I have a few suggestions to improve clarity, consistency, and robustness.

One improvement could be to add more comprehensive documentation in the README file for the security-center/snippets/v2 directory. This documentation should explain the purpose of each sample, how to run them, and the expected outcomes. It should also clearly outline the prerequisites, such as setting up a Security Center project and enabling the necessary APIs.

Additionally, the system tests could be enhanced by adding more assertions to validate the state of the security marks after each operation. This would ensure that the samples are functioning correctly and provide more confidence in the code's reliability.

code-review-assist-experimental · 2024-11-16T00:29:15Z

security-center/snippets/system-test/v1/notifications.test.js

+      try {
+        /*eslint no-unused-vars: ["error", { "varsIgnorePattern": "^_" }]*/
+        const [_response] = await client.createNotificationConfig({
+          parent: orgName,
+          configId: configId,
+          notificationConfig: {
+            description: 'Sample config for node.js',
+            pubsubTopic: pubsubTopic,
+            streamingConfig: {filter: 'state = "ACTIVE"'},
+          },
+        });
+      } catch (err) {
+        console.error(`Error creating config ${configId}:`, err.message);
+      }


Adding a try-catch block is a good practice for error handling. Consider adding more specific error handling within the try block to catch potential issues like incorrect input or network errors. This would make debugging easier and improve the robustness of the tests.

code-review-assist-experimental · 2024-11-16T00:29:15Z

security-center/snippets/system-test/v1/notifications.test.js

    await createNotificationConfig(deleteConfig);
+    await new Promise(resolve => setTimeout(resolve, 1000)); // 1-second delay
    await createNotificationConfig(getConfig);
+    await new Promise(resolve => setTimeout(resolve, 1000)); // 1-second delay
    await createNotificationConfig(listConfig);
+    await new Promise(resolve => setTimeout(resolve, 1000)); // 1-second delay
    await createNotificationConfig(updateConfig);


Adding delays using setTimeout can make tests flaky and increase execution time. Consider using a more robust approach like polling or a dedicated testing library that provides utilities for waiting for asynchronous operations to complete.

code-review-assist-experimental · 2024-11-16T00:29:16Z

security-center/snippets/system-test/v1/notifications.test.js

    async function deleteNotificationConfig(configId) {
-      const name = client.organizationNotificationConfigPath(
-        organizationId,
-        configId
-      );
-      await client.deleteNotificationConfig({name: name});
+      try {
+        const name = client.organizationNotificationConfigPath(
+          organizationId,
+          configId
+        );
+        await client.deleteNotificationConfig({name: name});
+      } catch (err) {
+        console.error(`Error deleting config ${configId}:`, err.message);
+      }


Similar to the creation process, adding a try-catch block is good practice for error handling during deletion. Consider adding more specific error handling within the try block.

code-review-assist-experimental · 2024-11-16T00:29:16Z

security-center/snippets/system-test/v1/notifications.test.js

    await deleteNotificationConfig(createConfig);
+    await new Promise(resolve => setTimeout(resolve, 1000)); // 1-second delay
    await deleteNotificationConfig(getConfig);
+    await new Promise(resolve => setTimeout(resolve, 1000)); // 1-second delay
    await deleteNotificationConfig(listConfig);
+    await new Promise(resolve => setTimeout(resolve, 1000)); // 1-second delay
    await deleteNotificationConfig(updateConfig);


Using setTimeout for delays can lead to flaky tests. Consider using a more robust approach like polling or a dedicated testing library for waiting for asynchronous operations.

code-review-assist-experimental · 2024-11-16T00:29:16Z

security-center/snippets/system-test/v2/assetSecurityMarks.test.js

+const {execSync} = require('child_process');
+
+// TODO(developers): update for your own environment
+const organizationId = '1081635000895';


The organization ID is hardcoded. Consider storing this in an environment variable or a configuration file to make the test more portable and avoid exposing sensitive information directly in the code.

Suggested change

const organizationId = '1081635000895';

// Retrieve organization ID from environment variable

const organizationId = process.env.ORGANIZATION_ID || 'YOUR_ORGANIZATION_ID';

if (!organizationId) {

throw new Error('Environment variable ORGANIZATION_ID is not set.');

}

code-review-assist-experimental · 2024-11-16T00:29:16Z

security-center/snippets/v2/addSecurityMarks.js

+    // assetName is the full resource path for the asset to update.
+    /*
+     * TODO(developer): Uncomment the following lines
+     */
+    // Specify the value of 'assetName' in one of the following formats:
+    //    `organizations/${org-id}/assets/${asset-id}`;
+    //    `projects/${project-id}/assets/${asset-id}`;
+    //    `folders/${folder-id}/assets/${asset-id}`;
+    // const assetName = "organizations/123123342/assets/12312321";


The TODO comments suggest uncommenting lines to specify the asset name. It's better to provide clear instructions or examples on how to obtain and use the asset name rather than relying on commented-out code.

security-center/snippets/v2/addSecurityMarks.js

security-center/snippets/v2/deleteAssetsSecurityMarks.js

Add Resource v2 assets security marks

129b04d

vijaykanthm requested review from a team as code owners November 4, 2024 20:13

product-auto-label bot added api: securitycenter Issues related to the Security Command Center API. samples Issues that are directly related to samples. labels Nov 4, 2024

vijaykanthm added 2 commits November 4, 2024 20:18

fix(security center): lint fix

dbe226b

Merge branch 'main' into security-marks-assets-v2

a948f68

vijaykanthm added 3 commits November 5, 2024 22:13

refactor notifications test to fix quota limit issue

5d8a033

Merge branch 'security-marks-assets-v2' of github.com:GoogleCloudPlat…

91e531e

…form/nodejs-docs-samples into security-marks-assets-v2 merge remote to local

fix lint issue

0201997

feywind reviewed Nov 13, 2024

View reviewed changes

vijaykanthm added 3 commits November 14, 2024 06:46

refactor the test file to address comments

f29640b

refactor the test to fix lint issue

a2eba15

pick the first asset as random

9a85b6b

vijaykanthm requested a review from feywind November 14, 2024 17:56

code-review-assist-experimental bot reviewed Nov 16, 2024

View reviewed changes

vijaykanthm added 3 commits November 19, 2024 22:37

Address comments by bot:

35e7fb7

Merge branch 'main' into security-marks-assets-v2

2fac912

fix test in notification

00edc7f

owenhuyn approved these changes Nov 26, 2024

View reviewed changes

Merge branch 'main' into security-marks-assets-v2

5094475

iennae merged commit 0469e1f into main Nov 27, 2024
136 checks passed

iennae deleted the security-marks-assets-v2 branch November 27, 2024 06:09

-const organizationId = '1081635000895';
+// Retrieve organization ID from environment variable
+const organizationId = process.env.ORGANIZATION_ID || 'YOUR_ORGANIZATION_ID';
+if (!organizationId) {
+  throw new Error('Environment variable ORGANIZATION_ID is not set.');
+}

feat(security-center): Add Resource v2 API Assets Security Marks Samples #3916

feat(security-center): Add Resource v2 API Assets Security Marks Samples #3916

Uh oh!

Conversation

vijaykanthm commented Nov 4, 2024

Description

Checklist

Uh oh!

conventional-commit-lint-gcf bot commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snippet-bot bot commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feywind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iennae commented Nov 16, 2024

Uh oh!

code-review-assist-experimental bot left a comment

Choose a reason for hiding this comment

Uh oh!

code-review-assist-experimental bot Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-review-assist-experimental bot Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-review-assist-experimental bot Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-review-assist-experimental bot Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-review-assist-experimental bot Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

code-review-assist-experimental bot Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

conventional-commit-lint-gcf bot commented Nov 4, 2024 •

edited

Loading

snippet-bot bot commented Nov 4, 2024 •

edited

Loading