Add PNG path functionality for IP Fabric v4 #61
Conversation
| ] | ||
| ) | ||
|
|
||
| # only supported in IP Fabric OS version 4.0+ |
There was a problem hiding this comment.
it looks to me that we could implement a generic version validation function, as the product evolves, more cases like this will arise
There was a problem hiding this comment.
yes I'd like to get some feedback on how we could approach version management.
There was a problem hiding this comment.
Anyone who is currently maintaining a library that is dependent on a specific API/software library and it's versioning. I was hoping there is a preferred approach for this kind of thing. We're not the first to do it!
There was a problem hiding this comment.
Mentioned during the EU call and the suggestion was to drop the old command in the next release and only support v4 in our next release. Makes it easier to maintain and allows users to use two versions. What do you think?
There was a problem hiding this comment.
if the approach is to only support latest IPfabric version we could go this way... but this is a question that we will need to answer more times, and not sure if losing support to "old" ipfabric versions is a good strategy (when there are just some differences).
If we opt for the support for only one version support we have to document it well
There was a problem hiding this comment.
Yes, I guess it's a trade off between maintenance vs functionality. It's harder and more time-consuming to maintain and test multiple versions, especially in our environment. But it's better for users to have greater support.
There was a problem hiding this comment.
I agree, it's not the right strategy for function. But it probably is the right strategy for maintenance.
There was a problem hiding this comment.
go ahead for now, we can implement it later if needed, before deprectaing
There was a problem hiding this comment.
Added a generic validation function for now. I agree, it'll come in handy.
nautobot_chatops_ipfabric/worker.py
Outdated
| # only supported in IP Fabric OS version 4.0+ | ||
| response = ipfabric_api.get_os_version() | ||
| os_version = float(response.get("version", "0.0").rpartition(".")[0]) | ||
| if os_version >= 4: |
There was a problem hiding this comment.
regarding version handling, I was thinking about another potential approach and I would like to get your feedback.
Your proposal is:
- if os_version <x -> command_1
- if os_version >= x -> command_2
what about:
- command and behind the scenes we check the os_version and decide which logic to use? (this works well if the input data is the same)
There was a problem hiding this comment.
I'm totally open to a better solution. Originally I had the logic in the end-to-end-path command. However the new v4 OS names the path simulation a different name with a different URL. Also, we could extend the new path lookup for JSON and SVG output if desired. I felt it would be adding complexity, for the short term. But perhaps I'm not understanding your proposal?
There was a problem hiding this comment.
my idea would be to expose the user with only one command: the_very_best_e2e_path
Inside this we would have a version if statement that would call the specific logic that you need for end-to-end-path and pathlookup (data input if there are changes, and the actual output)
The goal is to make the user experience "unique", so the user don' have to understand there are two implementations, there is one with different behaviour
There was a problem hiding this comment.
Ideally we'll extend the pathlookup command with JSON capability using an additional argument for output format. Unfortunately the JSON result for the new version and old version are completely different :-(
So the only thing that's similar is the input variables which, will change very soon anyway.
|
@chadell if you get the chance, can you check the CHANGELOG, open to a better approach. |
CHANGELOG.md
Outdated
| @@ -1,4 +1,16 @@ | |||
|
|
|||
| ## v1.1.0 - 2022-MM-DD | |||
|
|
|||
| ### Announcement | |||
There was a problem hiding this comment.
| ### Announcement | |
| ### Deprecated |
|
Oh good tip, thanks! |
This PR will add a new command that is supported in IP Fabric v4 to render a path simulation as a PNG.
I decided to separate this
pathlookupcommand from the existingend-to-end-pathcommand due to the different naming, URL and potential outputs for diagrams in v4. Also it will allow us to retire theend-to-end-pathcommand easier.