-
Notifications
You must be signed in to change notification settings - Fork 177
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
support OFFICE_NETWORKS and format title with [] #48
Conversation
|
||
return False | ||
return any(non_office_logins) |
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.
If I'm understanding this correctly, we're creating a list of bools and then checking for any True values in that list? Makes more sense (and more efficient) to just return True immediately when we find one:
for office_network in OFFICE_NETWORKS:
# Check the case where the OFFICE_NETWORK is a /32
if office_network.num_addresses == 1 and office_network.network_address != host_ipaddr:
return True
elif host_ipaddr not in office_network.hosts():
return True
return False
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 think we actually want to have return not any(non_office_logins)
, because as long as the IP address is from one of the office IP spaces we're happy right? So maybe:
for office_network in OFFICE_NETWORKS:
# Check the case where the OFFICE_NETWORK is a /32
if office_network.num_addresses == 1 and office_network.network_address == host_ipaddr:
return False
elif host_ipaddr in office_network.hosts():
return False
return True
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.
One minor suggestion.
for office_network in OFFICE_NETWORKS: | ||
# Check the case where the OFFICE_NETWORK is a /32 | ||
if office_network.num_addresses == 1: | ||
non_office_logins.append( | ||
office_network.network_address != host_ipaddr) | ||
# Check all possible hosts in the office subnet | ||
else: | ||
non_office_logins.append(host_ipaddr not in office_network.hosts()) |
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.
If you just check for host_ipaddr not in office_network
on as opposed to host_ipaddr not in office_network.hosts()
on line 28, you don't need the special case for a /32:
>>> import ipaddress
>>> office_networks = [ipaddress.ip_network('192.168.1.100/32'), ipaddress.ip_network('192.168.1.0/24')]
>>> host_ipaddr = ipaddress.IPv4Address('192.168.1.100')
>>> for office_network in office_networks:
... print(host_ipaddr in office_network)
...
True
True
>>>
I'm not sure which one if more memory/time efficient under the hood for large subnets, I imagine they are about similar though. But I think without the special case the code is more clear.
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.
Also holds true for public IP address spaces:
>>> office_networks = [ipaddress.ip_network('100.100.100.100/32'), ipaddress.ip_network('100.100.100.0/24')]
>>> host_ipaddr = ipaddress.IPv4Address('100.100.100.100')
>>> for office_network in office_networks:
... print(host_ipaddr in office_network)
...
True
True
>>>
d8c27b2
to
d46d8ff
Compare
d46d8ff
to
5f3e911
Compare
* add a check for invalid json/yaml, add file with errors to the invalid specs list * remove unnecessary else * convert to ruamel, which more strictly conforms to yaml spec * replace pyyaml with ruamel in setup script * remove duplicate error check
Background
Just doing some tuning
Changes
Testing