Skip to content
This repository has been archived by the owner on Feb 10, 2018. It is now read-only.

Enhance the cli method to allow piping #188

Merged
merged 4 commits into from
Jul 25, 2017
Merged

Enhance the cli method to allow piping #188

merged 4 commits into from
Jul 25, 2017

Conversation

mirceaulinic
Copy link
Member

One of the most common problems when using the cli method in napalm-junos is that Junos does not process the pipes.
If we are anyway at enhancing the cli (#186) and align it to the behaviour with the other drivers (which can do piping), this PR addresses to process the piping inside napalm-junos.

Ping @sincerywaing this might interest you as well.
Thoughts?

@coveralls
Copy link

coveralls commented Jul 17, 2017

Coverage Status

Coverage decreased (-3.5%) to 80.064% when pulling ef79b77 on cli-enhance into e6519c6 on develop.

@coveralls
Copy link

coveralls commented Jul 17, 2017

Coverage Status

Coverage decreased (-4.06%) to 79.555% when pulling 3bcef17 on cli-enhance into e6519c6 on develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-4.06%) to 79.555% when pulling 3bcef17 on cli-enhance into e6519c6 on develop.

@dbarrosop
Copy link
Member

Aren't we overdoing it? Seems like a lot of code for something that you are not even supposed to be using in the first place as this break the whole assumption that napalm is vendor agnostic.

@mirceaulinic
Copy link
Member Author

Seems like a lot of code for something that you are not even supposed to be using in the first place as this break the whole assumption that napalm is vendor agnostic.

The cli method itself is not vendor agnostic in the first place.
However, it turns useful sometimes, and there's a real-world demand for features that are not covered yet by napalm (and there are quite a few). Some others won't ever be supported anyway, e.g. show processes extensive | match netconf.

@sincerywaing
Copy link
Contributor

sincerywaing commented Jul 18, 2017 via email

@dbarrosop
Copy link
Member

dbarrosop commented Jul 18, 2017

Yes, I am fine with the cli command. I just want to make sure we don't overdo it. We are adding +100 lines of code to a method that is "nice to have" only to workaround something the vendor decided not to implement. If this is such an important feature, why is people asking us instead of asking them?

and there's a real-world demand for features that are not covered

For example? I'd expect a bunch of issues requesting features backing that up ;)
Or even PRs, if they are using this method to do something useful with it I'd expect people to be parsing the output themselves. Or are they using napalm just to build an alternative for pssh?

@dbarrosop
Copy link
Member

I have approved the PR as I mostly wanted to give some food for thought. Feel free to merge if you decide so after fixing the small CI issue.

@coveralls
Copy link

coveralls commented Jul 25, 2017

Coverage Status

Coverage decreased (-5.06%) to 78.556% when pulling a8330e8 on cli-enhance into 8b1f0b8 on develop.

@mirceaulinic mirceaulinic merged commit ea10edd into develop Jul 25, 2017
@mirceaulinic mirceaulinic deleted the cli-enhance branch July 25, 2017 16:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants