Fix IPv4 and IPv6 output in Armbian welcome MOTD #7911
Conversation
|
Thank you for addressing this problem. Current way is not the best and we also have a Jira ticket on the topic. https://armbian.atlassian.net/browse/AR-2571 I tried changed script and few of our production servers: What stand out is:
The other example machine: IPv4 addresses: (LAN) 100.100.100.100, XX.YYY.XX (WAN) 100.100.100.100 Here we also have unnecessary duplication. We need to shuffle the logic around a bit - space for displaying is very limited, so its important we try our best. The other issue is cosmetic nature - aligning data into columns. Now, IP sticks out as label text is longer. Thank you! |
|
VPS where we have one IP address that is local and global, only ipv6 is local and global. |
WalkthroughThe changes introduce a new configuration variable, Poem
🪧 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.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/bsp/common/etc/update-motd.d/10-armbian-header (5)
194-195: Potential command substitution issue in sed replacementThe current implementation uses unquoted variables in the sed command which could lead to issues if the IP address contains special characters that need escaping.
Consider using a more robust approach:
- ipv4s=$(echo $ipv4s | sed "s/"${wan_ip_address}"//g" | xargs ) + ipv4s=$(echo "$ipv4s" | sed "s/$(echo "$wan_ip_address" | sed 's/[\/&]/\\&/g')//g" | xargs)
209-210: Same sed replacement issue for IPv6 addressesSimilar to the IPv4 section, the unquoted variables in the sed command could cause issues with IPv6 addresses which contain more special characters.
Consider using a more robust approach:
- ipv6s=$(echo $ipv6s | sed "s/"${wan_ip6_address}"//g" | xargs ) + ipv6s=$(echo "$ipv6s" | sed "s/$(echo "$wan_ip6_address" | sed 's/[\/&]/\\&/g')//g" | xargs)
214-214: Fix alignment issue for IPv6 labelThe IPv6 label has an extra space compared to the IPv4 label, which could cause misalignment in the output.
- all_ip6_address=" IPv6: " + all_ip6_address=" IPv6: "
200-201: Consider adding conditional label for LAN addressesThe current implementation always adds the "(LAN)" label even if there's only one address, which might be unnecessary.
Consider a more concise approach:
- if [[ -n ${ipv4s} ]]; then - all_ip_address+=" \x1B[93m(LAN)\x1B[0m " - fi + [[ -n ${ipv4s} && $(echo "$ipv4s" | wc -w) -gt 1 ]] && all_ip_address+=" \x1B[93m(LAN)\x1B[0m "
89-91: Improve HIDE_LOCAL_IPV6 variable checkThe current implementation checks for the literal string "true" but it would be better to handle different possible inputs.
- if [[ "${HIDE_LOCAL_IPV6}" == true ]]; then + if [[ "${HIDE_LOCAL_IPV6}" == "true" || "${HIDE_LOCAL_IPV6}" == "yes" || "${HIDE_LOCAL_IPV6}" == "1" ]]; thenThis would make the configuration more user-friendly by accepting different common ways to express a boolean true value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bsp/common/etc/default/armbian-motd.dpkg-dist(1 hunks)packages/bsp/common/etc/update-motd.d/10-armbian-header(3 hunks)
🔇 Additional comments (12)
packages/bsp/common/etc/default/armbian-motd.dpkg-dist (1)
10-10: Good addition of configuration variableAdding
HIDE_LOCAL_IPV6="true"as a configurable option aligns with the PR objectives to allow users to hide local IPv6 addresses, especially useful for systems with numerous containers.packages/bsp/common/etc/update-motd.d/10-armbian-header (11)
15-15: Default setting for IPv6 address display controlThis establishes the default setting for hiding local IPv6 addresses, maintaining consistency with the configuration in armbian-motd.dpkg-dist.
35-38: Improved distribution detection logicThese changes enhance the robustness of distribution detection by:
- Adding proper variable scoping with braces
- Ensuring logical progression from one detection method to another
- Maintaining consistent variable naming and quoting
This makes the code more maintainable and readable.
44-50: Enhanced upgrade filter parsingThe improved formatting with proper variable quoting and consistent indentation makes this section more readable while maintaining the same functionality.
53-56: Consistent variable usage in script disabling logicThe addition of braces around variables provides more consistent syntax throughout the script.
58-60: Explicit IPv4 specification in WAN address retrievalAdding the
-4flag to curl ensures the function explicitly requests an IPv4 address, which aligns with the function's purpose.
62-64: New WAN IPv6 address retrieval functionGood addition of a dedicated function for fetching WAN IPv6 addresses, following the same pattern as the IPv4 function. This modular approach improves code organization and maintainability.
68-75: Improved variable declarationsBetter organization of variable declarations with more descriptive naming makes the function more readable and maintainable.
77-102: Enhanced IP address collection logicThis section includes several improvements:
- Better variable quoting for reliability
- Implementation of the
HIDE_LOCAL_IPV6flag to filter out link-local addresses- Consistent handling of both IPv4 and IPv6 addresses
- Use of arrays for cleaner collection of addresses
These changes address the key objectives of the PR by ensuring all relevant addresses are collected while filtering out unwanted ones.
114-114: Added WAN IPv6 address retrievalThis complements the existing WAN IPv4 address retrieval, enabling the script to display outgoing WAN IPv6 addresses as requested in the PR objectives.
191-207: Improved IPv4 address display logicThe new display logic for IPv4 addresses:
- Separates LAN and WAN addresses clearly with labels
- Removes WAN address from the LAN address list to avoid duplication
- Uses color coding for better visual distinction
This implementation addresses the PR objective of showing all available LAN IPv4 addresses while avoiding duplication.
208-222: New IPv6 address display implementationThis section implements a dedicated display for IPv6 addresses that:
- Removes WAN IPv6 from the list of local addresses
- Only displays IPv6 section when addresses are available
- Uses clear labeling and color coding to distinguish LAN and WAN addresses
This addresses the PR objective of displaying all available IPv6 addresses and sorting out duplicates.
Description
Now "Armbian welcome MOTD" only prints one LAN IPv4 address and does not print any IPv6 address if you have more than one.
I changed to print all IPv4 and IPv6 addresses. I also made some cosmetic changes (based on .editorconfig from this repository).
Added by @igorpecovnik
How Has This Been Tested?
Checklist:
Please delete options that are not relevant.