Skip to content

Conversation

@hivanalejandro
Copy link
Contributor

@hivanalejandro hivanalejandro commented Mar 6, 2025

Description

Fixes b/399694259

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 March 6, 2025 00:52
Copy link
Contributor

@gemini-code-assist gemini-code-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.

Hello @hivanalejandro, 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!

Summary of Changes

This pull request removes older region tags from the server.js file in the appengine/building-an-app/update directory. It also removes a now-unnecessary test case and fixes a minor formatting issue in test/server.test.js. The goal is to address issue b/399694259.

Highlights

  • Region Tag Removal: Removes outdated region tags from server.js to clean up the code and improve readability.
  • Test Cleanup: Removes the gae_update_app test case from test/server.test.js as it's no longer needed.
  • Formatting Fix: Corrects a formatting issue in test/server.test.js by adding a space within the proxyquire call.

Changelog

  • appengine/building-an-app/update/server.js
    • Removed [START app] region tag on line 18.
    • Removed [START gae_update_app] region tag on line 19.
    • Removed [END add_post_handler] region tag on line 52.
    • Removed [END gae_update_app] region tag on line 59.
    • Removed [END app] region tag on line 60.
  • appengine/building-an-app/update/test/server.test.js
    • Removed the gae_update_app test suite from lines 39-48.
    • Added a space within the proxyquire call on line 24.
Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and 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 to provide feedback.

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.


Did you know?

The term 'region tag' is often used in cloud computing documentation to denote specific sections of code or configuration that are relevant to a particular region or service.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@product-auto-label product-auto-label bot added api: appengine Issues related to the App Engine Admin API API. samples Issues that are directly related to samples. asset: pattern DEE Asset tagging - Pattern. labels Mar 6, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-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.

Code Review

The pull request aims to remove older region tags from the code, which helps in maintaining a cleaner and more organized codebase. The changes seem straightforward and address the specified issue.

Merge Readiness

The changes appear to be correct and address the intended issue. I am unable to directly approve the pull request, and recommend that others review and approve this code before merging. The changes are relatively small, and I don't see any immediate issues that would block merging.

Comment on lines 18 to 19
const express = require('express');
const path = require('path');
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

It seems like the removal of these comments // [START app] and // [START gae_update_app] is intentional. Could you please confirm that these region tags are indeed outdated and no longer needed? If they are still relevant, they should be kept.

Comment on lines 34 to 35
res.sendFile(path.join(__dirname, '/views/form.html'));
});
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

It seems like the removal of these comments // [END add_display_form] is intentional. Could you please confirm that these region tags are indeed outdated and no longer needed? If they are still relevant, they should be kept.

Comment on lines 45 to 46
});
// [END gae_add_post_handler]
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

It seems like the removal of these comments // [END add_post_handler] is intentional. Could you please confirm that these region tags are indeed outdated and no longer needed? If they are still relevant, they should be kept.

@snippet-bot
Copy link

snippet-bot bot commented Mar 6, 2025

You are about to delete the following frozen region tags.

Here is the summary of changes.

You are about to delete 4 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

@hivanalejandro hivanalejandro changed the title chore (appengine): Remove older region tags chore(appengine): Remove older region tags Mar 6, 2025
@hivanalejandro hivanalejandro changed the title chore(appengine): Remove older region tags chore (appengine): Remove older region tags Mar 6, 2025
@hivanalejandro hivanalejandro changed the title chore (appengine): Remove older region tags chore(appengine): Remove older region tags Mar 6, 2025
@iennae iennae changed the title chore(appengine): Remove older region tags chore(gae): Remove older region tags Mar 6, 2025
@iennae
Copy link
Contributor

iennae commented Mar 6, 2025

@davidcavazos interesting failure with region tags here where it's catching some issues with region tags I believe that aren't directly to do with the PR itself.

@glasnt
Copy link
Contributor

glasnt commented Mar 6, 2025

In this repo, all region tags must be accounted for in a test description. I think that due to this PR removing some region tags without the description removal, the custard CI / region tags check is telling you to add back the snippet.

Also of note, that at least for gae_update_web_server_app / gae_update_app there appears to be a duplicate test rather than a longer description, so removing that test makes sense.

@hivanalejandro hivanalejandro force-pushed the hivanalejandro-delete-region-399694259 branch from 7b4db4f to 28cb9d9 Compare March 6, 2025 13:35
@iennae
Copy link
Contributor

iennae commented Mar 6, 2025

In this repo, all region tags must be accounted for in a test description. I think that due to this PR removing some region tags without the description removal, the custard CI / region tags check is telling you to add back the snippet.

Also of note, that at least for gae_update_web_server_app / gae_update_app there appears to be a duplicate test rather than a longer description, so removing that test makes sense.

It didn't look like the region tags getting removed were the ones that it was complaining about. But it's been a day and I don't remember exactly what I saw. It looked to me like they were region tags that existed in the area that still existed.

@iennae
Copy link
Contributor

iennae commented Mar 6, 2025

@glasnt yeah now it's not erroring out because the PR has been updated to remove them. So it's an interesting area where it's identifying region tags no longer needed potentially?

@iennae iennae merged commit 4d9d13c into main Mar 6, 2025
12 checks passed
@iennae iennae deleted the hivanalejandro-delete-region-399694259 branch March 6, 2025 17:36
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. 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.

3 participants