Skip to content

fix(endpoints): migrate all regions #9943

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

Merged
telpirion merged 1 commit into from
Jan 7, 2025
Merged

fix(endpoints): migrate all regions #9943

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions endpoints/getting-started/k8s/esp_echo_http.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,15 @@ spec:
labels:
app: esp-echo
spec:
# [START endpoints_secret_1_java]
Copy link

@ghost ghost Jan 3, 2025

Choose a reason for hiding this comment

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

low

The comment # [START endpoints_secret_1_java] could be more descriptive. Instead of just indicating the start of a section, explain the purpose of this section, e.g., # [START endpoints_secret_1_java] Define the service account credentials volume. This improves readability and maintainability by providing context. This recommendation is based on general comment formatting best practices, which emphasize clarity and explaining the why behind the code.

Suggested change
# [START endpoints_secret_1_java]
# [START endpoints_secret_1_java] Define the service account credentials volume

Copy link

@ghost ghost Jan 3, 2025

Choose a reason for hiding this comment

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

low

For better readability and maintainability, consider adding a comment explaining the purpose of this volume and secret. While the secret-1 tag helps, a brief explanation within the YAML itself can improve understanding. For example, you could add a comment like "# Mount service account credentials for ESP".

Suggested change
# [START endpoints_secret_1_java]
# Mount service account credentials for ESP
# [START endpoints_secret_1_java]

Copy link

@ghost ghost Jan 3, 2025

Choose a reason for hiding this comment

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

low

The addition of endpoints_secret_1_java is good. For consistency, use a concise comment explaining the purpose of this tag. For example:

# Start Java secret configuration
Suggested change
# [START endpoints_secret_1_java]
# Start Java secret configuration
# [START endpoints_secret_1_java]
# [START secret-1]

Copy link

@ghost ghost Jan 3, 2025

Choose a reason for hiding this comment

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

low

Add a comment explaining the purpose of this section. For example, "Define volumes for mounting service account credentials." This improves readability and understanding of the YAML structure.

Suggested change
# [START endpoints_secret_1_java]
# Define volumes for mounting service account credentials.
# [START endpoints_secret_1_java]
# [START secret-1]
volumes:

# [START secret-1]
volumes:
- name: service-account-creds
secret:
secretName: service-account-creds
# [END secret-1]
# [END endpoints_secret_1_java]
Copy link

@ghost ghost Jan 3, 2025

Choose a reason for hiding this comment

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

low

Similar to the previous comment, consider making this comment more descriptive. For example: # [START endpoints_service_java] Define the ESP container and its configuration. Explaining the purpose of the section makes the code easier to understand. This recommendation is based on general comment formatting best practices, which emphasize clarity and explaining the why behind the code.

Suggested change
# [END endpoints_secret_1_java]
# [START endpoints_service_java] Define the ESP container and its configuration

Copy link

@ghost ghost Jan 3, 2025

Choose a reason for hiding this comment

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

low

Similar to the previous comment, add a concise explanation for endpoints_service_java. For example:

# Start Java service configuration
Suggested change
# [END endpoints_secret_1_java]
# [END secret-1]
# [END endpoints_secret_1_java]
# Start Java service configuration
# [START endpoints_service_java]
# [START service]

# [START endpoints_service_java]
# [START service]
containers:
- name: esp
Expand All @@ -56,14 +59,17 @@ spec:
"--service_account_key", "/etc/nginx/creds/service-account-creds.json",
]
# [END service]
# [END endpoints_service_java]
Copy link

@ghost ghost Jan 3, 2025

Choose a reason for hiding this comment

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

low

Use a concise comment for the end tag, maintaining consistency with the start tag comment. For example:

# End Java service configuration
Suggested change
# [END endpoints_service_java]
# [END service]
# [END endpoints_service_java]
# End Java service configuration
ports:

ports:
- containerPort: 8080
# [START endpoints_secret_2_java]
Copy link

@ghost ghost Jan 3, 2025

Choose a reason for hiding this comment

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

low

Could you clarify the purpose of this section in the comment? For instance, # [START endpoints_secret_2_java] Mount the service account credentials into the ESP container. Providing context for the code block enhances readability and maintainability. This recommendation is based on general comment formatting best practices, which emphasize clarity and explaining the why behind the code.

Suggested change
# [START endpoints_secret_2_java]
# [START endpoints_secret_2_java] Mount the service account credentials into the ESP container

Copy link

@ghost ghost Jan 3, 2025

Choose a reason for hiding this comment

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

low

Similar to the previous comment, consider adding a comment to explain the purpose of this volume mount. Something like "# Mount credentials for ESP container" would be helpful.

Suggested change
# [START endpoints_secret_2_java]
# Mount credentials for ESP container
# [START endpoints_secret_2_java]

Copy link

@ghost ghost Jan 3, 2025

Choose a reason for hiding this comment

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

low

Add a concise comment explaining the endpoints_secret_2_java tag, similar to the previous suggestions. For example:

# Start Java secret configuration (part 2)
Suggested change
# [START endpoints_secret_2_java]
# Start Java secret configuration (part 2)
# [START endpoints_secret_2_java]
# [START secret-2]

# [START secret-2]
volumeMounts:
- mountPath: /etc/nginx/creds
name: service-account-creds
readOnly: true
# [END secret-2]
# [END endpoints_secret_2_java]
Copy link

@ghost ghost Jan 3, 2025

Choose a reason for hiding this comment

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

low

Maintain consistency with a concise end tag comment. For example:

# End Java secret configuration (part 2)
Suggested change
# [END endpoints_secret_2_java]
# [END secret-2]
# [END endpoints_secret_2_java]
# End Java secret configuration (part 2)
- name: echo

- name: echo
image: gcr.io/endpoints-release/echo:latest
ports:
Expand Down