feat: add a REST sample and test for AML AI #3809

irataxy · 2024-08-28T01:14:51Z

Description

Fixes b: 355523386

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

Checklist

I have followed guidelines from CONTRIBUTING.MD and Samples Style Guide
Tests pass: npm test (see Testing)
Lint pass: npm run lint (see Style)
These samples need a new API enabled in testing projects to pass (let us know which ones)
These samples need a new/updated env vars in testing projects set to pass (let us know which ones)
This pull request is from a branch created directly off of GoogleCloudPlatform/nodejs-docs-samples. Not a fork.
This sample adds a new sample directory, and I updated the CODEOWNERS file with the codeowners for this sample
This sample adds a new sample directory, and I created GitHub Actions workflow for this sample
[] This sample adds a new Product API, and I updated the Blunderbuss issue/PR auto-assigner with the codeowners for this sample
Please merge this PR for me once it is approved

snippet-bot · 2024-08-28T01:14:55Z

Here is the summary of changes.

You are about to add 1 region tag.

aml-ai/listLocations.js:19, tag antimoneylaunderingai_list_locations

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

CODEOWNERS

subfuzion

A few changes requested, but looks good overall. Thanks.

aml-ai/listLocations.js

subfuzion

Approved. Test fails because financialservices.googleapis.com isn't enabled (pending: cl/674939991).

subfuzion · 2024-09-15T22:26:50Z

aml-ai/listLocations.js

+
+// node listLocations.js
+main().catch(err => {
+  console.error(err);


Changed this so mocha's reporter prints the reason for the failure. The current mocha test reporter filters out stdout but prints stderr, see logs using console.log() vs. console.error().

cl/674939991 is merged, waiting on API enablement now.

Hi @irataxy The API was enabled (internal: cl/676595619). Looks like a few conflicts must be resolved to get this merged, however. Do you have bandwidth to follow up, or would you like to convert this to a draft or close it for now?

Converted to draft for now

@irataxy The PR to enable the financialservices api was merged. I'm going to take this out of draft mode and rerun tests to confirm.

Per @irataxy, proper client libraries for this are coming soon. Closing for now. @irataxy Feel free to reopen or create a new PR when you're ready. Thanks!

code-review-assist-experimental

Hi @irataxy and reviewers,

I'm currently reviewing this pull request and will post my detailed review in a few minutes. In the meantime, here's a quick summary to help everyone get up to speed:

This pull request adds a new REST sample and test for the AML AI product. The changes include:

A new directory aml-ai containing the sample code (listLocations.js) and its corresponding test (aml-ai.test.js).
A new GitHub Actions workflow (aml-ai.yaml) for building and testing the sample.
Updates to CODEOWNERS to assign ownership of the new sample.
Updates to .github/workflows/utils/workflows.json to include the new workflow.
A package.json file for managing dependencies within the aml-ai directory.

The intent is to provide a clear and concise example of how to use the AML AI API to list locations. The test verifies that the sample correctly retrieves the expected location information.

If there's anything unclear or missing from this summary, please let me know. I'll provide a more detailed review shortly.

And now, a little haiku to lighten the mood:

Code flows like a stream,
Tests run, a green light appears,
Joy fills the coder's heart.

code-review-assist-experimental

This pull request adds a REST sample and test for AML AI. The implementation looks good, and the inclusion of tests and adherence to the contribution guidelines is commendable. Here are a few suggestions for improvement:

code-review-assist-experimental · 2024-11-15T18:21:57Z

.github/workflows/aml-ai.yaml

+    permissions:
+      contents: 'read'
+      id-token: 'write'
+    if: github.event.action != 'labeled' || github.event.label.name == 'actions:force-run'


Consider adding a comment explaining the purpose of the actions:force-run label and when it should be used.

Suggested change

if: github.event.action != 'labeled' || github.event.label.name == 'actions:force-run'

if: github.event.action != 'labeled' || github.event.label.name == 'actions:force-run' # Allows manual rerun via label

code-review-assist-experimental · 2024-11-15T18:21:57Z

.github/workflows/aml-ai.yaml

+    permissions:
+      contents: 'read'
+      id-token: 'write'
+    if: github.event_name == 'schedule' && always() # always() submits logs even if tests fail


Consider adding a comment explaining what the always() function does in this context.

Suggested change

if: github.event_name == 'schedule' && always() # always() submits logs even if tests fail

if: github.event_name == 'schedule' && always() # Submits logs even if tests fail

code-review-assist-experimental · 2024-11-15T18:21:58Z

aml-ai/listLocations.js

+      scopes: 'https://www.googleapis.com/auth/cloud-platform',
+      headers: {'Content-Type': 'application/json; charset=utf-8'},
+    });
+    // TODO(developer): update the project ID as needed


The TODO should be more specific about how the developer should update the project ID. Is it via an environment variable? Should it be hardcoded? Provide an example.

Suggested change

// TODO(developer): update the project ID as needed

// TODO(developer): Replace with your project ID. For example:

