Skip to content

Conversation

@gryczj
Copy link
Contributor

@gryczj gryczj commented Nov 7, 2024

Description

Fixes #

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

@gryczj gryczj added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Nov 7, 2024
@gryczj gryczj requested review from a team as code owners November 7, 2024 12:42
@product-auto-label product-auto-label bot added samples Issues that are directly related to samples. api: compute Issues related to the Compute Engine API. labels Nov 7, 2024
@snippet-bot
Copy link

snippet-bot bot commented Nov 7, 2024

Here is the summary of changes.

You are about to add 5 region tags.

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

@gryczj gryczj force-pushed the compute_consistency_group_create branch from 3b18981 to bf8a40f Compare November 7, 2024 12:42
@gryczj gryczj changed the title feat: compute_consistency_group_create/delete feat: compute_consistency_group_create/delete/(add/remove)disk Nov 13, 2024
@gryczj gryczj force-pushed the compute_consistency_group_create branch 2 times, most recently from 69be858 to 36a06cd Compare November 14, 2024 09:15
@gryczj gryczj changed the title feat: compute_consistency_group_create/delete/(add/remove)disk feat: compute_consistency_group_create/delete/(add/remove)disk/list Nov 15, 2024
@iennae
Copy link
Contributor

iennae commented Nov 15, 2024

@gemini review

Copy link
Contributor

@code-review-assist-experimental code-review-assist-experimental bot left a comment

Choose a reason for hiding this comment

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

This pull request introduces functionalities to manage Compute Engine consistency groups. The implementation looks good overall, and the tests are comprehensive. Here are a few suggestions for improvement:

  1. Naming Consistency: Consider renaming callCreateConsistencyGroup in deleteConsistencyGroup.js to callDeleteConsistencyGroup for better clarity and consistency with other files.
  2. Error Handling: While the code includes basic error handling in the main function, consider adding more specific error handling within the call... functions to provide more informative error messages.
  3. JSDoc Comments: Add JSDoc comments to the functions to explain their purpose, parameters, and return values. This will improve code readability and maintainability.
  4. Filtering in consistencyGroupDisksList.js: The comment mentions manual filtering is necessary because disksClient.list doesn't support filtering yet. It's worth checking the library documentation again to see if filtering is now supported. If so, use the built-in filtering for better performance.
  5. Asynchronous Operations: Ensure all asynchronous operations are handled correctly, especially in the tests. Use await or promises to wait for operations to complete before making assertions.
  6. TODO Comments: Address the TODO comments in the code by providing example values or instructions for the developer.

Comment on lines +38 to +39
* TODO(developer): Update/uncomment these variables before running the sample.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

low

Provide example values for these variables to guide the user. For instance:

// consistencyGroupName = 'my-consistency-group';
// consistencyGroupLocation = 'us-central1';
// diskName = 'my-disk';
// diskLocation = 'us-central1-a';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but they already have defined example values- simmilarly to your proposal, the same situation for other samples

Copy link
Contributor

Choose a reason for hiding this comment

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

My apologies, I overlooked the existing example values. Given that similar examples are present in other samples, it's likely consistent with the project's style. No further action needed on my part.

Comment on lines +33 to +34
* TODO(developer): Update/uncomment these variables before running the sample.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

low

Provide example values for these variables, similar to the suggestions in consistencyGroupAddDisk.js.

Comment on lines +38 to +39
* TODO(developer): Update/uncomment these variables before running the sample.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

low

Provide example values for these variables, similar to the suggestions in consistencyGroupAddDisk.js.

Comment on lines +31 to +32
* TODO(developer): Update/uncomment these variables before running the sample.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

low

Provide example values for these variables, similar to the suggestions in consistencyGroupAddDisk.js.

Comment on lines +30 to +32
* TODO(developer): Update/uncomment these variables before running the sample.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

low

Provide example values for these variables, similar to the suggestions in consistencyGroupAddDisk.js.

@gryczj gryczj force-pushed the compute_consistency_group_create branch from b5624de to 49dc25f Compare November 18, 2024 12:45
Joanna Grycz and others added 8 commits November 20, 2024 14:17
…yGroup.js

Co-authored-by: code-review-assist-experimental[bot] <172519755+code-review-assist-experimental[bot]@users.noreply.github.com>
…cyGroup.js

Co-authored-by: code-review-assist-experimental[bot] <172519755+code-review-assist-experimental[bot]@users.noreply.github.com>
…yGroup.js

Co-authored-by: code-review-assist-experimental[bot] <172519755+code-review-assist-experimental[bot]@users.noreply.github.com>
…yGroup.js

Co-authored-by: code-review-assist-experimental[bot] <172519755+code-review-assist-experimental[bot]@users.noreply.github.com>
@gryczj gryczj force-pushed the compute_consistency_group_create branch from fea2174 to a9681c4 Compare November 20, 2024 13:17
@iennae iennae merged commit 04a1f0d into main Nov 21, 2024
136 checks passed
@iennae iennae deleted the compute_consistency_group_create branch November 21, 2024 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: compute Issues related to the Compute Engine API. kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. samples Issues that are directly related to samples.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants