changes to tls-mode from default to prefer and update test cases #152
Conversation
|
|
|
README should be updated to reflect this change: https://github.com/vertica/vertica-nodejs/tree/master/packages/vertica-nodejs#tls |
| return self.emit('error', new Error('The server does not support TLS connections')) | ||
| if (self.tls_mode == 'prefer') { | ||
| self.attachListeners(self.stream) | ||
| self.emit('sslconnect') |
There was a problem hiding this comment.
For the management of our connection state events, 'sslconnect' is the proper event to emit. This may seem odd because we are not actually connecting with SSL wrapped socket in this case, however the sslconnect event just symbolizes that we completed our attempt at an ssl connection.
So this code and this emission of 'sslconnect' is correct. Just wanted to add some context
| @@ -1,3 +1,7 @@ | |||
| //Should we remove this file as its a redundant test for TLS mode? | |||
There was a problem hiding this comment.
I think it is reasonable to move this test for bad credentials to tls-tests.js.
Additionally, I would be partial to not only moving this test into tls-tests.js, but also moving tls-tests.js into mochatest/integration/connection (currently only client directory exists so we would add connection).
This is surely not any sort of requirement for this work so if you would prefer to leave things as they are, that is fine.
There was a problem hiding this comment.
I can open up another PR later for moving the tests and doing some more cleanup.
packages/vertica-nodejs/test/unit/connection/inbound-parser-tests.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This looks good to me now. I would give @sitingren a chance to take a look as well as she had some initial comments
No description provided.