-
Notifications
You must be signed in to change notification settings - Fork 21
Add notes about local Redis use for integration verification. #721
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
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
|
Claude finished @enmande's task —— View job PR Review CompleteTasks
Review SummaryOverall, this is a well-written addition to the documentation. The Redis section follows the established patterns and provides clear, actionable instructions. I have a few minor suggestions below. Feedback👍 What's Good
📝 ObservationsLine 232 ( For features that support Redis caching, verify locally that Redis is being used as opposed to an application's in-memory cache, database, or other backing store.Line 237 ( 1. Use a standard [Redis docker image](https://hub.docker.com/_/redis) such as `redis:alpine` for latest. Pull it locally and start the container.Line 243 ( 3. Connect to the Redis CLI via Docker if you wish to monitor activity, for example with [MONITOR](https://redis.io/docs/latest/commands/monitor/).Or even more direct: 3. (Optional) [Connect to the Redis CLI via Docker](https://redis.io/docs/latest/operate/oss_and_stack/install/install-stack/docker/#connect-with-redis-cli) to monitor activity using [MONITOR](https://redis.io/docs/latest/commands/monitor/).ℹ️ Side NoteI noticed the diff also includes two formatting fixes (adding blank lines) at:
These are good formatting improvements that improve readability. 👍 ✅ Style Guide Compliance
VerdictThe changes are solid and add valuable documentation. The minor wording suggestions above are optional improvements to make the instructions slightly more action-oriented per the style guide, but the content is clear and functional as-is. |
withinfocus
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.
Lint. I agree with Claude's comments on using action-oriented language too.
If we're going to suggest a local Redis option, go ahead and propose a change to the Docker Compose configuration for a dev setup in server too, and then you can refer to just bringing up a container that's pre-configured vs. describe options.
theMickster
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.
Agree with refactoring the terminology and new docker compose script as opposed to directions on how to configure the container. Doing so will also ensure we all use the same port for a local redis container thus making it easier to help one another troubleshoot local setup issues.
| available caching and persistence methods should have another backing store available, which will be | ||
| automatically used if Redis is not configured. | ||
| 1. You may use a standard [Redis docker image](https://hub.docker.com/_/redis) such as |
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.
🎨 Please note that the numbering found in this document (and, heck, all of our others) follow the 1., 2., 3., etc. markdown style. Will you please update this numbered list?
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.
🤔 Thanks for the feedback. You're right to suggest I should have matched the existing examples more closely; existing code is good documentation, and I appreciate the nudge.
I did check for explicit guidance first and the only current assertion I found on Markdown ordered lists is in the CLAUDE.md file. It addresses use of ordered lists for instructions in documentation, but doesn’t mention this style preference. Since 1. repetition is a common convention across Markdown flavors, I wonder if this is something we might want to surface more explicitly in a style guide or Claude instruction block? That could help ensure consistent usage across contributors and during future reviews.
|
Closing this for now, with a followup task. Totally agree with the suggestion that this should be codified in the docker-compose as a reproducible solution over describing options; it is a more stable path forward. As this increases the scope of engagement from a documentation task (“describe what had to be figured out”) to an engineering task (“implement what we will do going forward”), it has been ticketed for followup in PM-29150 to account for the time and domain needed to do that properly. The issue has been written so as to include followup here in the docs. |

🎟️ Tracking
No ticket for this specific addition, however local needs and subsequent team discussions around PM-23572 suggested a brief callout in documentation could be warranted.
📔 Objective
Add notes about how to locally verify Redis integration when developing features that would consume it in Cloud, or other environments in which it could be configured as a backing store (e.g., for ExtendedCache).
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes