Add mssql trustServerCertificate option. #4500
Conversation
|
Can this PR be reviewed please? CI failed because of some other reason. We want to use these missing types instead of supplying them ourselves. |
| @@ -62,6 +62,7 @@ class Client_MSSQL extends Client { | |||
| useColumnNames: false, | |||
| tdsVersion: settings.options.tdsVersion || '7_4', | |||
| appName: settings.options.appName || 'knex', | |||
| trustServerCertificate: false, | |||
There was a problem hiding this comment.
Could you please remove the default value? Considering that it might silently change behaviour of currently running applications, that would be a breaking change.
There was a problem hiding this comment.
probably false default is fine here? I mean if that option has not been given earlier it couldn't have been true.
There was a problem hiding this comment.
I checked mssql documentation and you are right, false is the default
|
Released in 0.95.8 |
|
This is a good bugfix, and greatly appreciated for those of us using docker containers based on the default mssql linux image for development purposes, which comes with a self-signed certificate. However, this fix is not immediately available to users of Objection.js because Objection.js requires knex@<0.95.0, which would exclude this bugfix. Under normal circumstances I could check out whatever major version Objection.js is tied to and create a bugfix PR for that stream. However, I'm not really sure where to start because there appears to be a rather large gap in the tagging between the 0.21.x stream, and the 0.95.x stream. I'm not sure what the reasoning is here as you are using what looks like a sematic versioned number, but not really using it semantically (see semver.org). I did not readily find contribution guidelines in the github.come readme page... so... If I were to try to create a PR that back-ports a change like this to a version number that Objection.js and other libraries could use, how would I go about that? |
|
I hope I did this properly. Here's my PR for the 0.21 stream. |
|
Note that 3.0.0-alpha.4 supports latest knex. That said, I'll review your PR and if it's not too disrupting, will be happy to merge! |
|
Thanks for the tip! Our company development policies require stable releases to develop against. So unfortunately 3.0.0-alpha.4 is not an option for us. Thanks for considering my PR! :) |
|
I saw that my PR was merged and released in 0.21.20. Thank you! |
|
np, thank you for your contribution! |
No description provided.