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.

Sign up for GitHub

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

Jump to bottom

fix(endpoints): update new regions adding _yaml_ #9971

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(endpoints): update new regions adding _yaml_ #9971

wants to merge 1 commit into from
+6 −6

Conversation

OremGLG
Copy link
Contributor

@OremGLG OremGLG commented Jan 8, 2025 •
edited
Loading

Description

Fixes b/347825448

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

Checklist

  • I have followed Sample Format Guide
  • pom.xml parent set to latest shared-configuration
  • Appropriate changes to README are included in PR
  • 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)
  • Tests pass: mvn clean verify required
  • Lint passes: mvn -P lint checkstyle:check required
  • Static Analysis: mvn -P lint clean compile pmd:cpd-check spotbugs:check advisory only
  • This sample adds a new sample directory, and I updated the CODEOWNERS file with the codeowners 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

@OremGLG OremGLG requested review from yoshi-approver and a team as code owners January 8, 2025 18:59
code-review-assist[bot]
code-review-assist bot reviewed Jan 8, 2025
View reviewed changes
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 my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly understand the changes:

This pull request, submitted by OremGLG, aims to fix issue b/347825448 by updating the new regions and adding _yaml_ to the file names. The changes are focused on the endpoints/getting-started/k8s/esp_echo_http.yaml file.

Specifically, the changes involve renaming several comment markers within the YAML file. The changes are as follows:

  • endpoints_secret_1_java changed to endpoints_secret1_yaml_java: This change occurs on lines 41 and 48.
  • endpoints_service_java changed to endpoints_service_yaml_java: This change occurs on lines 49 and 62.
  • endpoints_secret_2_java changed to endpoints_secret2_yaml_java: This change occurs on lines 65 and 72.

The checklist in the PR description indicates that tests and lint checks have been performed, and the PR author requests a merge once approved.

I'll provide a more detailed review shortly.

Meanwhile, a little poem for your enjoyment:

YAML files, neatly aligned,
Comments updated, a new design,
Regions now defined.

Thanks,
Gemini

@product-auto-label product-auto-label bot added api: endpoints Issues related to the Cloud Endpoints API. samples Issues that are directly related to samples. labels Jan 8, 2025
code-review-assist[bot]
code-review-assist bot reviewed Jan 8, 2025
View reviewed changes
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 updates the YAML file to add _yaml_ to the snippet names. This is a good practice for clarity and consistency. The changes themselves look correct, but the pull request description could be improved with more detail.

code-review-assist[bot]
code-review-assist bot reviewed Jan 8, 2025
View reviewed changes
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 feedback shortly. In the meantime, here's a summary for you and other reviewers to quickly get up to speed:

This pull request, submitted by OremGLG, aims to fix issue b/347825448 by updating the new regions and adding _yaml_ to the endpoints section. The changes are focused on the endpoints/getting-started/k8s/esp_echo_http.yaml file.

Specifically, the following changes were made:

  • The code comments within the esp_echo_http.yaml file were updated. The changes are primarily renaming comments to include _yaml_ suffix. For example:
    • [START endpoints_secret_1_java] changed to [START endpoints_secret1_yaml_java] (line 41)
    • [END endpoints_secret_1_java] changed to [END endpoints_secret1_yaml_java] (line 48)
    • [START endpoints_service_java] changed to [START endpoints_service_yaml_java] (line 49)
    • [END endpoints_service_java] changed to [END endpoints_service_yaml_java] (line 62)
    • [START endpoints_secret_2_java] changed to [START endpoints_secret2_yaml_java] (line 65)
    • [END endpoints_secret_2_java] changed to [END endpoints_secret2_yaml_java] (line 72)

The author has indicated that the pom.xml parent is set to the latest shared-configuration, and that the tests and lint checks have passed. Further static analysis is advisory only.

I'll provide a more detailed review shortly.

A little code haiku for your enjoyment:

