SSH timeout added #50
Conversation
|
|
||
| private @NotBlank @TunnelRoute String route; | ||
| private @Min(1) @Max(65535) int port = DEFAULT_PORT; | ||
| private String localhost = DEFAULT_LOCALHOST; | ||
| private @NotBlank String privateKeys; | ||
| private @NotBlank String knownHosts; | ||
| private @Min(0) int timeout = DEFAULT_TIMEOUT_MILLIS; |
There was a problem hiding this comment.
I think we should either name this timeoutMillis to be clear what unit it is or we introduce two values here - one which is a TimeUnit and the other being the value.
There was a problem hiding this comment.
Even if the docs say the units is millis?
There was a problem hiding this comment.
Yeah, I guess it's fine as long as its clearly documented.
| @@ -49,6 +50,15 @@ public void typical() { | |||
| assertThat(violations.size(), is(0)); | |||
| } | |||
|
|
|||
| @Test | |||
| public void infiniteTimeout() { | |||
There was a problem hiding this comment.
We should document in the README that setting it to zero means no timeout.
| @@ -148,4 +158,13 @@ public void blankPrivateKey() { | |||
| assertThat(violations.size(), is(1)); | |||
| } | |||
|
|
|||
| @Test | |||
| public void negativeTunnel() { | |||
There was a problem hiding this comment.
yeah, typo.. will rename
| } | ||
|
|
||
| public SessionFactorySupplier(int sshPort, String knownHosts, List<String> identityKeys, int sshTimeout) { | ||
| Preconditions.checkArgument(0 <= sshPort && sshPort <= 65535, "Invalid SSH port number " + sshPort); |
There was a problem hiding this comment.
Why do we have this check both here and in the TunnelHandler but we only have the timeout check here and not in the TunnelHandler ?
There was a problem hiding this comment.
I'm improving the code, the old version does not check... the constructor is public
|
This just got uglier but it works. Please take a look again and I'll try to do a release tomorrow. |
Fixes #49
I'll be running functional tests today, please take a look in the meantime