-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
fields: replace IPy dependency with ipaddress module #293
base: master
Are you sure you want to change the base?
Conversation
I ran the tests in the README and addressed the failure (python3's ipaddress module gave a different assertion). Please let know if you would like to see any additional regression tests, documentation, etc. |
We have IP addresses that don't work with ipaddress module such as:
|
I see, I didn't see #276 when I took a look at this. My fix was to check if '/' existed in the string. If no mask exists, then we just call ip_address like normal. Otherwise, we call into ip_interface, which handles those cases. Although, this won't work if we're inputting the addresses in a non-string format. If that's an issue, a simple workaround can be to guard the call to ip_address with a try: block and then call into ip_interface as a backup |
Also interesting a ipaddressfield is now available in Django but that might not work as it lacks the hostmask. |
@jelly I just rebased this if you want to take another look. Hostmasks do work in this PR now (and tests were added to avoid regressions) |
Thanks a lot, taking a look at testing the changes / impact locally! |
Tested by creating a new rsync ipv6 address in the django admin interface causes:
In https://archweb.lxd/admin/mirrors/mirror/214/change/ Same for a ipv4 address:
|
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.
Saving a mirror with a mirrorrsync ip address throws an exception
Resolves #229