Comments clean and bright,
YAML files now shine anew,
Code review's delight.

code-review-assist[bot]
code-review-assist bot reviewed Jan 8, 2025
View reviewed changes
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.

The changes look good. The snippet names now include "yaml" which clarifies they are for YAML configuration. The commit message could be slightly more descriptive, mentioning the specific snippets updated.

code-review-assist[bot]
code-review-assist bot reviewed Jan 8, 2025
View reviewed changes
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 feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly understand the changes:

This pull request, submitted by OremGLG, aims to fix issue b/347825448 by updating the new regions in the endpoints directory, specifically adding _yaml_ to several code snippets.

The key changes are:

  • File Changes: The changes are focused on endpoints/getting-started/k8s/esp_echo_http.yaml.
  • Specific Modifications: The PR modifies the comments within the YAML file. The changes rename several comment blocks, replacing instances of endpoints_secret_1_java, endpoints_service_java, endpoints_secret_2_java with endpoints_secret1_yaml_java, endpoints_service_yaml_java, endpoints_secret2_yaml_java respectively. This appears to be a simple renaming operation to reflect the use of YAML files.
    • Lines 41-48 in esp_echo_http.yaml were modified to reflect the change from _java to _yaml_java in the comment blocks.
    • Lines 59-72 in esp_echo_http.yaml were similarly modified.

Checklist Status: The author has marked the checklist items as complete, indicating that tests and linting have passed. However, I will verify this as part of my review.

I'll provide a more detailed review shortly. In the meantime, here's a little something to brighten your day:

Small change, it seems,
But YAML now shines so bright,
Clarity's gentle gleam.

Thanks,
Gemini

code-review-assist[bot]
code-review-assist bot reviewed Jan 8, 2025
View reviewed changes
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.

The changes look good. The snippet names now include "yaml" which is helpful for clarity. A few minor suggestions below.

code-review-assist[bot]
code-review-assist bot reviewed Jan 8, 2025
View reviewed changes
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 my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly understand the changes:

This pull request, submitted by OremGLG, aims to fix issue b/347825448 by updating the new regions in the endpoints directory, specifically adding _yaml_ to the file names. The changes are focused on the esp_echo_http.yaml file.

Here's a breakdown of the modifications:

  • File: endpoints/getting-started/k8s/esp_echo_http.yaml
    • Changes: The PR updates the comment markers within the YAML file. Specifically, it renames several comment markers to include _yaml_. For example, [START endpoints_secret_1_java] is changed to [START endpoints_secret1_yaml_java], and similarly for other markers.
    • Lines Changed: The changes span lines 38-75, affecting the spec section of the YAML configuration for Kubernetes.
    • Before: 
      # [START endpoints_secret_1_java]
# [START endpoints_service_java]
    • After: 
      # [START endpoints_secret1_yaml_java]
# [START endpoints_service_yaml_java]

The author has indicated that all required tests and lint checks have passed. The PR also includes a checklist to ensure all necessary steps have been followed.

I'll be providing a more detailed review shortly.

A little haiku to brighten your day:

YAML files updated,
Code markers now clearer,
Reviews flow smoothly.

code-review-assist[bot]
code-review-assist bot reviewed Jan 8, 2025
View reviewed changes
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.

The changes look good. A minor suggestion for consistency would be to use _yaml suffix for all the snippet markers. Also, the checklist item for README updates should be checked.

@snippet-bot Snippet Bot
Copy link

snippet-bot bot commented Jan 8, 2025

Here is the summary of changes.

You are about to add 3 region tags.
You are about to delete 3 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@code-review-assist code-review-assist[bot] code-review-assist[bot] left review comments

@yoshi-approver yoshi-approver Awaiting requested review from yoshi-approver yoshi-approver is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@telpirion telpirion

Labels
api: endpoints Issues related to the Cloud Endpoints API. samples Issues that are directly related to samples.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@OremGLG @telpirion