More exceptions #248
More exceptions #248
Conversation
Also better doc for the ConnectionException.
mirceaulinic
changed the title
napalm_base/exceptions.py
Outdated
| @@ -22,6 +22,35 @@ class ModuleImportError(Exception): | |||
|
|
|||
|
|
|||
| class ConnectionException(Exception): | |||
| ''' | |||
| Unable to connect to the network device. | |||
| Raised by the ``open`` method. | |||
There was a problem hiding this comment.
This last sentence is not accurate. For two reasons;
- childs of this class will be thrown in other places, which means you can actually catch this exception in other methods.
- It kind of implies this is the only exception that is going to be thrown by the open method while the truth is that it would be nice if its childs were raised instead.
napalm_base/exceptions.py
Outdated
|
|
||
| class ConnectTimeoutError(ConnectionException): | ||
| ''' | ||
| Connection to the network device takes too long. |
There was a problem hiding this comment.
Maybe mention timeout value can be tweaked with the timeout argument. Mostly to indicate there is a correlation.
|
|
||
| class ConnectionClosedException(ConnectionException): | ||
| ''' | ||
| The network device closed the connection. |
There was a problem hiding this comment.
I would reword this and use "dropped' instead unless we are expecting to raise this exception if the server ends the TCP socket. My guess is we will be raising this if the connection is silently dropped by a firewall or the server.
|
@dbarrosop I've pushed some changes: b3b5c9b, feel free to suggest more improvements if anything still doesn't sound good. |
|
In SSH you can also have the SSH channel closed by remote end (currently raises an EOFError in Netmiko). In this case the TCP connection is still open, but you are unable to communicate. I think this should probably fall into this |
|
Yes, that was my understanding too @ktbyers. Would you suggest including this explanation in the exception docsting? |
|
I think we might want to include that in the docstring (i.e. 'or SSH channel closing'). |
|
We don't really need to be super explicit. Most of the time people will just want to know the connection was dropped due to inactivity (regardless of who or how) and just try to reconnect. My point is that a single ConnectionDroppedException and then different messages for logging purposes should probably be enough. I can't imagine someone treating a timeout because a firewall dropped a flow being treated different than ssh closing the channel. |
Define new exception
ConnectionClosedExceptionas per #247Add the exceptions proposed in #218