napalm-automation / napalm-junos

Apache License 2.0
22 stars 42 forks source link

Enhance the cli method to allow piping #188

Closed mirceaulinic closed 7 years ago

mirceaulinic commented 7 years ago

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 (https://github.com/napalm-automation/napalm-junos/pull/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 commented 7 years ago

Coverage Status

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

coveralls commented 7 years ago

Coverage Status

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

coveralls commented 7 years ago

Coverage Status

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

dbarrosop commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

My easy assumption is that we can't put all the shows into functions in napalm, so we leave a 'back door' thus people can manipulate with, although it's not elegant.

Sent from my iPhone

On 18 Jul 2017, at 3:57 PM, David Barroso notifications@github.com wrote:

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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

dbarrosop commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Coverage Status

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