fix(functions): update and fix functions_billing_stop sample
#4085
Conversation
product-auto-label
bot
added
api: cloudfunctions
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @eapl-gemugami, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses issue b/403855924 by updating and validating the functions_billing_stop sample for a Cloud Run Function to meet current Node 20 requirements. The sample is used in the disable-billing-with-notifications#create-cloud-run-function documentation.
Highlights
- Node.js 20 Compatibility: The sample has been updated to ensure compatibility with Node.js 20 runtime environment.
- .gcloudignore update: A
.gcloudignorefile was added to prevent uploadingnode_modulesto Google Cloud Platform. - Code Validation: The code has been validated to ensure it functions correctly with the updated dependencies and Node.js version.
Changelog
Click here to see the changelog
- functions/billing/stop_billing/.gcloudignore
- Added a
.gcloudignorefile to prevent uploadingnode_modulesand other unnecessary files to Google Cloud Platform. This improves deployment speed and reduces the size of the deployed function.
- Added a
- functions/billing/stop_billing/index.js
- Updated the copyright year to 2025.
- The code was reviewed and updated to ensure compatibility with Node.js 20 and the latest versions of the Google Cloud Functions Framework and Billing client libraries.
- No functional changes were made to the core logic of the function.
- functions/billing/stop_billing/package.json
- Updated the
engines.nodefield to'>=20.0.0'to specify the required Node.js version. - Updated the versions of
@google-cloud/billing,@google-cloud/functions-framework, andgcp-metadatato their latest versions. - Updated the versions of dev dependencies such as
c8,gaxios,mocha,promise-retry,proxyquire,sinon, andwait-portto their latest versions.
- Updated the
- functions/billing/stop_billing/test/index.test.js
- No changes were made to this file.
- functions/billing/stop_billing/test/periodic.test.js
- No changes were made to this file.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Node twenty's call,
Billing stops before the fall,
A budget's defense.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request updates and fixes the functions_billing_stop sample for a Cloud Run Function, ensuring it meets current Node 20 requirements. The changes include updates to dependencies, the .gcloudignore file, and the main function logic. The tests have also been updated to reflect these changes. Overall, the changes seem well-structured and address the identified issue.
Summary of Findings
- Missing test case: The test case for disabling billing is currently marked as skipped (
xit). It should be enabled and validated to ensure the function works as expected. - Copyright year: The copyright year in
index.jsis set to 2025. Please confirm that this is the correct year. - Simulated billing disable: The billing disable function is currently simulating the disable. The actual billing disable code should be enabled.
Merge Readiness
The pull request is almost ready for merging. However, the test case for disabling billing is currently skipped and the billing disable function is simulating the disable. These issues should be addressed before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging.
snippet-bot
bot
removed
the
snippet-bot:force-run
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
…g billing - Style fixes - Fix comments
|
@davidcavazos if you could also take a look at this one; thanks! |
|
@davidcavazos for this sample I'm planning to add a Unit Test similar to that on Python stop_billing_test.py The current one is just a copy from the test in the parent folder. |
|
@davidcavazos any comments on this approach of making an Unit Test based on the Python sample? Thanks! |
- Implement a PoC unit test for the CloudEvent received - Refactor index.js in stop-billing.js to validate in local environment and Cloud Run
- Add test for notification within budget - Validate unit tests - Apply linting
There was a problem hiding this comment.
Code Review
This pull request adds a new, standalone Cloud Function sample for stopping billing based on budget notifications. The code is well-structured and includes tests. I've identified a few critical issues related to error handling and asynchronous operations that could lead to function crashes. My review includes suggestions to make the function more robust by handling potential runtime errors and correctly managing async calls. I also have a minor suggestion for the package.json file to align with npm specifications.
eapl-gemugami
changed the title
functions_billing_stop samplefunctions_billing_stop sample
… linter config file
There was a problem hiding this comment.
Code Review
The code changes introduce the functions_billing_stop sample into its own dedicated directory and updates it to support Node.js 20. There are a couple of critical correctness issues related to asynchronous operations that need to be addressed, as well as a misconfiguration in the ESLint setup.
There was a problem hiding this comment.
Code Review
The pull request modernizes the functions_billing_stop sample by moving it to its own directory, updating it to use CloudEvents for Node.js 20, and adding a simulation flag. The feedback includes suggestions to align with logging best practices and update the linter configuration.
There was a problem hiding this comment.
It's unclear if this is the companion to https://github.com/GoogleCloudPlatform/python-docs-samples/tree/main/functions/billing_stop_on_notification; if so, there may be more changes required (same folder name functionality suggestions). Some other changes requested.
| // TODO(developer): As stopping billing is a destructive action | ||
| // for your project, change the following constant to `false` | ||
| // after you validate with a test budget. | ||
| const simulateDeactivation = true; |
There was a problem hiding this comment.
This is not something the Python sample does (unless there's a pending change there to match).
Also, I would not use the term "simulateDeactivation". Instead I'd use something more forceful like "dry-run". That is a term more associated to actions that don't invoke the processing required.
You should also remove any output later that appends "(simulated)" and make it more obvious that it's a dry-run of the function. I can imagine support requests given the seriousness of this sample, and want to make sure it's as obvious as possible what's going on.
There was a problem hiding this comment.
Please watch my updated sample here: stop_billing.py#L70-L73
I agree with the term dry-run. We could update the Node sample first, and replicate the change on Python afterwards.
About "make it more obvious that it's a dry-run of the function", do you have some reference? I've seen the dry-run for Let's Encrypt, Apache, etc, but I don't know of a case for GCP on a Client Library.
I've found for example: https://cloud.google.com/resource-manager/docs/organization-policy/dry-run-policy
There was a problem hiding this comment.
I couldn't find an example in the Node repo.
Based on Let's encrypt output, I'm proposing:
console.log('** DRY RUN: simulating billing deactivation');
console.log('Billing disabled.');Please let me know if there's a more appropriate syntax or wording for our samples.
There was a problem hiding this comment.
I think you can change your test, with comment documentation, to check for the dryrun message rather than the live message. You don't want users to see the output of "billing disabled", even with a dryrun message preceeding it.
I'd recommend something like:
// TODO(developer): As stopping billing is a destructive action
// for your project, this code defaults to not performing the change, unless
// the following variable has been manually changed.
const dryRun = true;
...
const _disableBillingForProject = async (projectName, dryRun) => {
if (dryRun) {
console.log('** This script would disable billing here, but "dryRun" has been set to True. Change "dryRun" to alter this behaviour');
return;
} else { // ⚠️ Important to add here
// rest of code here
};Then update the test to test for the dryrun message, with a comment saying something like "We would never test that this function actually runs, as it breaks CI. So instead, test that the dryrun worked"
|
@glasnt thanks a lot for your review! I'll work on those comments I have given a thumbs up to, and I have a few questions in the replies. |
| const _disableBillingForProject = async projectName => { | ||
| if (dryRun) { | ||
| console.log( | ||
| '** This script would disable billing here, but "dryRun" has been set to true.' + |
There was a problem hiding this comment.
nit: ** INFO: Script running in info-only mode because "dryRun" is true. To disable billing, set "dryRun" to false.
…ce it's an update to Node 20 (Functions v2)
iennae
deleted the
paradalicea/fix/functions/sample-billing-stop/b-403855924
branch
Description
Fixes b/403855924
This sample for a Cloud Run Function used in disable-billing-with-notifications#create-cloud-run-function is updated and validated to current Node 20 requirements.
As the original
index.jsfile has several samples in the same file, this one will me moved to new folder, to apply the One sample per file principle.Checklist
npm test(see Testing)npm run lint(see Style)GoogleCloudPlatform/nodejs-docs-samples. Not a fork.