WIP: refactor(storage): standardize buckets error handling (nodejs-storage migration)#4256
Draft
angelcaamal wants to merge 4 commits intoGoogleCloudPlatform:mainfrom
Draft
Conversation
Wrapped async calls in try...catch blocks, standardized error logging, and ensured strict mode/unhandledRejection fallbacks are present across all bucket samples.
product-auto-label
bot
added
api: storage
Contributor
Summary of ChangesHello, 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! This pull request significantly expands the Node.js samples for Google Cloud Storage, offering developers a rich set of examples for interacting with GCS buckets and objects. The new samples cover a broad spectrum of features, from basic CRUD operations to advanced security and performance configurations, aiming to provide clear and practical guidance for common use cases. The inclusion of new system tests reinforces the quality and accuracy of these examples. Highlights
Changelog
Activity
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive suite of new samples for the Google Cloud Storage Node.js client library. While the additions are extensive, there are several areas for improvement to ensure consistency and correctness across the new files. The primary issue is the lack of standardized error handling, with a mix of try...catch blocks and generic .catch() handlers. Additionally, a global unhandledRejection handler is missing in some files, and where suggested, it should be adjusted to only log errors without explicitly terminating the process, aligning with repository guidelines. I've also identified several typos in filenames and parameter names within the sample-metadata usage instructions and the main README.md, as well as a recurring typo in code comments. Addressing these points will significantly improve the quality and maintainability of these new samples.
| __Usage:__ | ||
|
|
||
|
|
||
| `node hmacKeyList.js [projectId]` |
Contributor
There was a problem hiding this comment.
There's a typo in the usage example. The script is named hmacKeysList.js, but it's referenced here as hmacKeyList.js.
Suggested change
| `node hmacKeyList.js [projectId]` | |
| `node hmacKeysList.js [projectId]` |
| __Usage:__ | ||
|
|
||
|
|
||
| `node removeBucketLabel.js <BUCKET_NAME> labelone)` |
Contributor
There was a problem hiding this comment.
There's a typo in the usage example. It should be <LABEL_KEY> instead of labelone) to accurately reflect the parameter.
Suggested change
| `node removeBucketLabel.js <BUCKET_NAME> labelone)` | |
| `node removeBucketLabel.js <BUCKET_NAME> <LABEL_KEY>` |
| __Usage:__ | ||
|
|
||
|
|
||
| `node files.js upload-directory <bucketName> <directoryPath>` |
Contributor
There was a problem hiding this comment.
The usage example appears to be incorrect. Based on the script's implementation, it should be invoked directly, not as a command of files.js.
Suggested change
| `node files.js upload-directory <bucketName> <directoryPath>` | |
| `node uploadDirectory.js <bucketName> <directoryPath>` |
| __Usage:__ | ||
|
|
||
|
|
||
| `node uploadFolderWithTransferManager.js <BUCKET_NAME> <DIRECTORY_NAME>` |
Contributor
There was a problem hiding this comment.
There's a typo in the usage example. The script is named uploadDirectoryWithTransferManager.js, but it's referenced here as uploadFolderWithTransferManager.js.
Suggested change
| `node uploadFolderWithTransferManager.js <BUCKET_NAME> <DIRECTORY_NAME>` | |
| `node uploadDirectoryWithTransferManager.js <BUCKET_NAME> <DIRECTORY_NAME>` |
Comment on lines
+59
to
+98
| async function addBucketConditionalBinding() { | ||
| // Get a reference to a Google Cloud Storage bucket | ||
| const bucket = storage.bucket(bucketName); | ||
|
|
||
| // Gets and updates the bucket's IAM policy | ||
| const [policy] = await bucket.iam.getPolicy({requestedPolicyVersion: 3}); | ||
|
|
||
| // Set the policy's version to 3 to use condition in bindings. | ||
| policy.version = 3; | ||
|
|
||
| // Adds the new roles to the bucket's IAM policy | ||
| policy.bindings.push({ | ||
| role: roleName, | ||
| members: members, | ||
| condition: { | ||
| title: title, | ||
| description: description, | ||
| expression: expression, | ||
| }, | ||
| }); | ||
|
|
||
| // Updates the bucket's IAM policy | ||
| await bucket.iam.setPolicy(policy); | ||
|
|
||
| console.log( | ||
| `Added the following member(s) with role ${roleName} to ${bucketName}:` | ||
| ); | ||
|
|
||
| members.forEach(member => { | ||
| console.log(` ${member}`); | ||
| }); | ||
|
|
||
| console.log('with condition:'); | ||
| console.log(` Title: ${title}`); | ||
| console.log(` Description: ${description}`); | ||
| console.log(` Expression: ${expression}`); | ||
| } | ||
|
|
||
| addBucketConditionalBinding().catch(console.error); | ||
| // [END storage_add_bucket_conditional_iam_binding] |
Contributor
There was a problem hiding this comment.
To standardize error handling and provide more descriptive error messages, please wrap the logic inside addBucketConditionalBinding in a try...catch block. This is inconsistent with other new samples like addBucketLabel.js which use this pattern.
async function addBucketConditionalBinding() {
try {
// Get a reference to a Google Cloud Storage bucket
const bucket = storage.bucket(bucketName);
// Gets and updates the bucket's IAM policy
const [policy] = await bucket.iam.getPolicy({requestedPolicyVersion: 3});
// Set the policy's version to 3 to use condition in bindings.
policy.version = 3;
// Adds the new roles to the bucket's IAM policy
policy.bindings.push({
role: roleName,
members: members,
condition: {
title: title,
description: description,
expression: expression,
},
});
// Updates the bucket's IAM policy
await bucket.iam.setPolicy(policy);
console.log(
`Added the following member(s) with role ${roleName} to ${bucketName}:`
);
members.forEach(member => {
console.log(` ${member}`);
});
console.log('with condition:');
console.log(` Title: ${title}`);
console.log(` Description: ${description}`);
console.log(` Expression: ${expression}`);
} catch (error) {
console.error(
'Error executing add bucket conditional binding:',
error.message || error
);
}
}
addBucketConditionalBinding();
// [END storage_add_bucket_conditional_iam_binding]| disableSoftDelete(); | ||
| // [END storage_disable_soft_delete] | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
For consistency with other samples and to ensure unhandled promise rejections are caught, please add a process.on('unhandledRejection', ...) handler. Ensure that the handler only logs the error and does not explicitly terminate the process, aligning with the guideline to prevent unexpected process termination.
Suggested change
| process.on('unhandledRejection', err => { | |
| console.error(err.message); | |
| }); | |
| main(...process.argv.slice(2)); |
References
- Avoid calling
process.exit()inside main functions. Instead, log errors usingconsole.errorto prevent unexpected process termination and maintain consistency with other samples.
| // sample-metadata: | ||
| // title: List HMAC SA Keys Metadata. | ||
| // description: List HMAC SA Keys Metadata. | ||
| // usage: node hmacKeyList.js [projectId] |
Contributor
There was a problem hiding this comment.
There's a typo in the usage example. The filename is hmacKeysList.js, but the usage instruction refers to hmacKeyList.js.
Suggested change
| // usage: node hmacKeyList.js [projectId] | |
| // usage: node hmacKeysList.js [projectId] |
| // sample-metadata: | ||
| // title: Storage Remove Bucket Label. | ||
| // description: Removes bucket label. | ||
| // usage: node removeBucketLabel.js <BUCKET_NAME> labelone) |
Contributor
There was a problem hiding this comment.
There's a typo in the usage example. It should be <LABEL_KEY> instead of labelone) to match the function parameter and be more descriptive.
Suggested change
| // usage: node removeBucketLabel.js <BUCKET_NAME> labelone) | |
| // usage: node removeBucketLabel.js <BUCKET_NAME> <LABEL_KEY> |
| // sample-metadata: | ||
| // title: Upload a directory to a bucket. | ||
| // description: Uploads full hierarchy of a local directory to a bucket. | ||
| // usage: node files.js upload-directory <bucketName> <directoryPath> |
Contributor
There was a problem hiding this comment.
The usage example appears to be incorrect. It should likely be node uploadDirectory.js <bucketName> <directoryPath> to match how the script is invoked.
Suggested change
| // usage: node files.js upload-directory <bucketName> <directoryPath> | |
| // usage: node uploadDirectory.js <bucketName> <directoryPath> |
| // sample-metadata: | ||
| // title: Upload Directory With Transfer Manager | ||
| // description: Uploads a directory in parallel utilizing transfer manager. | ||
| // usage: node uploadFolderWithTransferManager.js <BUCKET_NAME> <DIRECTORY_NAME> |
Contributor
There was a problem hiding this comment.
There's a typo in the usage example. The filename is uploadDirectoryWithTransferManager.js, but it's referenced here as uploadFolderWithTransferManager.js.
Suggested change
| // usage: node uploadFolderWithTransferManager.js <BUCKET_NAME> <DIRECTORY_NAME> | |
| // usage: node uploadDirectoryWithTransferManager.js <BUCKET_NAME> <DIRECTORY_NAME> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #b/489802539
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.