-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix host info
for host with multiple IPs on same MAC
#338
Fix host info
for host with multiple IPs on same MAC
#338
Conversation
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.
Smooth fix. :)
mreg_cli/api/models.py
Outdated
if len(hosts) > 1: | ||
raise MultipleEntititesFound( | ||
raise MultipleEntitiesFound( | ||
f"Multiple hosts found with IP address or PTR {identifier}." |
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.
Could we hit the same error here?
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.
Yes, indeed.
mreg_cli/api/models.py
Outdated
@@ -2701,7 +2701,7 @@ def get_by_any_means( | |||
return hosts[0] | |||
|
|||
if len(hosts) > 1: |
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.
We could do this:
if len(hosts) > 1: | |
if len(hosts) > 1 and any(hosts[0].id != host.id for host in hosts): |
But should we consider refactoring the whole thing and change the method used to fetch hosts to Host.get_list_by_query_unique
, and then instead catch MultipleEntitiesFound
and raise the more granular MultipleEntitiesFound
we already have? Something like:
try:
ptr = False
ipaddress.ip_address(identifier)
host = Host.get_by_query_unique({"ipaddresses__ipaddress": identifier})
if not host:
host = Host.get_by_query_unique({"ptr_overrides__ipaddress": identifier})
ptr = True
if host:
if ptr and inform_as_ptr:
OutputManager().add_line(f"{identifier} is a PTR override for {host.name}")
return host
except MultipleEntitiesFound as e:
raise MultipleEntitiesFound(
f"Multiple hosts found with IP address or PTR {identifier}."
) from e
except ValueError: # invalid IP (should use NetworkOrIP.parse...)
pass
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.
I have pushed a commit which implements this. I do think we need more comprehensive tests for a broader range of host/ip/mac/network permutations though. I'll look into it once we have implemented #323.
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
Fixes #337
We should probably have a test in the test suite for this.