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

fix: tag management with netbox >= 2.9 #307

Closed
wants to merge 3 commits into from
Closed

fix: tag management with netbox >= 2.9 #307

wants to merge 3 commits into from

Conversation

jbe-dw
Copy link

@jbe-dw jbe-dw commented Dec 14, 2020

Hello,

This PR is made with the purpose of keeping backward compatibility with previous versions. There's in my opinion another way arround I though about: we can make tags membership of LIST_AS_SET conditional to the API version and keep it the way it was.

This would force end users to first create or retrieve tags objects in or from Netbox API and use the records in the append() function. In this case #291 would just need a version check conditional if i'm correct. But it wouldn't work when user provides a dictionnary.

@zachmoody
Copy link
Contributor

Hey, thanks for the PR. I'm a little hesitant to take this approach because (I think) you'll end up beating up the endpoint we're getting the version from if you have a lot of changes. We could cache that value, but I'd like to explore just inspecting the elements of tags and handling them based on their type. I hope to have some time next week to dig into that solution, if folks can be patient. If that doesn't pan out, let's cache the version and merge this one.

@markkuleinio
Copy link
Contributor

I support the idea of inspecting the tag object type and acting accordingly.

Also, instead of caching the NetBox API version (if needed frequently for any reason, internal to pynetbox or for the application reasons) in the first call, I would like to investigate an option to keep an updated API version attribute in the pynetbox.Api instance so that from every NetBox response header we would update (if needed) the API version attribute so that we always have an updated NetBox API version at hand. That way we would not introduce a dependency to restart each and every (possibly remote) application component whenever we update the NetBox server, which would be needed if we cache the API version unconditionally. Restarting apps is not a problem in small deployments or when a script runs separate runs, but for example I have various webhook handlers and DHCP automation modules that are running all the time on different platforms, and keeping the running apps synchronized with various changes is getting harder. Time-based caches are not significantly better.

@zachmoody
Copy link
Contributor

💯 , that would be nice.

@jbe-dw
Copy link
Author

jbe-dw commented Dec 18, 2020

Hello

I didn't update the pytest yet but I made some changes to catch the api version from headers after each calls. The changes don't look elegant to me and may probably have a more pythonic version in your mind.

Tell me about the changes, I didn't spend a lot of time for this, I'm still getting confident that you'll soon find some time to do the fix. If you wish an improvement (and make CI green again) to release this fix because you lack of time let me know.

Regards.

PS: @zachmoody I let you handle the object inspection/comparison if you wish later as well

@markkuleinio
Copy link
Contributor

See also #311

@zachmoody
Copy link
Contributor

Thanks again for the PR, but ended up opting for the approach in #311.

@zachmoody zachmoody closed this Dec 24, 2020
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.

3 participants