-
Notifications
You must be signed in to change notification settings - Fork 19
docs: add known_hosts container deployment warning to storage config #384
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
Pull Request ReviewThank you for addressing issue #383! This is a valuable addition that will help users deploying Flipt in containers avoid the SSH known_hosts configuration issue. ✅ Strengths
📝 Suggestions1. Consider Adding Context About WhyThe documentation shows how to mount the file but could benefit from a brief explanation of why the user home directory doesn't work. Consider adding a sentence like: **Container Deployment**: When running Flipt in containers, the flipt user is a system user without a home directory. Mount the known_hosts file to the system-wide SSH path instead:2. Minor Formatting ConsistencyIn the v1 file, the whitespace changes in the Azure credentials section (lines 497-504) are good cleanup - changing tabs to spaces after comments. This improves consistency but isn't directly related to the PR's main purpose. Consider mentioning this in the PR description as "also fixes comment alignment" to make the scope clear. 3. Consider SSH_KNOWN_HOSTS VariableThe issue mentioned that setting the volumes:
- /path/to/your/known_hosts:/etc/ssh/ssh_known_hosts:ro
# Alternatively, you can set: SSH_KNOWN_HOSTS=/etc/ssh/ssh_known_hosts✨ Quality Checks
🎯 VerdictThis PR is ready to merge with the current changes. The suggestions above are optional enhancements that could make the documentation even more helpful, but they're not blockers. The core value - helping users avoid the known_hosts error in containers - is well addressed. Great work! 🚀 |
fixes #383 Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Pull Request ReviewOverviewThis PR addresses issue #383 by adding documentation for mounting known_hosts files in containerized Flipt deployments. The changes directly solve a common configuration issue users encounter when using SSH credentials with Git storage. Positive Aspects ✅
Issues and Concerns
|
Pull Request ReviewThank you for addressing issue #383! This PR adds helpful documentation for container deployments using SSH credentials. Here's my review: ✅ Positive Aspects
📝 Suggestions for Improvement1. Formatting ConsistencyThe new warning text doesn't match the formatting of the existing warning content. Consider: Current: _Container Deployment_: When running Flipt in containers, mount the known_hosts file...Suggested: **Container Deployment**: When running Flipt in containers, mount the known_hosts file...Using 2. Enhanced ClarityConsider adding a brief explanation of why the system-wide path is needed: **Container Deployment**: When running Flipt in containers, the `flipt` user is a system user without a home directory. Mount the known_hosts file to the system-wide SSH path instead:This provides immediate context about why the user home directory approach fails. 3. Cross-Reference OpportunityConsider mentioning the GitOps guide earlier in the warning, since it contains practical examples of Git backend setup. 4. Minor: Code Block LanguageThe volume mount example uses 🔍 Technical Review
🧪 Testing ConsiderationsWhile this is documentation, consider:
📊 Writing StandardsPer the Microsoft Writing Style Guide and CLAUDE.md:
Overall AssessmentThis is a valuable documentation improvement that will help users avoid a common containerization issue. The changes are accurate, well-placed, and solve a real problem. With minor formatting adjustments, this will be excellent documentation. Recommendation: Approve with minor suggestions for enhanced clarity. |
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Pull Request ReviewSummaryThis PR adds helpful documentation for container deployments when using SSH credentials with Git storage backends. It addresses issue #383 by clarifying where to mount the known_hosts file in containerized environments. Positive Observations✅ Clear Problem Resolution: The added documentation directly addresses a real user pain point documented in issue #383 ✅ Consistency: The same guidance is added to both v1 and v2 documentation, maintaining version parity ✅ Good Example: The Docker volume mount example is practical and immediately actionable ✅ Code Quality Improvements: Fixed comment syntax from HTML-style ( ✅ Whitespace Cleanup: Fixed trailing tabs in Azure credential code comments for better consistency Suggestions for Improvement1. Markdown Link Formatting (per CLAUDE.md guidelines)The CLAUDE.md file specifies that raw URLs should use proper markdown link syntax. Currently line 357 in v1/configuration/storage.mdx has: While this doesn't contain a raw URL, consider making "Container Deployment" more prominent, perhaps as a note or using bold formatting: 2. Language Tag on Code BlockPer CLAUDE.md requirements ("Language tags on all code blocks"), the YAML code block should have a language tag. Currently: volumes:
- /path/to/your/known_hosts:/etc/ssh/ssh_known_hosts:roThis is already correct! ✅ 3. Consider Adding ContextThe new content could benefit from a brief explanation of why the system-wide path is needed. For example: 4. Sentence StructureThe sentence could be split for better readability: Current:
Suggested:
(Note the colon after "Docker" to better introduce the code block) Security Considerations✅ The Testing Recommendations
Overall AssessmentThis is a solid documentation improvement that addresses a real user issue. The changes are minimal, focused, and follow the project's "smallest reasonable changes" principle from CLAUDE.md. The minor suggestions above are optional refinements - the PR is good to merge as-is. Recommendation: ✅ Approve with optional minor refinements |
fixes #383