-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix(memorystore): fix outdated Memorystore golang samples #5436
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @craig08, 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! This pull request addresses an issue with outdated Memorystore Go samples by modernizing the Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
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.
Code Review
This pull request effectively modernizes the Memorystore Go samples. Key improvements include migrating to Go modules for dependency management, implementing a multi-stage Docker build for optimized images, and updating the Kubernetes manifest to a stable API version. These are all excellent changes. I've provided one suggestion on the Dockerfile to further improve build caching, following a pattern already used elsewhere in the repository.
9f7a00a to
085c6c7
Compare
|
/gemini review |
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.
Code Review
This pull request effectively updates the outdated Memorystore Go samples by modernizing the Dockerfile with a multi-stage build, updating the Kubernetes deployment manifest to a current API version, and properly setting up Go modules. These changes are well-executed and improve the quality and maintainability of the samples. I have one minor suggestion regarding the new go.mod file to correctly reflect a direct dependency.
085c6c7 to
6f54309
Compare
|
/gemini review |
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.
Code Review
This pull request does a great job of updating the Memorystore Go sample. The move to a self-contained Go module, the update of the Kubernetes deployment YAML to apps/v1, and the modernization of the Dockerfile to use a multi-stage build are all excellent improvements. I've added a suggestion for the Dockerfile to further improve it by building a static binary and using a scratch base image, which will result in a smaller and more secure container image.
6f54309 to
66f3231
Compare
|
/gemini review |
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.
Code Review
This pull request introduces a new Go module for a visit-counter application, updating its go.mod and go.sum files. The Dockerfile for the application's GKE deployment was refactored to use a multi-stage build, transitioning from golang:1.24-alpine to a golang:1.24-bookworm builder stage and a scratch final image, aiming for a smaller image size. Additionally, the Kubernetes visit-counter.yaml deployment manifest was updated from extensions/v1beta1 to apps/v1 and now includes the required selector field. A minor comment change was also made in main.go. The review comments point out that the refactored Dockerfile's final scratch image is missing an ENTRYPOINT to specify the application binary to run and an EXPOSE instruction for port documentation, suggesting their inclusion for proper container functionality and best practices.
66f3231 to
06b8ffc
Compare
|
/gemini review |
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.
Code Review
This pull request provides a much-needed update to the Memorystore Go sample, bringing it up to modern standards. The changes are mostly well-executed, including refactoring the Go module, improving the Dockerfile with multi-stage builds, and updating the Kubernetes deployment manifest. These are great improvements. However, I found one issue with the Go version specified in the new go.mod file that will prevent the sample from building. Please see the detailed comment.
06b8ffc to
84019a9
Compare
|
/gemini review |
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.
Code Review
This pull request provides a much-needed update to the Memorystore Go samples, bringing them in line with modern practices. The changes are well-executed and improve the sample's correctness, security, and maintainability. Key improvements include updating the Kubernetes manifest to a current API version, significantly enhancing the Dockerfile with a multi-stage build for a smaller and more secure image, and cleaning up the Go module dependencies. The result is a more robust and up-to-date example for users. I have no further comments; the changes are excellent.
84019a9 to
df4a94c
Compare
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
df4a94c to
379b822
Compare
|
@iennae are you able to review this PR? |
rodydavis
left a comment
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.
LGTM
379b822 to
36c2f79
Compare
36c2f79 to
78d485e
Compare
78d485e to
d628d4d
Compare
Description
Fixes #5435
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
go test -v ./..(see Testing)gofmt(see Formatting)go vet(see Formatting)