Allow --primary-interface matter server arg#4092
Allow --primary-interface matter server arg#4092csuich2 wants to merge 2 commits intohome-assistant:masterfrom
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
📝 WalkthroughWalkthroughThe script handling the startup of the Matter server now appends user-provided arguments earlier and checks for a user-specified primary network interface. If found, it skips automatic detection; otherwise, it detects the interface as before. This prevents duplication and respects user configuration. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
matter_server/rootfs/etc/s6-overlay/s6-rc.d/matter-server/run(1 hunks)
🔇 Additional comments (2)
matter_server/rootfs/etc/s6-overlay/s6-rc.d/matter-server/run (2)
56-59: LGTM! Good refactoring to enable user argument inspection.Moving the
matter_server_argsconfiguration earlier in the flow is essential for the new logic to work correctly. This allows the script to inspect user-provided arguments before performing auto-detection.
66-79: LGTM! Well-structured conditional auto-detection logic.The auto-detection logic is properly wrapped in the else block and maintains all the existing functionality:
- Primary method via supervisor API
- Fallback method using
ip route- Proper error handling with descriptive messages
- Defensive programming with null checks
This ensures backward compatibility while respecting user configuration.
| # Only determine primary_interface if not already specified in matter_server_args (from config) | ||
| primary_interface_pattern=" --primary-interface (.*) " | ||
| if [[ " ${extra_args[*]} " =~ $primary_interface_pattern ]]; then | ||
| bashio::log.info "Using primary interface from config: ${BASH_REMATCH[1]}." | ||
| else |
There was a problem hiding this comment.
Fix the regex pattern to handle edge cases correctly.
The current regex pattern " --primary-interface (.*) " has several potential issues:
- Trailing space requirement: The pattern requires a trailing space, but the last argument won't have one
- Greedy matching: The
(.*)capture group could match multiple arguments if they exist after--primary-interface
Apply this diff to make the pattern more robust:
-primary_interface_pattern=" --primary-interface (.*) "
+primary_interface_pattern="--primary-interface[[:space:]]+([^[:space:]]+)"This pattern:
- Doesn't require surrounding spaces (works at start/end of argument list)
- Uses
[[:space:]]+to match one or more whitespace characters - Uses
[^[:space:]]+to match the interface name (non-whitespace characters only)
📝 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.
| # Only determine primary_interface if not already specified in matter_server_args (from config) | |
| primary_interface_pattern=" --primary-interface (.*) " | |
| if [[ " ${extra_args[*]} " =~ $primary_interface_pattern ]]; then | |
| bashio::log.info "Using primary interface from config: ${BASH_REMATCH[1]}." | |
| else | |
| # Only determine primary_interface if not already specified in matter_server_args (from config) | |
| primary_interface_pattern="--primary-interface[[:space:]]+([^[:space:]]+)" | |
| if [[ " ${extra_args[*]} " =~ $primary_interface_pattern ]]; then | |
| bashio::log.info "Using primary interface from config: ${BASH_REMATCH[1]}." | |
| else |
🤖 Prompt for AI Agents
In matter_server/rootfs/etc/s6-overlay/s6-rc.d/matter-server/run around lines 61
to 65, the regex pattern used to detect the primary interface argument is too
permissive and requires a trailing space, which may not exist for the last
argument. Update the regex to remove the trailing space requirement, use
`[[:space:]]+` to match one or more whitespace characters before the interface
name, and replace `(.*)` with `[^[:space:]]+` to capture only the interface name
without trailing arguments, ensuring more accurate and robust matching.
There was a problem hiding this comment.
- Is intentional since the test pattern joins all array entries with spaces.
- This is reasonable. I changed the regex to capture
([^\s]*)
There was a problem hiding this comment.
@csuich2 Thank you for the clarification and the fix!
You're absolutely right about point 1 - since " ${extra_args[*]} " joins the array elements with spaces, the surrounding spaces in the pattern are indeed intentional and necessary for proper matching.
Your change from (.*) to ([^\s]*) perfectly addresses the greedy matching concern. This ensures that only the interface name itself is captured, stopping at the first whitespace character, which is exactly what we need. The [^\s]* approach is concise and effective for this use case.
The fix looks good - it maintains the intentional space-padding logic while preventing the regex from capturing beyond the interface name.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
Duplicate? #4070 |
|
Doh! It sure is. I'm happy to close in favor of that PR if we prefer that approach. |
|
I just double checked, the primary interface argument to the Python Matter Server works as expected. See also the note at #4070 (comment). However, there is something to note here: There is also a message about the primary Ethernet interface on startup printed by the SDK: This message has no significance for the Matter Server. It is used as a source for diagnostic information for Matter devices (see |
3 participants
The "--primary-interface" argument is always ignored since the resolved interface is always specified first in the matter server args. This PR uses the specified interface and only resolves the primary interface if one isn't specified.
Summary by CodeRabbit