network-importer icon indicating copy to clipboard operation
network-importer copied to clipboard

Upgrading to Pydantic v2 and DiffSync v2

Open theandrelima opened this issue 1 year ago • 4 comments

Overview

network-importer's dependency on pydantic = "^1.6" limits users who need to use other packages dependent on Pydantic 2.x in the same Python environment.

The original motivation behind this PR was to move network-importer's dependency to Pydantic v2.x. However, network-importer depends on DiffSync, which in turn also depends on Pydantic.

For that, moving to DiffSync v2 is also required for compatibility with Pydantic v2.

Changes Made

  • Upgraded to Pydantic v2.
  • Refactored settings classes to use pydantic-settings.
  • Applied overall code fixes for Pydantic v2 compatibility.
  • Upgraded to DiffSync v2 and made necessary code adjustments for breaking changes.
  • Proposed project version bump to 4.0.0 (subject to NTC's discretion) due to breaking changes.

Testing

  • All tests passed using the invoke pytest command. (see Additional Notes below)
  • Manual testing in CLI mode against a dev instance of our Nautobot environment confirmed the functionality both in check and apply modes, and with/without --update-configs and --limit flags.

Additional Notes

invoke pytest only worked after I added the following line to the end of the Dockerfile: RUN pip install .

I'm not sure why, but lines 14 and 15 of the current Dockerfile were not taking the effect of making network_importer itself available in the python environment of the resulting built image.

I decided to not tackle this at all within this PR because: 1 - It might be some weirdness with my own sandbox environment. 2 - Even if it is not, I believe it is out-of-scope.

theandrelima avatar Jul 04 '24 18:07 theandrelima

Looks like this is failing because of the removed docstrings in BatfishSettings and NetworkSettings classes:

./network_importer/config.py:41 in public class `BatfishSettings`:
        D101: Missing docstring in public class
./network_importer/config.py:51 in public class `NetworkSettings`:
        D101: Missing docstring in public class

msheiny avatar Dec 24 '24 00:12 msheiny

I'm onboard with getting this merged in and let's then take a pass at getting the rest updated.

For that update (not this one), @theandrelima would we be good to update Python minimums in your environment? Only because Pandas and Numpy are aggressively moving forward.

jvanderaa avatar Dec 24 '24 15:12 jvanderaa

I'm onboard with getting this merged in and let's then take a pass at getting the rest updated.

For that update (not this one), @theandrelima would we be good to update Python minimums in your environment? Only because Pandas and Numpy are aggressively moving forward.

I was off for a couple of weeks. Back now and happy to see that this might get merged soon, hopefully?

All right! tWill do. But just to confirm, that is to happen in a separate PR, right?

theandrelima avatar Jan 01 '25 22:01 theandrelima

It seems the idea is to deprecate network-importer as a standalone package in favour of the new onboarding plugin, which will include network importing functionality too.

Whereas that is nice, I'm a big proponent of an independent NI package that can be integrated in whichever way best suits each use case. For example, I currently would love to have NI as a service with a backend REST API, dedicated workers, and running independently of Netbox/Nautobot app, while updating it.

I'll keep my own version of this package, possibly with a different name too. Hope that's not a problem...

cheers... 🍻

theandrelima avatar May 19 '25 19:05 theandrelima