fix(cloudstack): Improve domain-name DHCP lease lookup (Cloudstack) #6554
Conversation
* Adding case-insensitive options for systemd-networkd leases ("DOMAINNAME", "Domain", "domain-name").
* Falling back gracefully from systemd leases to ISC dhclient leases.
* Including dhcpcd ephemeral leases as an additional fallback.
* Returning an empty string when no domain name found instead of None for non-fatal missing cases.
|
@TheRealFalcon @holmanb Not sure why the CLA check is failing. I'm pretty sure I signed a CLA a long time ago. Here is another PR that I've commited and was merged in cloud-init : #4660 |
|
@CodeBleu You'll need to sign the new CLA, sorry. |
|
@holmanb the CLA was signed by my company. I closed and re-opend the PR, but it appears it's still not validating my CLA check. Not sure if we should get an "approved" email after submitting the CLA or not? |
Thanks for signing. I think the email address associated with your github account would need to match the company domain. It looks like the check failed with a github.com domain -> |
The whole reason to have that email in github is to not actually show my email! 😄 The email domain that is used to sign the CLA is a valid email I have under my github account. There should be a way to have the CLA still validate it with being able to use the obscure email that Github provides. |
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Thank you @CodeBleu for continuing to contribute to cloud-init and making it better on more platforms.
I've pinged internally the right persons to attempt to sort what is missing in CLA validation. In the meantime, here's a review which hopefully gets us to landing closer to landing this branch. We'd definitely like to see some unit test coverage of Domain and domain-name values as well as NoDHCPLeaseError condition being raised.
Other than that. this looks good to me.
| # cloud-init set up. | ||
| with suppress(FileNotFoundError): | ||
| with suppress( | ||
| Exception |
There was a problem hiding this comment.
Generally, we'd like to track known Exception types, not blanket catch all Exceptions as that can hide incorrect assumptions about the expected state of the code. We want to fail hard with a traceback if there is an unrecognized exception type raised instead of emitting a debug message and continuing because rarely does anyone read the debug messages.
| Exception | |
| NoDHCPLeaseError, FileNotFoundError |
I like the support for other DHCP option keys, can we add parametrized unittests for _get_domainname to tests/unittests/sources/test_cloudstack.py::TestCloudStackHostname too to cover the new support paths. We don't want to degrade test coverage where we can avoid it so we don't break this behavior in the future.
3 participants
Proposed Commit Message
Additional Context
Test Steps
Merge type