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 NetBox 2.9.3 compatibility issues #38

Merged
merged 5 commits into from Sep 22, 2020
Merged

Fix NetBox 2.9.3 compatibility issues #38

merged 5 commits into from Sep 22, 2020

Conversation

hoanhan101
Copy link
Contributor

This PR:

@hoanhan101 hoanhan101 self-assigned this Sep 15, 2020
@edaniszewski
Copy link

The changes themselves look okay here, but just a few questions:

  1. Are all the changes here related to the 2.9.3 compatibility fix? If not, does it make sense to split the different changes into their own PRs for sake of isolation?
  2. If this makes the plugin compatible with 2.9.3, does that mean that it breaks backwards compatibility with any previous versions where the ChangeLoggedModel hadn't moved? If so, we'll need to figure out what the best practices are for pinning to versions of netbox. Generally for python, its setting the version in setup.py's install_requires field, but given that can be loaded more dynamically into netbox, I'm not sure if that necessarily works, so its something to read up on.

@hoanhan101
Copy link
Contributor Author

hoanhan101 commented Sep 16, 2020

@edaniszewski

Are all the changes here related to the 2.9.3 compatibility fix? If not, does it make sense to split the different changes into their own PRs for sake of isolation?

Yes, I believe all the changes here are related to the 2.9.3 compatibility fix. The NetBox configuration and required login models needs to be happened along with the updated ChangeLoggedModel path.

If this makes the plugin compatible with 2.9.3, does that mean that it breaks backwards compatibility with any previous versions where the ChangeLoggedModel hadn't moved?

That's a good call. The plugin will probably break our fork which is currently at v2.8.5 if one bumps it to 1.1.0. Let's see if I can find what the best practices are for pinning NetBox version. As for now, do you think having a compatibility matrix is good enough as a workaround?

@edaniszewski
Copy link

I think it probably is, but maybe just take a quick look around to see if you can quickly find anything on version pinning practices for netbox. If nothing obvious comes up, or the process to do it is more intensive than just some configuration, then yeah, a compatibility matrix is good and we can open an issue to tackle the version pinning separately.

@hoanhan101 hoanhan101 mentioned this pull request Sep 21, 2020
@hoanhan101
Copy link
Contributor Author

I'm opening #39 to tackle the version pinning separately as nothing good came up in my finding.

@hoanhan101 hoanhan101 merged commit 74d7e5a into master Sep 22, 2020
@hoanhan101 hoanhan101 deleted the fix-changelog branch September 22, 2020 20:44
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.

None yet

2 participants