This repository has been archived by the owner on Sep 7, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add PNG path functionality for IP Fabric v4 #61
Add PNG path functionality for IP Fabric v4 #61
Changes from 1 commit
4706ca7
0a54269
ba8f2e0
d7b1776
89f2284
256a9cc
03ee356
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I'd like to get some feedback on how we could approach version management.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feedback from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go ahead for now, we can implement it later if needed, before deprectaing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a generic validation function for now. I agree, it'll come in handy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regarding version handling, I was thinking about another potential approach and I would like to get your feedback.
Your proposal is:
what about:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 forend-to-end-path
andpathlookup
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.