Skip to content

Conversation

@erka
Copy link
Collaborator

@erka erka commented Jan 21, 2026

fixes #383

@erka erka requested a review from a team as a code owner January 21, 2026 13:18
@claude
Copy link
Contributor

claude bot commented Jan 21, 2026

Pull Request Review

Thank 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

  1. Clear Documentation: The added container deployment guidance is practical and directly addresses the reported issue
  2. Consistency: Applied the same change to both v1 and v2 documentation, maintaining version parity
  3. Good Example: The YAML code block provides a clear, actionable example with the read-only mount flag
  4. Proper Context: Placed within the existing Warning block where SSH configuration is already discussed

📝 Suggestions

1. Consider Adding Context About Why

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

In 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 Variable

The issue mentioned that setting the SSH_KNOWN_HOSTS environment variable didn't work. You might want to briefly mention this as an alternative approach or clarify why the mount method is preferred:

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

  • ✅ Follows Microsoft Writing Style Guide conventions
  • ✅ Uses proper markdown formatting
  • ✅ Code blocks have language tags
  • ✅ Consistent with existing documentation style
  • ✅ No security concerns introduced
  • ✅ Clear and actionable guidance

🎯 Verdict

This 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>
@claude
Copy link
Contributor

claude bot commented Jan 21, 2026

Pull Request Review

Overview

This 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 ✅

  1. Addresses Real User Pain Point: The addition directly solves the documented issue where users struggle with SSH known_hosts configuration in containers.

  2. Consistent Implementation: The warning is added to both v1 and v2 documentation, maintaining consistency across versions.

  3. Clear and Actionable: The Docker volume mount example is practical and easy to follow.

  4. Good Placement: The information is added within the existing Warning block about insecure_ignore_host_key, which is contextually appropriate.

  5. Code Quality Fix: The Azure credentials comment spacing standardization (line 504-507 in v1/configuration/storage.mdx) improves consistency.

Issues and Concerns ⚠️

1. Incorrect MDX Comment Syntax (High Priority)

Files: docs/v2/cli/commands/license/activate.mdx and docs/v2/cli/commands/license/check.mdx

The comment syntax {/-- TODO --/} is incorrect for MDX files. According to MDX standards, comments should use JSX comment syntax:

Current (Incorrect):

{/-- TODO: Add screenshot of license activation wizard --/}

Should be:

{/* TODO: Add screenshot of license activation wizard */}

This will cause rendering issues. Please update both files to use the correct {/* */} syntax.

2. Minor Formatting Inconsistencies

Files: docs/v1/configuration/storage.mdx and docs/v2/configuration/storage.mdx

The new content has a minor formatting issue:

  • Line 357 (v1) and 201 (v2): Missing colon after "For example, with Docker"

Current:

_Container Deployment_: When running Flipt in containers, mount the known_hosts file to the system-wide SSH path instead of a user directory. For example, with Docker

Suggested:

_Container Deployment_: When running Flipt in containers, mount the known_hosts file to the system-wide SSH path instead of a user directory. For example, with Docker:

Recommendations

  1. Fix the MDX comment syntax in the license files (lines 38 in both files)
  2. Add a colon after "with Docker" for better readability
  3. Consider adding the :ro (read-only) flag explanation in a brief comment or note, as users may wonder why it's recommended
  4. Run linting before the final commit:
    • npm run lint (from docs/ directory)
    • npm run format (from docs/ directory)
    • vale * (from docs/ directory) to check prose quality

Security & Performance ✅

  • No security concerns identified
  • The read-only mount (:ro) is a security best practice
  • No performance implications

Conclusion

This is a valuable addition to the documentation that will help users avoid a common deployment pitfall. Once the MDX comment syntax is corrected and the minor formatting issue is addressed, this will be ready to merge.

Action Required: Please fix the comment syntax in the license files before merging.

@claude
Copy link
Contributor

claude bot commented Jan 21, 2026

Pull Request Review

Thank you for addressing issue #383! This PR adds helpful documentation for container deployments using SSH credentials. Here's my review:

✅ Positive Aspects

  1. Addresses Real Pain Point: The added warning about container deployments directly solves a documented user issue with known_hosts file mounting.

  2. Consistent Application: The fix is applied to both v1 and v2 documentation, ensuring consistency across versions.

  3. Clear Example: The Docker volume mount example is practical and actionable.

  4. Good Code Hygiene: The HTML comment to JSX comment conversion (<!-- --> to {/* */}) in the license files is a welcome MDX best practice improvement.

  5. Whitespace Cleanup: The tab/space alignment fix in the Azure credentials section improves code consistency.

📝 Suggestions for Improvement

1. Formatting Consistency

The 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 **bold** instead of _italic_ maintains consistency with Markdown conventions where bold is typically used for emphasis/labels rather than italics.

2. Enhanced Clarity

Consider 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 Opportunity

Consider mentioning the GitOps guide earlier in the warning, since it contains practical examples of Git backend setup.

4. Minor: Code Block Language

The volume mount example uses yaml but it's specifically a Docker Compose syntax. Consider adding a comment or using docker-compose as the language identifier for clarity.

🔍 Technical Review

  • No Security Issues: Read-only mount (:ro) is correctly used
  • No Performance Impact: Documentation-only changes
  • Correct Paths: /etc/ssh/ssh_known_hosts is the proper system-wide location
  • No Breaking Changes: Purely additive documentation
  • Proper Frontmatter: Existing frontmatter remains intact

🧪 Testing Considerations

While this is documentation, consider:

  • Validating the Docker volume mount example works with the actual Flipt container
  • Ensuring the path /etc/ssh/ssh_known_hosts is correct for the Flipt container base image

📊 Writing Standards

Per the Microsoft Writing Style Guide and CLAUDE.md:

  • ✅ Uses second-person voice
  • ✅ Provides actionable guidance
  • ✅ Includes practical examples
  • ⚠️ Minor: Consider using "you" explicitly (e.g., "mount your known_hosts file")

Overall Assessment

This 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>
@claude
Copy link
Contributor

claude bot commented Jan 21, 2026

Pull Request Review

Summary

This 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 (<!-- -->) to JSX-style ({/* */}) in MDX files (lines in activate.mdx and check.mdx), which is the correct syntax for Mintlify/MDX

Whitespace Cleanup: Fixed trailing tabs in Azure credential code comments for better consistency

Suggestions for Improvement

1. 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:

_Container Deployment_: When running Flipt in containers, mount the known_hosts file...

While this doesn't contain a raw URL, consider making "Container Deployment" more prominent, perhaps as a note or using bold formatting:

**Container Deployment**: When running Flipt in containers, mount the known_hosts file...

2. Language Tag on Code Block

Per 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:ro

This is already correct! ✅

3. Consider Adding Context

The new content could benefit from a brief explanation of why the system-wide path is needed. For example:

**Container Deployment**: When running Flipt in containers, mount the known_hosts file to the system-wide SSH path (`/etc/ssh/ssh_known_hosts`) instead of a user directory, as the flipt user in the container may not have a home directory configured.

4. Sentence Structure

The sentence could be split for better readability:

Current:

When running Flipt in containers, mount the known_hosts file to the system-wide SSH path instead of a user directory. For example, with Docker

Suggested:

When running Flipt in containers, mount the known_hosts file to the system-wide SSH path instead of a user directory. For example, with Docker:

(Note the colon after "Docker" to better introduce the code block)

Security Considerations

✅ The :ro (read-only) flag on the volume mount is excellent security practice

Testing Recommendations

  • Verify the documentation renders correctly in Mintlify preview (mint dev)
  • Run Vale linting to check prose quality (vale docs/v1/configuration/storage.mdx docs/v2/configuration/storage.mdx)
  • Test that the suggested volume mount actually resolves the issue in a real Docker environment

Overall Assessment

This 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

@erka erka merged commit b831fe9 into main Jan 21, 2026
5 checks passed
@erka erka deleted the ssh-host-file branch January 21, 2026 17:24
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.

Git ssh known host configuration is not documented

3 participants