don't leak eznc connect exceptions

[grizz]

No description provided.

[coveralls]

Coverage remained the same at 84.773% when pulling 8486beb on grizz:eznc_exc into dcaf134 on napalm-automation:develop.

[mirceaulinic]

Leave both please

[mirceaulinic]

@grizz what problem are you trying to fix more specifically? Can you provide an example? ConnectError is the base class for all Connect* exceptions in junos-eznc. If too generic we will loose some important data, as the error messages do not provide enough information: https://github.com/Juniper/py-junos-eznc/blob/436632a26d6cb4279b055971ddf87ed0be0f229c/lib/jnpr/junos/device.py#L910

E.g.: if the connection is rejected due to unknown host, we would only see that ConnecError has been raised without any error message.

[grizz]

@mirceaulinic If you're not abstracting the exceptions, it makes it very hard to use as a library. For example, if the eznc exception is propagated up the stack, my generic program at the top has to know that it's an eznc object and/or deal with each type of network driver separately.

[mirceaulinic]

@grizz this is not the answer I am looking for.

Why not then:

try:

except Exception

and avoid propagating any errors.

Can you give an example of the error you encountered and broke your app? And here I refer to errors that may occur by non-deterministic causes: e.g. ConnectAuthError is NOT one of them, as this can be thrown at most once - after you have your environment properly setup, this will never be raised etc.

[coveralls]

Coverage decreased (-0.2%) to 84.548% when pulling cebe9c5 on grizz:eznc_exc into dcaf134 on napalm-automation:develop.

[grizz]

@mirceaulinic You could just raise Exception but since both eznc and napalm classify connection errors separately, why not at least keep that. I guess the driver's exception would be preferable to just Exception, but either way the calling program is getting no actionable information.

What does it matter whether or not it's deterministic matter for exception type?

In https://github.com/20c/ngage/tree/master/ngage/plugins I have plugins for both eznc and now napalm, in eznc I catch ConnectionAuthError and prompt for a password if it's thrown, in napalm, since you don't have an auth exception, I was settling for ConnectionError.

[grizz]

Maybe this should be closed and discussed in an issue on a broader scope?

[mirceaulinic]

@grizz

I guess the driver's exception would be preferable to just Exception, but either way the calling program is getting no actionable information.

I was sarcastic on this :)

What does it matter whether or not it's deterministic matter for exception type?

As previously said, exceptions such as ConnectionAuthError provide you information about your environment and they should fail loudly! (e.g.: you see this error -> you go to your device and discover that the user is not configured / rights issue -> configure -> you will never have this problem again).

On the other hand, if you restrict your visibility to napalm_base.exceptions.ConnectionException, as you proposed, how can you distinguish between an error raised because of credentials or an error caused by wrong hostname?

[grizz]

I was sarcastic on this :)

Was wondering, either way, it's virtually the same from a program's perspective :)

As previously said, exceptions such as ConnectionAuthError provide you information about your environment and they should fail loudly! (e.g.: you see this error -> you go to your device and discover that the user is not configured / rights issue -> configure -> you will never have this problem again).

It's a command line tool, so that wouldn't be true in my case.

On the other hand, if you restrict your visibility to napalm_base.exceptions.ConnectionException, as you proposed, how can you distinguish between an error raised because of credentials or an error caused by wrong hostname?

Ideally, you'd have exception wrappers for each driver that conveyed all of the information in a standard and more granular way. Without that, at least with this I can tell if it's happened during connection, try a password, and then throw if not.

Without an exception abstraction, how would you propose I handle any exception thrown from open()? Just give up and stack trace?

[mirceaulinic]

Ideally, you'd have exception wrappers for each driver that conveyed all of the information in a standard and more granular way.

That would make sense - we should indeed have more granular exceptions. Feel free to open an issue for discussion under napalm-base or even come with a PR with new Exception classes.

[dbarrosop]

My two cents on this, we should certainly catch "transport library" specific exceptions and throw our owns. Otherwise, as @grizz mentioned it's a nightmare from a napalm user perspective.

To avoid losing information:

1. We can have as many as needed; one for unreachable, autherror, whatever is needed to unify exceptions across vendors without losing visibility.
2. We can also add as many relevant information to the Exceptions thrown as needed, maybe even the original exception class name and data.

[mirceaulinic]

We can have as many as needed; one for unreachable, autherror, whatever is needed to unify exceptions across vendors without losing visibility.

Totally agree, let's do that!

[mirceaulinic]

Closing this as better exceptions will be tracked under napalm-automation/napalm-base#218.

@grizz if you are able to give a helping hand with this, it would be much appreciated! Thanks!