Skip to content
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

some adaptation for netbox 3.7 #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Cormoran96
Copy link

modify script to output only geoloc = True and None

feed += entry['prefix'] + ',' + country + ',' + region + ',' + city + ',\n'
i += 1

elif fields['geoloc_has_location'] == None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure the field can never be null if it's required, only true or false. Shouldn't this be elif not fields['geoloc_has_location']: (or simply else)?
image

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if you have a fresh install and an empty netbox when you install the plugin. If, like me, your netbox is already full, the prefixes are not completed with the info geloloc true or false. As a consequence, they have a null value

Copy link
Member

@NoahvdAa NoahvdAa Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that makes sense. IMO it would make more sense to trigger the logic for when has_location is true when a null value is encountered, so the prefix inherits its parent's geoloc (covered by the check if all location values are null).
The check highlighted here should still be changed to check for false (or just a simple else if null means inheriting), since there is currently no way to specify that a prefix has no location (which is what the ,,,, line is exclusively for).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants