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.
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
cli:ip:leases: search for NM's internal DHCP client and dhclient leases (LP: #1979674) #284
Conversation
19b4704
to
4b8baf5
Compare
1ff82e4
to
070ec61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I've added two minor comments. I think you should have a look at the os.environ.get
thing at least to confirm that this is what you originally intended.
stdout=subprocess.PIPE) | ||
for line in nmcli_dev_out.stdout: | ||
line = line.decode('utf-8') | ||
nmcli_dev_out = subprocess.check_output(['nmcli', 'dev', 'show', self.interface], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly related to the change from this PR but if you want (and if you're targetting a version of NM that has this option) you can use the following construct to simplify how the output is parsed:
nmcli --get-values GENERAL.CONNECTION dev show "$interface"
nmcli --get-values connection.uuid show id "$conn_id"
This is strictly up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good suggestion, but I'll leave it open for a bigger refactoring of ip.py
. For now, I just want to provide a minimal fix to solve the reported bug (LP#1979674).
V2: use os.defpath fallback for os.environ.get('PATH')
070ec61
to
bd99841
Compare
Description
Check for NetworkManager leases in
var/lib/NetworkManager/internal-{UUID}-{INTERFACE}.lease
and fallback tovar/lib/NetworkManager/dhclient-{UUID}-{INTERFACE}.lease
if that does not exist. Dhclient is deprecated, but might still be used in some legacy setups.Also, use some mocking (custom
MockCmd
class, as used elsewhere in our code) to actually run some unit tests on this code and increase the coverage.The whole code in
ip.py
is pretty old and in bad shape; it should be refactored, but I don't have the time for that right now, just wanted to fix LP#1979674 as a first step.Checklist
make check
successfully.make check-coverage
).