Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions .github/workflows/aml-ai.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
# Copyright 2024 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

name: aml-ai
on:
push:
branches:
- main
paths:
- 'aml-ai/**'
- '.github/workflows/aml-ai.yaml'
pull_request:
types:
- opened
- reopened
- synchronize
- labeled
paths:
- 'aml-ai/**'
- '.github/workflows/aml-ai.yaml'
schedule:
- cron: '0 0 * * 0'
jobs:
test:
# Ref: https://github.com/google-github-actions/auth#usage
permissions:
contents: 'read'
id-token: 'write'
if: github.event.action != 'labeled' || github.event.label.name == 'actions:force-run'
Copy link
Contributor

Choose a reason for hiding this comment

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

low

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

uses: ./.github/workflows/test.yaml
with:
name: 'aml-ai'
path: 'aml-ai'
flakybot:
# Ref: https://github.com/google-github-actions/auth#usage
permissions:
contents: 'read'
id-token: 'write'
if: github.event_name == 'schedule' && always() # always() submits logs even if tests fail
Copy link
Contributor

Choose a reason for hiding this comment

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

low

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

uses: ./.github/workflows/flakybot.yaml
needs: [test]
1 change: 1 addition & 0 deletions .github/workflows/utils/workflows.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
[
"aml-ai",
"appengine/analytics",
"appengine/building-an-app/build",
"appengine/building-an-app/update",
Expand Down
1 change: 1 addition & 0 deletions CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ vision @GoogleCloudPlatform/dee-data-ai @GoogleCloudPlatform/nodejs-samples-revi

# Self-service
ai-platform @GoogleCloudPlatform/dee-data-ai @GoogleCloudPlatform/nodejs-samples-reviewers @GoogleCloudPlatform/text-embedding @GoogleCloudPlatform/cloud-samples-reviewers
aml-ai @GoogleCloudPlatform/aml-ai @GoogleCloudPlatform/nodejs-samples-reviewers @GoogleCloudPlatform/cloud-samples-reviewers
asset @GoogleCloudPlatform/cloud-asset-analysis-team @GoogleCloudPlatform/nodejs-samples-reviewers @GoogleCloudPlatform/cloud-samples-reviewers
dlp @GoogleCloudPlatform/googleapis-dlp @GoogleCloudPlatform/nodejs-samples-reviewers @GoogleCloudPlatform/cloud-samples-reviewers
security-center @GoogleCloudPlatform/gcp-security-command-center @GoogleCloudPlatform/nodejs-samples-reviewers @GoogleCloudPlatform/cloud-samples-reviewers
Expand Down
50 changes: 50 additions & 0 deletions aml-ai/listLocations.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/**
* Copyright 2024 Google LLC
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

'use strict';

const main = async () => {
// [START antimoneylaunderingai_list_locations]
// Import google-auth-library for authentication.
const {GoogleAuth} = require('google-auth-library');

const listLocations = async () => {
const auth = new GoogleAuth({
scopes: 'https://www.googleapis.com/auth/cloud-platform',
headers: {'Content-Type': 'application/json; charset=utf-8'},
});
// TODO(developer): update the project ID as needed
Copy link
Contributor

Choose a reason for hiding this comment

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

low

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';

const projectId = process.env.GOOGLE_CLOUD_PROJECT;
const url = `https://financialservices.googleapis.com/v1/projects/${projectId}/locations`;
const client = await auth.getClient();
let response;
try {
response = await client.request({url, method: 'GET'});
} catch (err) {
throw new Error(`API request failed: ${err}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

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})`);

}

console.log(JSON.stringify(response.data));
};

return await listLocations();
Copy link
Contributor

Choose a reason for hiding this comment

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

low

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();

// [END antimoneylaunderingai_list_locations]
};

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

@subfuzion subfuzion Sep 15, 2024

Choose a reason for hiding this comment

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

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().

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converted to draft for now

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

process.exitCode = 1;
});
19 changes: 19 additions & 0 deletions aml-ai/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"name": "nodejs-docs-samples-aml-ai",
"version": "0.0.1",
"private": true,
"license": "Apache-2.0",
"author": "Google LLC",
"repository": "GoogleCloudPlatform/nodejs-docs-samples",
"engines": {
"node": ">=12.0.0"
},
"scripts": {
"test": "c8 mocha -p -j 2 system-test/*.test.js --timeout=60000"
},
"devDependencies": {
"c8": "^10.0.0",
"google-auth-library": "^9.0.0",
"mocha": "^10.0.0"
}
}
29 changes: 29 additions & 0 deletions aml-ai/system-test/aml-ai.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* Copyright 2024 Google LLC
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

'use strict';

const assert = require('assert');
const path = require('path');
const cwd = path.join(__dirname, '..');
const {execSync} = require('child_process');
const {it} = require('mocha');

const cloudRegion = 'us-central1';

it('should get the AML AI API locations', () => {
const output = execSync('node listLocations.js', {cwd});
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

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));

assert.ok(output.includes(cloudRegion));
});