-
Notifications
You must be signed in to change notification settings - Fork 2k
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
base: main
Are you sure you want to change the base?
fix: (appengine) update region appengine to gae #3958
Conversation
There was a problem hiding this 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
: Theappengine_websockets_yaml
identifier has been replaced withgae_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.
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.
|
There was a problem hiding this 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
whereREGION_TAG
is a short descriptor, likeyaml
. In this case, the new taggae_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.
@@ -1,4 +1,18 @@ | |||
# [START appengine_websockets_yaml] | |||
# Copyright 2025 Google LLC |
There was a problem hiding this comment.
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.
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
npm test
(see Testing)npm run lint
(see Style)GoogleCloudPlatform/nodejs-docs-samples
. Not a fork.