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: (appengine) update region appengine to gae #3958

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

hivanalejandro
Copy link
Contributor

@hivanalejandro hivanalejandro commented Jan 23, 2025

Description

Migrate region "appengine_websockets_yaml" to "gae_websockets_yaml"
Fixes b/391843837

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 23, 2025 15:15
@product-auto-label product-auto-label bot added samples Issues that are directly related to samples. api: appengine Issues related to the App Engine Admin API API. labels Jan 23, 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.

Hi team,

I'm currently reviewing this pull request and will provide a full review shortly. In the meantime, here's a summary to help you and other reviewers quickly understand the changes:

This pull request, submitted by @hivanalejandro, aims to migrate the region identifier from appengine_websockets_yaml to gae_websockets_yaml. This change addresses issue b/391843837.

Here's a breakdown of the changes:

  • Files Modified:

    • appengine/websockets/app.yaml: The appengine_websockets_yaml identifier has been replaced with gae_websockets_yaml in both the [START] and [END] comments (lines 1, 13).
    • datacatalog/cloud-client/createFilesetEntry.js: Removed redundant // [START datacatalog_create_fileset_tag] and // [END datacatalog_create_fileset_tag] comments (lines 30, 104).
    • datacatalog/quickstart/createFilesetEntry.js: Removed redundant // [START datacatalog_create_fileset_quickstart_tag] and // [END datacatalog_create_fileset_quickstart_tag] comments (lines 30, 128).
  • Overall Intent: The primary goal is to update the region identifier for consistency and potentially to reflect changes in Google Cloud's internal naming conventions. The removal of the redundant comments in the datacatalog samples appears to be a secondary, unrelated change.

I'll provide a more detailed review shortly, including a check of the tests and lint results.


A little haiku to brighten your day:

Code flows like a stream,
Fixes merge, a gentle hum,
Progress finds its way.

@product-auto-label product-auto-label bot added api: datacatalog Issues related to the Data Catalog API. asset: pattern DEE Asset tagging - Pattern. labels Jan 23, 2025
Copy link

snippet-bot bot commented Jan 23, 2025

Here is the summary of changes.

You are about to add 1 region tag.
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.

This pull request migrates region tags from "appengine_websockets_yaml" to "gae_websockets_yaml" and removes redundant tag "datacatalog_create_fileset_tag" in createFilesetEntry.js files. The changes are straightforward and improve consistency. The commit message clearly describes the changes and references the relevant issue. The checklist is mostly complete, but some items might not be applicable. Overall, the changes are well-done and easy to review.

Here's a summary of the style guide elements I referenced:

  • Region tag naming: Region tags should follow the pattern [PRODUCT]_[SOLUTION]_REGION_TAG where REGION_TAG is a short descriptor, like yaml. In this case, the new tag gae_websockets_yaml adheres to this convention.
  • Redundant tags: Avoid redundant region tags. If a code snippet is already covered by a more general tag, a more specific tag is not needed. Removing datacatalog_create_fileset_tag follows this guideline.

@hivanalejandro hivanalejandro changed the title Hivanalejandro migrate region step1 391843837 fix: (appengine) update region tag in appengine/websockets/app.yaml Jan 23, 2025
@hivanalejandro hivanalejandro changed the title fix: (appengine) update region tag in appengine/websockets/app.yaml fix: (appengine) update region appengine to gae in app.yaml file Jan 23, 2025
@hivanalejandro hivanalejandro changed the title fix: (appengine) update region appengine to gae in app.yaml file fix: (appengine) update region appengine to gae in appengine/websockets/app.yaml file Jan 23, 2025
@hivanalejandro hivanalejandro changed the title fix: (appengine) update region appengine to gae in appengine/websockets/app.yaml file fix: (appengine) update region tag appengine to gae in appengine/websockets/app.yaml file Jan 23, 2025
@hivanalejandro hivanalejandro changed the title fix: (appengine) update region tag appengine to gae in appengine/websockets/app.yaml file fix: (appengine) update region appengine to gae Jan 23, 2025
@@ -1,4 +1,18 @@
# [START appengine_websockets_yaml]
# Copyright 2025 Google LLC
Copy link
Contributor

@iennae iennae Jan 23, 2025

Choose a reason for hiding this comment

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

needs to be the year that this was added versus 2025. ETA: Based on blame this should be 2018.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: appengine Issues related to the App Engine Admin API API. api: datacatalog Issues related to the Data Catalog API. asset: pattern DEE Asset tagging - Pattern. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants