add option -i for ips to display interface(s) #1004
Conversation
There was a problem hiding this comment.
Please take a look at the review comments.
For the tests: Call me paranoid, but I'm not going to download a random zip file that someone I don't know has added to a ticket 😄
Please take a look at the test/plugins/base.plugin.bats file - there is an existing test for the ips function. Please add additional tests that cover the new functionality there, so they are run on every commit (using Travis CI).
| @@ -4,13 +4,24 @@ about-plugin 'miscellaneous tools' | |||
| function ips () | |||
| { | |||
| about 'display all ip addresses for this host' | |||
| param 'optional -i' | |||
There was a problem hiding this comment.
Please provide an explanation for what this parameter does.
There was a problem hiding this comment.
Could I write "param 'optional -i , display the interfaces and the corresponding ip addresses ' "? or other way to do it?
There was a problem hiding this comment.
I would write something like param '-i: (optional) display the interfaces and the corresponding ip addresses
| group 'base' | ||
| if command -v ifconfig &>/dev/null | ||
| then | ||
| ifconfig | awk '/inet /{ gsub(/addr:/, ""); print $2 }' | ||
| if [ -n "$1" -a "$1" = "-i" ];then | ||
| ifconfig | grep -B 1 -E "inet "| sed 's/addr://g' | sed 's/: / /g' | awk '{if (NR%3==1) print $1 ":"; else if (NR%3==2) print $2}' |
There was a problem hiding this comment.
Do you really have to run grep, sed (twice) and awk all in the same line? I'm sure there's got to be a way that does not use three different tools to accomplish this.
There was a problem hiding this comment.
Beacause running the ifconfig command get different results on different OS.
on Centos:
lo Link encap:Local Loopback
inet addr:127.0.0.1 Mask:255.0.0.0
inet6 addr: ::1/128 Scope:Host
UP LOOPBACK RUNNING MTU:16436 Metric:1
RX packets:5302709 errors:0 dropped:0 overruns:0 frame:0
TX packets:5302709 errors:0 dropped:0 overruns:0 carrier:0
collisions:0 txqueuelen:0
RX bytes:253434162 (241.6 MiB) TX bytes:253434162 (241.6 MiB)
On Ubuntu:
lo: flags=73<UP,LOOPBACK,RUNNING> mtu 65536
inet 127.0.0.1 netmask 255.0.0.0
inet6 ::1 prefixlen 128 scopeid 0x10
loop txqueuelen 1000 (Local Loopback)
RX packets 390 bytes 29016 (29.0 KB)
RX errors 0 dropped 0 overruns 0 frame 0
TX packets 390 bytes 29016 (29.0 KB)
TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0
There was a problem hiding this comment.
Yes, I understand that, but ideally you don't need to run three different tools to parse the output. What you're doing with grep/sed/awk can probably be done with just awk alone.
| elif command -v ip &>/dev/null | ||
| then | ||
| ip addr | grep -oP 'inet \K[\d.]+' | ||
| if [ -n "$1" -a "$1" = "-i" ];then | ||
| ip addr | grep -E '^\s*[1-9]| (25[0-5]|2[0-4][0-9]|[0-1][0-9]{2}|[1-9][0-9]|[1-9])\.(25[0-5]|2[0-4][0-9]|[0-1][0-9]{2}|[0-9]{1,2})\.(25[0-5]|2[0-4][0-9]|[0-1][0-9]{2}|[0-9]{1,2})\.(25[0-5]|2[0-4][0-9]|[0-1][0-9]{2}|[0-9]{1,2})' | awk '{print $2}' |
There was a problem hiding this comment.
I'm sorry, but I don't even want to try to understand what this line does 😄
Can you please provide some explanation what this does? Ideally in a code comment...
Anyway, this looks far too complex.
There was a problem hiding this comment.
I will added a code comment.
ip addr --- running ip addr to display all interfaces and ip addresses on the device.
the result:
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 16436 qdisc noqueue state UNKNOWN
link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
inet 127.0.0.1/8 scope host lo
inet 3.3.3.29/24 brd 3.3.3.255 scope global lo:1
inet 3.3.3.30/24 brd 3.3.3.255 scope global secondary lo:2
inet6 ::1/128 scope host
valid_lft forever preferred_lft forever
21: gre0: mtu 1476 qdisc noop state DOWN
link/gre 2.2.2.2 brd 2.2.2.1
grep command --- filter line 1 and 3,4,5 in lo device list. in gre0 device list, filter line 1 and 2.
awk command --- print lo: or ip addresses. print gre0: or ip addresses
the result of the ips -i:
lo:
127.0.0.1/8
3.3.3.29/24
3.3.3.30/24
gre0:
2.2.2.2
There was a problem hiding this comment.
Sorry, but there has got to be a simpler way for doing that...
|
I don't want to mege this PR. beacause these two commands are so dependent on the OS. The code is difficult to cover all cases. the below comments are my cases. |
|
the result of running "ifconfig" on CentOS: eth1 Link encap:Ethernet HWaddr 00:0C:29:8B:5F:39 eth2 Link encap:Ethernet HWaddr 00:0C:29:8B:5F:4D eth3 Link encap:Ethernet HWaddr 00:0C:29:8B:5F:43 gre2 Link encap:UNSPEC HWaddr 02-02-02-02-FF-FF-E0-E6-00-00-00-00-00-00-00-00 lo Link encap:Local Loopback lo:1 Link encap:Local Loopback lo:2 Link encap:Local Loopback tun_10_45 Link encap:UNSPEC HWaddr 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00 tun_10_47 Link encap:UNSPEC HWaddr 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00 |
|
the result of running "ifconfig" on Ubuntu: lo: flags=73<UP,LOOPBACK,RUNNING> mtu 65536 vmnet1: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500 vmnet8: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500 wlp7s0: flags=4099<UP,BROADCAST,MULTICAST> mtu 1500 |
|
the result of running "ip addr" on CentOS: |
|
the result of running "ip addr" on Ubuntu: |
|
Sorry, not sure what to do with these comments. Before merging this, we need to have some assurance that the changes work reasonably well on the major operating systems. Please let me know how you want to proceed. |
|
I will try to modify as your comments to make sure this command run well on the major OS. |
|
@YumingMa want to take another stab at this? |
|
If this isn't picked back up, I vote this function be removed or moved to a plugin called "misc", or something (open to suggestions. maybe "networking"?). Relates to #1640 My argument is that this function is too opinionated to be worth keeping. @nwinkler @NoahGorny @davidpfarrell Edit: as for the code, if people really feel compelled to keep this function around, I recommend simplifying this: if _command_exists ip ; then
ip address | awk '
/^[0-9]+:/ { sub(/:/,"",$2) ; iface=$2 }
/^[[:space:]]*inet / {
split($2, a, "/")
print iface" : "a[1]
}'
elif _command_exists ifconfig ; then
ifconfig | awk '
/^[a-z]/ { iface=$1 }
/inet addr:/ {
sub(/inet addr:/, "")
print iface" : "$1
}'
else
>&2 echo 'iproute and ifconfig not found! unable to obtain ips!"
return 1
fiNote: this would require some changes to support ipv6 too, but it's pretty easy. |
I also think that this should be removed from the base plugins. I could see it being its own stand-alone plugin. I don't know how popular it is, but there's only one use within bash-it itself (not counting the base plugin test). We'd probably want to update that theme to include a Side note: Additionally, the phoenix alias set creates an As for the code: It does seem like it could be simplified - I'd probably consider that a post-move action, separated from the act of moving this into its own plugin. |
|
I'm fine with moving it to a separate plugin. I'm not a huge fan of a misc plugin, since it will simply become another collection of odds and ends without any real purpose. Maybe a network plugin? The usage in the theme is a bit problematic anyway, the theme relies on the function to be available. At the moment, that's OK, since most people have the |
|
closing this as this is stale |
I have made a lot of test with the files in the attachment. Please review the code.
Edit (nwinkler): Removed zip attachment