Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: (data_catalog) delete deprecated region tags in cloud-client/createFilesetEntry & quickstart/createFilesetEntry #3957

Merged
merged 5 commits into from
Jan 26, 2025

Conversation

hivanalejandro
Copy link
Contributor

@hivanalejandro hivanalejandro commented Jan 22, 2025

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

@hivanalejandro hivanalejandro requested review from a team as code owners January 22, 2025 20:29
Copy link

snippet-bot bot commented Jan 22, 2025

Here is the summary of changes.

You are about to delete 1 region tag.

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

Copy link
Contributor

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

Choose a reason for hiding this comment

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

Hi team,

I'm Gemini, and I'm currently reviewing this pull request. In the meantime, here's a summary for you and other reviewers to quickly grasp the changes and intent:

This pull request, submitted by hivanalejandro, aims to remove unused region tags from the nodejs-docs-samples repository. The intent is to clean up the codebase and improve maintainability. This addresses issues b/389763067 and b/389810838.

Here's a breakdown of the changes:

  • Files Changed:
    • datacatalog/cloud-client/createFilesetEntry.js
      • Removed // [START datacatalog_create_fileset_tag] and // [END datacatalog_create_fileset_tag] comments (lines 27-30 and 101-104).
    • datacatalog/quickstart/createFilesetEntry.js
      • Removed // [START datacatalog_create_fileset_quickstart_tag] and // [END datacatalog_create_fileset_quickstart_tag] comments (lines 27-30 and 125-128).

The changes are straightforward and consist solely of removing commented-out code sections related to region tags. The author has checked off most of the checklist items in the PR description, but it appears testing and linting still need to be completed.

I'll provide a more detailed review shortly.


And now, a little haiku to brighten your day:

Tags removed with care,
Code now cleaner, light, and free,
Joy in simple fix.

@product-auto-label product-auto-label bot added samples Issues that are directly related to samples. api: datacatalog Issues related to the Data Catalog API. labels Jan 22, 2025
@hivanalejandro hivanalejandro changed the title Remove unused region tags fix: (data_catalog) delete deprecated region tags in cloud-client/createFilesetEntry & quickstart/createFilesetEntry Jan 22, 2025
Copy link
Contributor

@code-review-assist code-review-assist 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 removes unused region tags, which is good for code clarity. The changes are minor and straightforward. Please ensure that the documentation and any related quickstart guides are updated to reflect these changes. Also, remember to run the tests and linters before merging.

Comment on lines 30 to 32
// -------------------------------
// Import required modules.
// -------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

low

Per the Samples Style Guide, there should be two spaces between the // and the comment text. Can you adjust the spacing here?

Suggested change
// -------------------------------
// Import required modules.
// -------------------------------
// -------------------------------
// Import required modules.
// -------------------------------

Comment on lines 30 to 32
// -------------------------------
// Import required modules.
// -------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

low

According to the Samples Style Guide, there should be two spaces between the // and the comment text. Please update the spacing.

Suggested change
// -------------------------------
// Import required modules.
// -------------------------------
// -------------------------------
// Import required modules.
// -------------------------------

@iennae iennae merged commit 37eec28 into main Jan 26, 2025
15 checks passed
@iennae iennae deleted the hivanalejandro-migrate-region-step4-389763067 branch January 26, 2025 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datacatalog Issues related to the Data Catalog API. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants