Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: prefer using iproute2 instead of ifconfig #1090

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: prefer using iproute2 instead of ifconfig #1090

wants to merge 2 commits into from

Conversation

hellodword
Copy link

resolves #1089

Same with this

bash-completion/completions/wol

Line 18 in 2b7fc19

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

scop
scop reviewed Feb 1, 2024
View reviewed changes
Copy link
Owner

@scop scop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bash_completion Outdated Show resolved Hide resolved
@hellodword hellodword force-pushed the patch-1 branch 3 times, most recently from e8a84d5 to 80b3632 Compare February 5, 2024 05:35
akinomyoga
akinomyoga reviewed Feb 5, 2024
View reviewed changes
Copy link
Collaborator

@akinomyoga akinomyoga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
scop
scop requested changes Feb 8, 2024
View reviewed changes
bash_completion Outdated Show resolved Hide resolved
bash_completion Show resolved Hide resolved
@hellodword
Copy link
Author

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 akinomyoga force-pushed the patch-1 branch 3 times, most recently from ec1b4b3 to 962e9ca Compare May 13, 2024 10:52
@akinomyoga
Copy link
Collaborator

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

@akinomyoga akinomyoga changed the title chore: prefer using iproute2 instead of ifconfig fix: prefer using iproute2 instead of ifconfig May 13, 2024
@akinomyoga
Copy link
Collaborator

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
Copy link
Author

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
akinomyoga approved these changes May 14, 2024
View reviewed changes
Copy link
Collaborator

@akinomyoga akinomyoga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

yedayak
yedayak reviewed May 17, 2024
View reviewed changes
bash_completion Outdated Show resolved Hide resolved
hellodword and others added 2 commits May 21, 2024 18:00
@hellodword @akinomyoga @yedayak
fix: prefer using iproute2 instead of ifconfig
3be2efb 
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 <myoga.murase@gmail.com>
Co-authored-by: Yedaya Katsman <43016107+yedayak@users.noreply.github.com>
@akinomyoga
test(xinetd_services): mock a proper /etc/xinetd.d
8cab3b4 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akinomyoga akinomyoga akinomyoga approved these changes

@yedayak yedayak yedayak left review comments

@scop scop scop requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong network interface name if the name is long
4 participants
@hellodword @akinomyoga @scop @yedayak