fix: prefer using iproute2 instead of ifconfig #1090

hellodword · 2024-01-08T01:49:31Z

resolves #1089

Same with this

bash-completion/completions/wol

Line 18 in 2b7fc19

ip -c=never addr show || ip addr show || ifconfig -a

scop

Could we add a comment, preferably in the commit message, about why are we making this change?

bash_completion

akinomyoga

The commit message starts with chore: (which is used for something outside the code, such as library and CI setups), but I think this is more like fix: because this fixes an actual problem #1089.

Also, a test for wol is failing.

bash_completion

hellodword · 2024-02-08T14:27:02Z

Thanks for your review and patience :)

I learned the CONTRIBUTING.md again, and I'll learn to test it locally before pushing to this pr.

akinomyoga · 2024-05-13T10:53:34Z

I rebased it and adjusted the code comment and the commit message.

akinomyoga · 2024-05-13T12:02:16Z

The test for wol is failing because the test requires 33:33:33:33:33:33, which is missing with the iproute2 implementation. The ifconfig command generates 33:33:33:33:33:33 as an HWaddr configured in test/fixtures/shared/bin/ifconfig while ip doesn't read HWaddr in test/fixtures/shared/bin/ifconfig.

hellodword · 2024-05-14T04:37:19Z

The test for wol is failing because the test requires 33:33:33:33:33:33, which is missing with the iproute2 implementation. The ifconfig command generates 33:33:33:33:33:33 as an HWaddr configured in test/fixtures/shared/bin/ifconfig while ip doesn't read HWaddr in test/fixtures/shared/bin/ifconfig.

Thanks!

So a test/fixtures/shared/bin/ip is required, right?

1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default 
    link/ether 33:33:33:33:33:33 brd ff:ff:ff:ff:ff:ff link-netnsid 0

for link show up and:

1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host 
       valid_lft forever preferred_lft forever
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default 
    link/ether 33:33:33:33:33:33 brd ff:ff:ff:ff:ff:ff link-netnsid 0
    inet 192.168.80.11/24 brd 192.168.80.255 scope global eth0
       valid_lft forever preferred_lft forever
    inet6 fe80::000:0000:0000:0000/64 scope link 
       valid_lft forever preferred_lft forever

for addr show

akinomyoga

Thank you. I added another commit to solve another issue in the test framework. I think now this is ready.

bash_completion

For `_comp_compgen_available_interfaces`, we prefer to use ip (iproute) since long interface names will be truncated by ifconfig (respective packages in the operating system, e.g. inetutils) [1]. Even for the other functions that use "ifconfig" and "ip", we change to use `ip` because `ip`'s behavior is more uniform among the systems and also `ip` is becoming more common in Linux distributions. [1]: https://github.com/scop/bash-completion/pull/1090/files Co-authored-by: Koichi Murase <[email protected]> Co-authored-by: Yedaya Katsman <[email protected]>

To test `_comp_compgen_xinetd_services`, we have been using a directory /test/fixtures/shared/bin (which contained two files `arp` and `ifconfig`as a mock /etc/xinetd.d. However, the directory /test/fixtures/shared/bin is shared with other tests, and the contents of the files therein are not proper xinetd configurations. We want to prepare a separate directory for a mock /etc/xinetd.d. This patch adds it under /test/fixtures/_comp_compgen_xinetd_services/xinetd.d.

yedayak · 2024-08-26T18:05:07Z

I rebased on origin/main

yedayak · 2024-09-01T18:06:59Z

The issue of truncation seems to only affect the inetutils version of ifconfig, but the net-tools version doesn't seem to work currently with long interfaces, presumably because it needs some changes in the awk command. I still think this should be merged, but ideally we will make a follow-up PR that fixes ifconfig where it's possible.
My testing was flawed (I tested with an interface with numbers in the name). The issue only seems to happen to the inetutils version.

akinomyoga · 2024-09-01T23:11:31Z

The issue of truncation seems to only affect the inetutils version of ifconfig, but the net-tools version doesn't seem to work currently with long interfaces, presumably because it needs some changes in the awk command. I still think this should be merged, but ideally we will make a follow-up PR that fixes ifconfig where it's possible. My testing was flawed (I tested with an interface with numbers in the name). The issue only seems to happen to the inetutils version.

Thank you. So, does that mean the issue of truncation will be totally solved by this PR? Or does it mean there is another issue with the truncation with inetutils? In the latter case, is the part "I still think this should be merged, but ideally we will make a follow-up PR that fixes ifconfig where it's possible." (which was deleted) would be still valid?

yedayak · 2024-09-02T15:32:58Z

Yeah this should totally fix the problem, I think we should merge this. Since the other ifconfig doesn't have an issue there is no need to fix it in a new PR

akinomyoga · 2024-09-02T19:35:34Z

Thank you for your clarification!

scop · 2024-11-24T22:55:33Z

Unfortunately this has fallen through the cracks based on my old "changes requested" review that has not been reset (I look through open PR's from the list view but do not generally open all of them, including those in that state). And because of that, it also missed the 2.15.0 release :/

Sorry about that, merging now.

Follow up to scop#1090

Follow up to #1090

scop reviewed Feb 1, 2024

View reviewed changes

bash_completion Outdated Show resolved Hide resolved

hellodword force-pushed the patch-1 branch 3 times, most recently from e8a84d5 to 80b3632 Compare February 5, 2024 05:35

akinomyoga reviewed Feb 5, 2024

View reviewed changes

bash_completion Outdated Show resolved Hide resolved

scop requested changes Feb 8, 2024

View reviewed changes

bash_completion Outdated Show resolved Hide resolved

bash_completion Show resolved Hide resolved

akinomyoga force-pushed the patch-1 branch 3 times, most recently from ec1b4b3 to 962e9ca Compare May 13, 2024 10:52

akinomyoga changed the title ~~chore: prefer using iproute2 instead of ifconfig~~ fix: prefer using iproute2 instead of ifconfig May 13, 2024

hellodword force-pushed the patch-1 branch from 962e9ca to a3fdc0d Compare May 14, 2024 05:46

akinomyoga approved these changes May 14, 2024

View reviewed changes

yedayak reviewed May 17, 2024

View reviewed changes

bash_completion Outdated Show resolved Hide resolved

akinomyoga force-pushed the patch-1 branch from 94408c9 to 8cab3b4 Compare May 21, 2024 09:01

hellodword and others added 2 commits August 26, 2024 20:59

yedayak force-pushed the patch-1 branch from 8cab3b4 to c2cafae Compare August 26, 2024 18:03

yedayak approved these changes Aug 26, 2024

View reviewed changes

scop mentioned this pull request Nov 24, 2024

Support completing interface alternative names #1267

Open

2 tasks

scop merged commit 3b1586b into scop:main Nov 24, 2024
7 checks passed

hellodword deleted the patch-1 branch November 24, 2024 22:57

akinomyoga mentioned this pull request Nov 26, 2024

chore: Quote changelog lines with $.*{.*$ #1273

Merged

yedayak added a commit to yedayak/bash-completion that referenced this pull request Nov 26, 2024

chore: Run shfmt on ip fixture

da34404

Follow up to scop#1090

yedayak mentioned this pull request Nov 26, 2024

chore: Run shfmt on ip fixture #1274

Merged

akinomyoga pushed a commit that referenced this pull request Nov 26, 2024

chore: Run shfmt on ip fixture (#1274)

9143322

Follow up to #1090

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prefer using iproute2 instead of ifconfig #1090

fix: prefer using iproute2 instead of ifconfig #1090

hellodword commented Jan 8, 2024

scop left a comment

akinomyoga left a comment

hellodword commented Feb 8, 2024

akinomyoga commented May 13, 2024

akinomyoga commented May 13, 2024

hellodword commented May 14, 2024

akinomyoga left a comment

yedayak commented Aug 26, 2024

yedayak commented Sep 1, 2024 •

edited

Loading

akinomyoga commented Sep 1, 2024 •

edited

Loading

yedayak commented Sep 2, 2024

akinomyoga commented Sep 2, 2024

scop commented Nov 24, 2024

fix: prefer using iproute2 instead of ifconfig #1090

fix: prefer using iproute2 instead of ifconfig #1090

Conversation

hellodword commented Jan 8, 2024

scop left a comment

Choose a reason for hiding this comment

akinomyoga left a comment

Choose a reason for hiding this comment

hellodword commented Feb 8, 2024

akinomyoga commented May 13, 2024

akinomyoga commented May 13, 2024

hellodword commented May 14, 2024

akinomyoga left a comment

Choose a reason for hiding this comment

yedayak commented Aug 26, 2024

yedayak commented Sep 1, 2024 • edited Loading

akinomyoga commented Sep 1, 2024 • edited Loading

yedayak commented Sep 2, 2024

akinomyoga commented Sep 2, 2024

scop commented Nov 24, 2024

yedayak commented Sep 1, 2024 •

edited

Loading

akinomyoga commented Sep 1, 2024 •

edited

Loading