Add use_napalm parameter to NetBoxInventory2 #27
Conversation
|
|
||
| self.session = requests.Session() | ||
| self.session.headers.update({"Authorization": f"Token {nb_token}"}) | ||
| self.session.verify = ssl_verify | ||
|
|
||
| def load(self) -> Inventory: | ||
|
|
||
| platforms: List[Dict[str, Any]] = [] |
There was a problem hiding this comment.
We should fence this with a if use_napalm. This shouldn't be called if there is no intention for that.
|
Thx for the work on this. We could argue that both arguments can't be "True" at the same time and raise an exception? |
That sounds good to me - if it's what you think best I'll implement it and also update the README to explain the behaviour |
| @@ -256,6 +266,10 @@ def load(self) -> Inventory: | |||
|
|
|||
| if isinstance(device["platform"], dict) and self.use_platform_slug: | |||
| platform = device["platform"].get("slug") | |||
| elif isinstance(device["platform"], dict) and self.use_napalm: | |||
| platform = [ | |||
| d for d in platforms if device["platform"]["slug"] == d["slug"] | |||
There was a problem hiding this comment.
Can you pick a better var name than d. platform makes a lot of sens to me.
README.md
Outdated
| @@ -107,6 +107,8 @@ Arguments: | |||
| (defaults to False) | |||
| use_platform_slug: Use the NetBox platform slug for the platform attribute of a Host | |||
| (defaults to False) | |||
| use_napalm: Use the Netbox platform napalm driver setting for the platform attribute | |||
There was a problem hiding this comment.
Can we rename the option to use_platform_napalm_driver. It better describes what the option exactly does
Yes, the should be "mutually exclusive". It doesn't make sense for them to be |
|
Thanks for the review suggestions - I made the changes as requested. The tests are failing on something unrelated - I also have some changes to update python versions and package dependencies which shows the tests passing, but I wanted to keep that change separate so as not to pollute this PR: |
|
Can you rebase on develop branch? That should fix the tests. The last piece I see missing is documentation: |
Raise exception if both use_platform_sluf and use_platform_napalm_driver are set to true
Hi, both are done 👍 |
|
LGTM! Thx |
|
Nice one! Thank you for the project, it's really useful 👍 |
Allow the use of napalm_driver value from the device platform to set the platform value
This could resolve #22
I wasn't certain how to deal with the priority of use_platform_slug or use_napalm, so I defaulted to use_platform_slug "winning".