Skip to content

Conversation

@enmande
Copy link
Contributor

@enmande enmande commented Dec 2, 2025

🎟️ 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

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
    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 confirmed
    issue 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

@enmande enmande marked this pull request as ready for review December 2, 2025 19:50
@enmande enmande requested a review from a team as a code owner December 2, 2025 19:50
@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2025

Logo
Checkmarx One – Scan Summary & Details13360d4d-960d-4d5b-b512-4662af746ffd

Great job! No new security vulnerabilities introduced in this pull request

@claude
Copy link

claude bot commented Dec 2, 2025

Claude finished @enmande's task —— View job


PR Review Complete

Tasks

  • Read the modified file to understand changes
  • Review documentation against style guidelines
  • Provide comprehensive feedback

Review Summary

Overall, 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

  • Clear explanation of the use case for local Redis verification
  • Good note about fallback to other backing stores when Redis isn't configured
  • Proper use of numbered instructions following the style guide
  • Inline code formatting for configuration paths and commands
  • Helpful reference to Redis CLI monitoring with the MONITOR command

📝 Observations

Line 232 (docs/getting-started/server/advanced-setup.md:232): The phrase "you may want to verify locally" is conversational. Consider making it more direct:

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 (docs/getting-started/server/advanced-setup.md:237): "You may use a standard" - consider starting with an action verb per the style guide:

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 (docs/getting-started/server/advanced-setup.md:243): "You may connect" - again, consider action-oriented language:

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 Note

I noticed the diff also includes two formatting fixes (adding blank lines) at:

  • Line 65 (before the Azure Storage nested list)
  • Line 171 (before the Command line nested list)

These are good formatting improvements that improve readability. 👍

✅ Style Guide Compliance

  • ✅ Numbered instructions with action-oriented verbs (mostly)
  • ✅ Proper heading capitalization
  • ✅ Code blocks used appropriately
  • ✅ Inline code formatting for technical terms
  • ✅ Concise paragraphs
  • ✅ Links to external documentation

Verdict

The 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.


Copy link
Contributor

@withinfocus withinfocus left a 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.

Copy link
Contributor

@theMickster theMickster left a 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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@enmande
Copy link
Contributor Author

enmande commented Dec 3, 2025

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.

@enmande enmande closed this Dec 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants