EOS: Add a JSON option when running a cli command

[dasturiasArista]

pyeapi supports JSON as an option for the cli commands. However right
now the JSON option isn't exposed to users of this API. This exposes the
JSON option as well.

I manually tested this with an Arista switch and verified the changes work.

[ktbyers]

Tests are failing on this change.

[dasturiasArista]

Tests are failing on this change.

I was noticing that. I'm wondering the best way to handle adding functionality would be. It looks like tests are failing because I've added an extra argument to this function that it's not expecting. Would it ok to modify the test to allow the extra args in the EOS case?

[ktbyers]

So the test issue is that all of the NAPALM drivers (junos, eos, ios, etc) must have the same signature for the method:

    def cli(self, commands, encoding="text"):

So if we add the encoding argument, then we have to add it for all of the drivers.

You can always access pyeapi via napalm using:

napalm_object.device

I guess I wonder if we really need to be able to execute the cli method but with JSON output. Note, I basically never use the cli method...so probably would be better if @mirceaulinic or other users of CLI commented on this and whether it is worth doing this.

[dasturiasArista]

So the test issue is that all of the NAPALM drivers (junos, eos, ios, etc) must have the same signature for the method:

    def cli(self, commands, encoding="text"):

So if we add the encoding argument, then we have to add it for all of the drivers.

You can always access pyeapi via napalm using:

napalm_object.device

I guess I wonder if we really need to be able to execute the cli method but with JSON output. Note, I basically never use the cli method...so probably would be better if @mirceaulinic or other users of CLI commented on this and whether it is worth doing this.

Ok. that seems reasonable. I was unaware that the "device" attribute was considered a stable part of the api. I have proposed this solution for the customer trying to use this to confirm if this works for them or not.

[ktbyers]

Yeah, I would think we would not change napalm_object.device (at least not without really good reason and without following semantic versioning i.e. a major release) as that would break lots of things.

It is possible this napalm_object.device convention is not followed in napalm-community drivers, but it should be followed in napalm core.

[ktbyers]

@dasturiasArista Yeah, let us know what you find and if that solution works.

[dasturiasArista]

@dasturiasArista Yeah, let us know what you find and if that solution works.

Yes will do! Not to keep you hanging, still waiting from the response from the customer.

[mirceaulinic]

Just to add to Kirk's comment further:

So if we add the encoding argument, then we have to add it for all of the drivers.

We have a similar request for Junos (#1500) so it'd be great if you could go ahead with this and I can take care of the Junos implementation.

[dasturiasArista]

Just to add to Kirk's comment further:

So if we add the encoding argument, then we have to add it for all of the drivers.

We have a similar request for Junos (#1500) so it'd be great if you could go ahead with this and I can take care of the Junos implementation.

Ok. I can go ahead and add the argument to all of the drivers, but for those that don't support that encoding i'll just have it raise an error. How do you feel about raising a ValueError?

[dasturiasArista]

Just to add to Kirk's comment further:

So if we add the encoding argument, then we have to add it for all of the drivers.

We have a similar request for Junos (#1500) so it'd be great if you could go ahead with this and I can take care of the Junos implementation.

Ok. I can go ahead and add the argument to all of the drivers, but for those that don't support that encoding i'll just have it raise an error. How do you feel about raising a ValueError?

Btw I have proactively made the changes we talked about here. Also, each method will check that it's valid and throw a ValueError if it's not a valid encoding.

[mirceaulinic]

Looks good to me, just requesting to raise NotImplementedError instead of ValueError.

[mirceaulinic]

Thanks @dasturiasArista!