-
Notifications
You must be signed in to change notification settings - Fork 74
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
Update site-to-site networking related documentation #457
Conversation
Caution Review failedThe pull request is closed. WalkthroughThe documentation for the Tailscale Home Assistant add-on has been updated with clarifications regarding configuration options, particularly focusing on the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tailscale/DOCS.md (3)
150-152
: Consider rephrasing for better readability.The note provides valuable clarification. Consider rephrasing to avoid repetition:
-**Note:** If you only want to access your local subnet from other clients on -your tailnet, but you don't want to access other clients on your tailnet from -your local subnet, you don't need to disable the `userspace_networking` option. +**Note:** The `userspace_networking` option can remain enabled if you only need +one-way access from tailnet clients to your local subnet, without requiring +access from your local subnet to other tailnet clients.🧰 Tools
🪛 LanguageTool
[style] ~150-~150: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...rted interfaces. Note: If you only want to access your local subnet from other cli...(REP_WANT_TO_VB)
288-294
: Fix abbreviation and enhance clarity.The note provides helpful context about the add-on's capabilities. Consider these improvements:
-steps on [Site-to-site networking][tailscale_info_site_to_site] (Note: "IP -address forwarding" and "Clamp the MSS to the MTU" is already done by the -add-on). +steps in the [Site-to-site networking][tailscale_info_site_to_site] guide (Note: The add-on +already handles "IP address forwarding" and "Clamp the MSS to the MTU" for you). -**Note:** Disable this option only when you really understand why you need this. -If you are not interested in the real source IP address, you don't need to -disable this option. +**Note:** Only disable this option if you fully understand the implications. +Keep it enabled if preserving the real source IP address is not critical for +your use case.
348-355
: Consider consolidating duplicate notes and fix abbreviation.
The note about userspace_networking (lines 348-351) is duplicated from lines 150-152. Consider keeping it only in one location, preferably here in the userspace_networking section.
Fix the abbreviation in the last note:
-in the real source IP address, ie. subnet devices can see the traffic +in the real source IP address, i.e., subnet devices can see the traffic🧰 Tools
🪛 LanguageTool
[uncategorized] ~353-~353: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...terested in the real source IP address, ie. subnet devices can see the traffic orig...(I_E)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tailscale/DOCS.md
(3 hunks)
🧰 Additional context used
🪛 LanguageTool
tailscale/DOCS.md
[style] ~150-~150: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...rted interfaces. Note: If you only want to access your local subnet from other cli...
(REP_WANT_TO_VB)
[uncategorized] ~353-~353: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...terested in the real source IP address, ie. subnet devices can see the traffic orig...
(I_E)
🔇 Additional comments (2)
tailscale/DOCS.md (2)
337-346
: Excellent addition of subnet collision behavior.The note about subnet collision behavior is a crucial addition that helps users understand the add-on's networking priorities and limitations. This information can prevent confusion and potential issues during setup.
Line range hint
150-355
: Documentation updates successfully achieve their objectives.The changes effectively:
- Remove references to step numbers in the Tailscale documentation
- Add valuable context about configuration options
- Provide clear guidance about when to modify specific settings
- Include important technical details about networking behavior
These updates will help users better understand the implications of various configuration options.
🧰 Tools
🪛 LanguageTool
[style] ~334-~334: The adverb ‘also’ is commonly used to connect clauses and isn’t usually used at the end of a phrase or before a conjunction. Consider replacing it with a more formal alternative.
Context: ... configure Home Assistant's DNS options also. If you want to access other clients o...(ALSO_AS_WELL)
[uncategorized] ~353-~353: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...terested in the real source IP address, ie. subnet devices can see the traffic orig...(I_E)
✅ Actions performedReview triggered.
|
59e8c5b
to
f6bd985
Compare
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, @lmagyar 👍
../Frenck
Proposed Changes
Rephrase docs to not reference step numbers in continuously changing TS docs.
Plus some Note on config options.
Related Issues
Summary by CodeRabbit
snat_subnet_routes
option and its implications, advising users on its management.userspace_networking
and its usage scenarios, clarifying its necessity for one-way access.