Merge proxy and funnel options into share_homeassistant, rename proxy_and_funnel_port to share_on_port (config automatically updated) #445
Conversation
|
Warning Rate limit exceeded@lmagyar has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (7)
WalkthroughThe pull request introduces significant changes to the Tailscale Home Assistant add-on's configuration and documentation. The primary modifications involve replacing the existing Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tailscale/DOCS.md (1)
190-192: Fix grammar in browser security explanationThere's a minor grammar issue in the sentence explaining browser security awareness.
-browsers are not aware of the connections between Tailscale nodes are secured +browsers are not aware that the connections between Tailscale nodes are secured🧰 Tools
🪛 LanguageTool
[uncategorized] ~191-
191191: “of” seems less likely than “if” (in case).
Context: ...ook unencrypted (browsers are not aware of the connections between Tailscale nodes...(AI_HYDRA_LEO_CP_OF_IF)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tailscale/DOCS.md(4 hunks)tailscale/config.yaml(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/post-tailscaled/run(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-homeassistant/finish(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-homeassistant/run(2 hunks)tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh(2 hunks)tailscale/translations/en.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/post-tailscaled/run
🧰 Additional context used
🪛 LanguageTool
tailscale/DOCS.md
[uncategorized] ~191-191191: “of” seems less likely than “if” (in case).
Context: ...ook unencrypted (browsers are not aware of the connections between Tailscale nodes...
(AI_HYDRA_LEO_CP_OF_IF)
🔇 Additional comments (17)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-homeassistant/finish (2)
5-5: Clarify naming consistency in comment.
Renamed reference from 'serve' to 'share-homeassistant' is consistent with the newly introduced config option. No issues found.
10-10: Verify script references to 'share-homeassistant'.
The service variable has been updated to "share-homeassistant." Ensure other scripts consistently reference this new name to avoid confusion.tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-homeassistant/run (4)
5-5: Good rewording in the comment.
Renaming the comment to mention both Tailscale Serve or Funnel clarifies the actual usage. Clear improvement.
46-46: Correct new reference for documentation.
The updated error message now points to "Option: share_homeassistant," aligning with the new configuration. This is correct and more descriptive.
51-52: Appropriate check for Funnel capability.
Conditionally verifying the Tailscale funnel capability whenshare_homeassistantis set to 'funnel' prevents misconfiguration errors. Excellent approach.
61-62: Efficient handling via direct config usage.
Using$(bashio::config 'share_homeassistant')for the Tailscale command is clean and avoids separate condition branching. This is concise and maintainable.tailscale/config.yaml (1)
39-40: Schema updates reflect new configuration structure.
The introduction ofshare_homeassistantandshare_on_portin the schema effectively replaces the old funnel/proxy options. This is well-organized and consistent with the PR's goal of clarifying configuration.tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh (6)
8-10: Proper variable declarations for upgrade logic.
Declaring the variables up-front aligns with best practices, ensuring the script remains clear and self-documenting.
11-17: Seamless upgrade approach.
Reading existing addon options forproxy,funnel, andproxy_and_funnel_portensures backward compatibility. Excellent approach to preserve user settings.
18-24: Upgrade logic sets correct values.
Translating olderproxy+funneltofunnelorserveis straightforward. No issues found.
25-28: share_on_port migration is clear.
The script properly assignsproxy_and_funnel_porttoshare_on_port. Straightforward and consistent with the rest of the upgrade logic.
29-38: Removing deprecated options.
This ensures the older fields no longer appear in configuration. It's a clean approach to maintain an updated config state.
61-65: Disabling the service aligns with the new config.
Ifshare_homeassistantis not set or explicitly disabled, removing the service fromcontents.dprevents startup overhead. Good approach to conditionally manage the service.tailscale/translations/en.yaml (2)
48-53: LGTM! Clear and descriptive configuration option.The new
share_homeassistantoption effectively combines and clarifies the previous proxy and funnel functionality. The description accurately explains the purpose and capabilities.
55-60: Verify port restrictions in implementation codeThe description correctly lists the allowed ports (443, 8443, 10000). Let's verify this is enforced in the implementation.
✅ Verification successful
Port restrictions correctly enforced via regex validation
The implementation in
config.yamlvalidates the port numbers using a regex pattern^(443|8443|10000)$which exactly matches the allowed ports mentioned in the description (443, 8443, 10000). The validation is properly enforced at the configuration level.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for port validation logic rg -A 5 "share_on_port|proxy_and_funnel_port" --type yamlLength of output: 820
tailscale/DOCS.md (2)
179-208: LGTM! Comprehensive documentation of the new features.The documentation for
share_homeassistantis thorough and well-structured:
- Clear explanation of both Serve and Funnel features
- Security implications are properly documented
- Setup instructions are detailed and accurate
🧰 Tools
🪛 LanguageTool
[uncategorized] ~191-
191191: “of” seems less likely than “if” (in case).
Context: ...ook unencrypted (browsers are not aware of the connections between Tailscale nodes...(AI_HYDRA_LEO_CP_OF_IF)
253-259: LGTM! Clear port configuration documentation.The
share_on_portdocumentation clearly specifies:
- Allowed port numbers (443, 8443, 10000)
- Default value (443)
- Purpose of the configuration
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh (4)
29-29: Fix typo in comments.The word "Ugrade" should be "Upgrade".
-# Ugrade to share_homeassistant +# Upgrade to share_homeassistant -# Ugrade to share_on_port +# Upgrade to share_on_portAlso applies to: 41-41
32-32: Enhance warning messages with deprecation timeline.The warning messages about deprecated options would be more helpful if they included information about when these options will be removed.
- bashio::log.warning "The proxy and funnel options are already migrated to share_homeassistant option, do not configure deprecated options, proxy and funnel options are dropped." + bashio::log.warning "The proxy and funnel options are deprecated and will be removed in the next major version. Please use only the share_homeassistant option." - bashio::log.warning "The proxy_and_funnel_port option is already migrated to share_on_port option, do not configure deprecated options, proxy_and_funnel_port option is dropped." + bashio::log.warning "The proxy_and_funnel_port option is deprecated and will be removed in the next major version. Please use only the share_on_port option."Also applies to: 44-44
46-49: Enhance error message for invalid port configuration.The error message for invalid port values could be more helpful by explaining the expected format and valid range.
- bashio::log.warning "The proxy_and_funnel_port option value '${proxy_and_funnel_port}' is invalid, proxy_and_funnel_port option is dropped." + bashio::log.warning "The proxy_and_funnel_port option value '${proxy_and_funnel_port}' is invalid. Port values must be between 1 and 65535. The option will be dropped."
21-61: Excellent implementation of the configuration upgrade process.The migration logic successfully implements the PR objectives by:
- Providing a smooth upgrade path from old to new options
- Maintaining backward compatibility
- Including appropriate cleanup of deprecated options
- Adding clear user notifications
This implementation will help reduce user confusion by simplifying the configuration options while ensuring existing configurations continue to work.
Consider adding telemetry or logging to track the usage of deprecated options. This data could help determine when it's safe to remove the migration code in a future version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh(2 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh
[error] 16-16: Double quote array expansions to avoid re-splitting elements.
(SC2068)
🔇 Additional comments (1)
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh (1)
84-88: LGTM! Clean implementation of service management.The service management logic correctly handles both cases where the share-homeassistant service should be disabled:
- When the option is not set
- When it's explicitly set to 'disabled'
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh (4)
29-30: Fix typo in comments.The word "Upgrade" is misspelled as "Ugrade" in the comments.
-# Ugrade to share_homeassistant +# Upgrade to share_homeassistant -# Ugrade to share_on_port +# Upgrade to share_on_portAlso applies to: 41-42
46-50: Enhance error handling for share_on_port validation.The current implementation only logs a warning when the port value is invalid. Consider adding a default fallback value or documenting the behavior in the logs.
try bashio::addon.option 'share_on_port' "^${proxy_and_funnel_port}" if (($TRY_ERROR)); then - bashio::log.warning "The proxy_and_funnel_port option value '${proxy_and_funnel_port}' is invalid, proxy_and_funnel_port option is dropped." + bashio::log.warning "The proxy_and_funnel_port option value '${proxy_and_funnel_port}' is invalid, proxy_and_funnel_port option is dropped. Using default port." + try bashio::addon.option 'share_on_port' "^443" fi
21-61: Add success logging for configuration migration.The migration logic is well-structured, but adding success logs would help users understand when the migration completes successfully.
if bashio::var.true "${proxy}"; then if bashio::var.has_value "${share_homeassistant}"; then bashio::log.warning "The proxy and funnel options are already migrated to share_homeassistant option, do not configure deprecated options, proxy and funnel options are dropped." else if bashio::var.true "${funnel}"; then bashio::addon.option 'share_homeassistant' 'funnel' + bashio::log.info "Successfully migrated proxy and funnel options to share_homeassistant: funnel" else bashio::addon.option 'share_homeassistant' 'serve' + bashio::log.info "Successfully migrated proxy and funnel options to share_homeassistant: serve" fi fi fi
21-23: Plan for future maintenance.As noted in the comment, the configuration upgrade logic can be removed in a later version. Consider creating a follow-up issue to track the removal of this migration code after a reasonable deprecation period (e.g., after 2-3 releases).
Would you like me to create a GitHub issue to track the future removal of the migration code?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh(2 hunks)
🔇 Additional comments (2)
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh (2)
8-19: LGTM! Well-structured variable declarations and error handling.The variable declarations are clean and the try function implementation properly handles command arguments with proper quoting.
84-88: LGTM! Clean service management implementation.The share-homeassistant service management follows consistent patterns with other service blocks and properly handles both undefined and disabled states.
|
Marking it draft: I will convert this logic into a oneshot service after banner and log-level, this is a bit ugly to see a log message like 'INFO: Successfully migrated proxy and funnel options to share_homeassistant: funnel' before the main add-on banner. |
|
No change, ready for review, stage2_hook uses the new options in conditions, they have to be there, so config upgrade must happen at the beginning of stage2_hook. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh (2)
21-55: Consider optimizing the configuration upgrade flow.The upgrade logic is comprehensive but could be optimized:
- Consider adding an early exit if no deprecated options are present
- The "^" prefix in
share_on_portvalidation could use documentationConsider this optimization:
options=$(bashio::addon.options) proxy=$(bashio::jq "${options}" '.proxy // empty') +# Skip upgrade if no deprecated options are present +if ! bashio::var.has_value "${proxy}" && \ + ! bashio::var.has_value "${funnel}" && \ + ! bashio::var.has_value "${proxy_and_funnel_port}"; then + bashio::log.debug "No configuration upgrade needed" + exit 0 +fi funnel=$(bashio::jq "${options}" '.funnel // empty')
48-48: Document the port validation prefix.The "^" prefix in the port validation is not documented. Consider adding a comment explaining its purpose and validation rules.
+ # The "^" prefix ensures the port number is valid and within allowed range try bashio::addon.option 'share_on_port' "^${proxy_and_funnel_port}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh(2 hunks)
🔇 Additional comments (4)
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh (4)
8-10: Well-structured variable declarations!The variables are logically grouped and explicitly declared, following good shell scripting practices.
12-19: Robust error handling implementation!The
tryfunction effectively isolates potentially failing API calls while maintaining proper error state management. The implementation correctly handles command arguments with spaces.
88-92: Clean service management implementation!The conditions for managing the share-homeassistant service state are clear and consistent with the rest of the service management patterns in the file.
21-65: Verify configuration option updates across the codebase.Let's ensure all references to the old configuration options have been updated.
✅ Verification successful
Configuration migration is properly documented and implemented
The configuration changes from
proxy,funnel, andproxy_and_funnel_porttoshare_homeassistantandshare_on_portare properly handled:
- The migration code in
stage2_hook.shcorrectly handles the transition- The old options are not present in the
config.yamlschema, indicating they are fully removed- The
DOCS.mdhas been updated to only reference the new options with proper documentation- No other files in the codebase contain references to the old configuration options
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old configuration options echo "Searching for old configuration references..." rg -l 'proxy_and_funnel_port|\.proxy\s|\.funnel\s' --type yaml --type bash --type markdown # Search for potential documentation that needs updating echo "Checking documentation files..." fd -e md -e yaml . -x grep -l -E 'proxy_and_funnel_port|proxy.*funnel|funnel.*proxy'Length of output: 382
Script:
#!/bin/bash # Search for any remaining references to the old configuration options with correct file types echo "Searching for old configuration options in all files..." rg -l 'proxy_and_funnel_port|\.proxy\b|\.funnel\b' # Search specifically in configuration and documentation echo "Checking configuration files..." find . -name "config.yaml" -o -name "*.md" | xargs grep -l 'proxy_and_funnel_port\|proxy\|funnel'Length of output: 443
Script:
#!/bin/bash # Let's check the content of these files to see if they still contain old configuration references echo "Content of DOCS.md:" cat ./tailscale/DOCS.md echo -e "\nContent of config.yaml:" cat ./tailscale/config.yamlLength of output: 18298
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh (1)
50-55: Consider adding retry logic for port migration.The port migration is a one-shot operation. If it fails due to temporary API issues, it drops to the default port without retrying. Consider adding retry logic for transient failures.
- try bashio::addon.option 'share_on_port' "^${proxy_and_funnel_port}" - if ((TRY_ERROR)); then - bashio::log.warning "The proxy_and_funnel_port option value '${proxy_and_funnel_port}' is invalid, proxy_and_funnel_port option is dropped, using default port." - else - bashio::log.info "Successfully migrated proxy_and_funnel_port option to share_on_port: ${proxy_and_funnel_port}" - fi + max_retries=3 + retry_count=0 + while ((retry_count < max_retries)); do + try bashio::addon.option 'share_on_port' "^${proxy_and_funnel_port}" + if ((TRY_ERROR)); then + ((retry_count++)) + if ((retry_count == max_retries)); then + bashio::log.warning "The proxy_and_funnel_port option value '${proxy_and_funnel_port}' is invalid after ${max_retries} attempts, using default port." + break + fi + bashio::log.info "Retry ${retry_count}/${max_retries} for port migration..." + sleep 1 + else + bashio::log.info "Successfully migrated proxy_and_funnel_port option to share_on_port: ${proxy_and_funnel_port}" + break + fi + done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh(2 hunks)
🔇 Additional comments (3)
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh (3)
8-19: LGTM! Clean variable declarations and error handling.The variable declarations follow best practices, and the
tryfunction implementation correctly handles command arguments with proper error propagation.
90-94: LGTM! Clear service management logic.The conditions for enabling/disabling the share-homeassistant service are well-defined and consistent with the add-on's service management pattern.
24-67: Verify the configuration upgrade logic.The migration logic looks solid, but let's verify a few edge cases:
- What happens if
proxy=falsebutfunnel=true?- Is there a risk of data loss if the migration fails midway?
✅ Verification successful
Configuration upgrade logic is safe and well-implemented
The migration logic handles edge cases appropriately:
- Only migrates when proxy=true, preventing unintended migrations
- Validates port numbers against allowed values (443, 8443, 10000)
- Sets new options before removing old ones, preventing data loss
- Falls back to default values when validation fails
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential edge cases in configuration files # Look for any existing configurations that might be affected # Search for proxy/funnel configurations rg -l '(proxy|funnel|proxy_and_funnel_port):' --type yaml # Search for any error logs related to configuration migration rg -i 'error.*migrat|fail.*migrat' /var/log/Length of output: 156
Script:
#!/bin/bash # Search for configuration schema and related files fd -e yaml -e json config # Search for test files related to configuration fd -e yaml test fd -e json test # Search for documentation mentioning these options rg -i "share_homeassistant|share_on_port|proxy_and_funnel" -t mdLength of output: 429
Script:
#!/bin/bash # Check configuration schema cat tailscale/config.yaml # Get detailed documentation about these options rg -A 10 "Option: .share_homeassistant" tailscale/DOCS.md rg -A 10 "Option: .share_on_port" tailscale/DOCS.mdLength of output: 1933
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-homeassistant/run(2 hunks)
🔇 Additional comments (3)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-homeassistant/run (3)
5-5: LGTM! Clear and accurate documentation.The comment accurately describes the script's purpose using the new terminology.
46-46: LGTM! Clear error message with correct documentation reference.The error message has been properly updated to reference the new
share_homeassistantconfiguration option.
51-59: LGTM! Well-implemented conditional Funnel capability check.The code correctly verifies Funnel support only when needed and provides appropriate error handling.
| # Set up serve or funnel | ||
| exec /opt/tailscale $(bashio::config 'share_homeassistant') --https=$(bashio::config 'share_on_port' '443') --set-path=/ "http://127.0.0.1:$(bashio::core.port)" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for share_homeassistant value.
While the command execution is more concise, it should validate that share_homeassistant contains only allowed values ('serve' or 'funnel') before execution.
Add this validation before the command execution:
+# Validate share_homeassistant value
+if ! bashio::config.equals 'share_homeassistant' 'serve' && \
+ ! bashio::config.equals 'share_homeassistant' 'funnel'; then
+ bashio::log.error "Invalid value for share_homeassistant. Must be either 'serve' or 'funnel'"
+ bashio::exit.nok
+fi
+
# Set up serve or funnel
exec /opt/tailscale $(bashio::config 'share_homeassistant') --https=$(bashio::config 'share_on_port' '443') --set-path=/ "http://127.0.0.1:$(bashio::core.port)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Set up serve or funnel | |
| exec /opt/tailscale $(bashio::config 'share_homeassistant') --https=$(bashio::config 'share_on_port' '443') --set-path=/ "http://127.0.0.1:$(bashio::core.port)" | |
| # Validate share_homeassistant value | |
| if ! bashio::config.equals 'share_homeassistant' 'serve' && \ | |
| ! bashio::config.equals 'share_homeassistant' 'funnel'; then | |
| bashio::log.error "Invalid value for share_homeassistant. Must be either 'serve' or 'funnel'" | |
| bashio::exit.nok | |
| fi | |
| # Set up serve or funnel | |
| exec /opt/tailscale $(bashio::config 'share_homeassistant') --https=$(bashio::config 'share_on_port' '443') --set-path=/ "http://127.0.0.1:$(bashio::core.port)" |
…_and_funnel_port to share_on_port (config automatically updated)
lmagyar
force-pushed
the
pr-share-homeassistant
branch
from
2b126fb to
fc8efb5
Compare
Proposed Changes
Note: This is not a breaking change, this updates the config automatically on startup!
The standalone, dual, bool?
proxyandfunneloptions were wrong on many levels:After several naming attempt, this seems to be the most logical to me:
Screenshot:
Related Issues
fixes: #439 (and several closed old ones)
Summary by CodeRabbit
Documentation
share_homeassistantandshare_on_portsettings.funnelandproxyconfiguration options.New Features
Configuration Changes