// const projectId = 'my-project-id';

code-review-assist-experimental · 2024-11-15T18:21:58Z

aml-ai/listLocations.js

+    try {
+      response = await client.request({url, method: 'GET'});
+    } catch (err) {
+      throw new Error(`API request failed: ${err}`);


Instead of just logging the error message, consider including the original error object (err) as well. This can be helpful for debugging. Also, consider logging the status code if available.

Suggested change

throw new Error(`API request failed: ${err}`);

throw new Error(`API request failed: ${err.message} (status: ${err.status}, error: ${err})`);

code-review-assist-experimental · 2024-11-15T18:21:58Z

aml-ai/listLocations.js

+    console.log(JSON.stringify(response.data));
+  };
+
+  return await listLocations();


The await keyword is unnecessary here since listLocations is already awaited when called. Removing it won't change the behavior but will slightly improve performance.

Suggested change

return await listLocations();

return listLocations();

code-review-assist-experimental · 2024-11-15T18:21:58Z

aml-ai/system-test/aml-ai.test.js

+const cloudRegion = 'us-central1';
+
+it('should get the AML AI API locations', () => {
+  const output = execSync('node listLocations.js', {cwd});


It's good practice to assert on the full response or specific fields within the response data, rather than just checking if a string is included. This makes the test more robust and less prone to false positives. Also, consider using JSON.parse() to parse the output and assert on the structured data.

Suggested change

const output = execSync('node listLocations.js', {cwd});

const output = JSON.parse(execSync('node listLocations.js', {cwd}));

assert.ok(output.locations.some(location => location.locationId === cloudRegion));

docs: add a REST sample and test for AML AI

d0873bf

irataxy requested review from a team as code owners August 28, 2024 01:14

product-auto-label bot added samples Issues that are directly related to samples. api: workflows Issues related to the Workflows API. labels Aug 28, 2024

blunderbuss-gcf bot assigned pattishin Aug 28, 2024

irataxy changed the title ~~docs: add a REST sample and test for AML AI~~ feat: add a REST sample and test for AML AI Aug 28, 2024

iennae reviewed Aug 28, 2024

View reviewed changes

CODEOWNERS Show resolved Hide resolved

subfuzion unassigned pattishin Sep 3, 2024

subfuzion suggested changes Sep 3, 2024

View reviewed changes

aml-ai/listLocations.js Outdated Show resolved Hide resolved

aml-ai/listLocations.js Outdated Show resolved Hide resolved

aml-ai/listLocations.js Outdated Show resolved Hide resolved

aml-ai/listLocations.js Outdated Show resolved Hide resolved

aml-ai/listLocations.js Show resolved Hide resolved

irataxy and others added 2 commits September 11, 2024 13:06

Merge branch 'main' into aml

0ca1215

address feedback

0cc6c9e

irataxy requested a review from subfuzion September 11, 2024 20:12

subfuzion and others added 2 commits September 15, 2024 13:51

Merge branch 'main' into aml

d399ab3

refactor: use error instead of log

100207c

subfuzion approved these changes Sep 15, 2024

View reviewed changes

irataxy marked this pull request as draft October 7, 2024 18:17

subfuzion marked this pull request as ready for review November 15, 2024 18:07

subfuzion requested a review from a team as a code owner November 15, 2024 18:07

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

View reviewed changes

Merge branch 'main' into aml

6d85a88

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

View reviewed changes

subfuzion closed this Nov 18, 2024

	if: github.event.action != 'labeled' \|\| github.event.label.name == 'actions:force-run'
	if: github.event.action != 'labeled' \|\| github.event.label.name == 'actions:force-run' # Allows manual rerun via label

	if: github.event_name == 'schedule' && always() # always() submits logs even if tests fail
	if: github.event_name == 'schedule' && always() # Submits logs even if tests fail

	// TODO(developer): update the project ID as needed
	// TODO(developer): Replace with your project ID. For example:
	// const projectId = 'my-project-id';

	throw new Error(`API request failed: ${err}`);
	throw new Error(`API request failed: ${err.message} (status: ${err.status}, error: ${err})`);

	const output = execSync('node listLocations.js', {cwd});
	const output = JSON.parse(execSync('node listLocations.js', {cwd}));
	assert.ok(output.locations.some(location => location.locationId === cloudRegion));

feat: add a REST sample and test for AML AI #3809

feat: add a REST sample and test for AML AI #3809

Uh oh!

Conversation

irataxy commented Aug 28, 2024

Description

Checklist

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

subfuzion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

subfuzion left a comment

Choose a reason for hiding this comment

Uh oh!

subfuzion Sep 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subfuzion Sep 16, 2024

Choose a reason for hiding this comment

Uh oh!

subfuzion Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

irataxy Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

subfuzion Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

subfuzion Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

code-review-assist-experimental bot left a comment

Choose a reason for hiding this comment

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 15, 2024

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

snippet-bot bot commented Aug 28, 2024 •

edited

Loading

subfuzion Sep 15, 2024 •

edited

Loading