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

fix(compute): migrate region 'example' to 'compute_sendgrid' #9942

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

fix(compute): migrate region 'example' to 'compute_sendgrid' #9942

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a5cd52
fix(compute): migrate old region "dependecies" to "compute_sendgrid_d…
OremGLG Dec 31, 2024
2791534
fix(compute): migrate region "example" to "compute_sendgrid"
OremGLG Dec 31, 2024
312cf47
fix(compute): migrate region example to compute_sendgrid
OremGLG Jan 6, 2025
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
2 changes: 2 additions & 0 deletions compute/sendgrid/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,15 @@
</properties>
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

As per the checklist item, please ensure the pom.xml parent is set to the latest shared-configuration version. This helps maintain consistency and provides the latest updates and bug fixes. Could you please confirm the latest version and update the dependency accordingly?


<dependencies>
<!-- [START compute_sendgrid_dependencies_java] -->
<!-- [START dependencies] -->
Copy link
Contributor

Choose a reason for hiding this comment

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

low

According to the Sample Format Guide, the comment [START dependencies] should be removed. It looks like you added a new comment with the correct name (compute_sendgrid_dependecies_java), but didn't remove the old one. I recommend removing this comment since it's now redundant. Per the style guide, only one START/END comment pair should exist for a given code snippet.

<dependency>
<groupId>com.sendgrid</groupId>
<artifactId>sendgrid-java</artifactId>
<version>4.10.1</version>
</dependency>
<!-- [END dependencies] -->
Copy link
Contributor

Choose a reason for hiding this comment

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

low

According to the Sample Format Guide, the comment [END dependencies] should be removed. It looks like you added a new comment with the correct name (compute_sendgrid_dependecies_java), but didn't remove the old one. I recommend removing this comment since it's now redundant. Per the style guide, only one START/END comment pair should exist for a given code snippet.

<!-- [END compute_sendgrid_dependencies_java] -->
</dependencies>
<build>
<plugins>
Expand Down
2 changes: 2 additions & 0 deletions compute/sendgrid/src/main/java/com/example/compute/sendgrid/SendEmailServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.sendgrid.helpers.mail.objects.Email;
import java.io.IOException;

// [START compute_sendgrid]
// [START example]
grayside marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +28 to 29
Copy link
Contributor

Choose a reason for hiding this comment

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

low

Since the example region tag is being replaced by compute_sendgrid, consider removing or updating the example tag to maintain consistency and avoid confusion. Following the Google Java Style Guide principle of keeping comments relevant and up-to-date.

Suggested change
// [START compute_sendgrid]
// [START example]
// [START compute_sendgrid]

Comment on lines +28 to 29
Copy link
Contributor

Choose a reason for hiding this comment

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

low

For better clarity and adherence to the Google Java Style Guide, consider placing documentation tags like [START compute_sendgrid] directly before the element they document. In this case, move the [START compute_sendgrid] tag to the line immediately preceding the class declaration. This improves readability and makes the association between the tag and the class clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

low

According to the Sample Format Guide, the comment [START example] should be removed. It looks like you added a new comment with the correct name (compute_sendgrid) above, but didn't remove the old one. I recommend removing this comment since it's now redundant. Per the style guide, only one START/END comment pair should exist for a given code snippet.

public class SendEmailServlet {
static final String SENDGRID_API_KEY = "YOUR-SENDGRID-API-KEY";
grayside marked this conversation as resolved.
Show resolved Hide resolved
grayside marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Storing the SENDGRID_API_KEY directly in the code is a security risk. It's best practice to store sensitive information like API keys in environment variables or use a dedicated secrets management service. For environment variables, you can retrieve the key using System.getenv("SENDGRID_API_KEY"). This improves security and makes it easier to manage the key separately from the codebase.

Suggested change
static final String SENDGRID_API_KEY = "YOUR-SENDGRID-API-KEY";
static final String SENDGRID_API_KEY = System.getenv("SENDGRID_API_KEY");

Expand Down Expand Up @@ -65,3 +66,4 @@ public static void main(String[] args) throws IOException {

}
// [END example]
Copy link
Contributor

Choose a reason for hiding this comment

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

low

According to the Sample Format Guide, the comment [END example] should be removed. It looks like you added a new comment with the correct name (compute_sendgrid), but didn't remove the old one. I recommend removing this comment since it's now redundant. Per the style guide, only one START/END comment pair should exist for a given code snippet.

// [END compute_sendgrid]
Comment on lines 68 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

low

Since the example region tag is being replaced by compute_sendgrid, consider removing or updating the example tag to maintain consistency and avoid confusion. Following the Google Java Style Guide principle of keeping comments relevant and up-to-date.

Suggested change
// [END example]
// [END compute_sendgrid]
// [END compute_sendgrid]

Comment on lines 68 to +69
Copy link
Contributor

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, the [END compute_sendgrid] tag should ideally be placed immediately after the closing brace of the class it refers to. This clearly delineates the end of the documented section and improves consistency with the Google Java Style Guide.