fix: allow ca, key and cert will be string (regression)
#1676
Conversation
| @@ -610,7 +623,7 @@ class Server { | |||
| const pems = createCertificate(attrs); | |||
|
|
|||
| fs.writeFileSync(certPath, pems.private + pems.cert, { | |||
| encoding: 'utf-8', | |||
| encoding: 'utf8', | |||
There was a problem hiding this comment.
utf8 is right value https://nodejs.org/api/fs.html#fs_fs_writefilesync_file_data_options
Codecov Report
@@ Coverage Diff @@
## master #1676 +/- ##
==========================================
+ Coverage 75.12% 76.17% +1.04%
==========================================
Files 17 17
Lines 591 596 +5
Branches 172 173 +1
==========================================
+ Hits 444 454 +10
+ Misses 113 108 -5
Partials 34 34
Continue to review full report at Codecov.
|
|
|
||
| if (stats) { | ||
| // It is file | ||
| options.https[property] = fs.readFileSync(path.resolve(value)); |
There was a problem hiding this comment.
Shouldn’t this have utf8 encoding as well?
There was a problem hiding this comment.
@alan-agius4 hm, maybe, can you provide example?
There was a problem hiding this comment.
No need
https://github.com/nodejs/node/blob/master/lib/fs.js#L392
buffer.toString :
encoding <string> The character encoding to use. Default: 'utf8'.
buf.toString([encoding[, start[, end]]])
There was a problem hiding this comment.
So default is utf8
|
/cc @hiroppy need review |
For Bugs and Features; did you add new tests?
Yes
Motivation / Use-Case
fixes #1674
Breaking Changes
No
Additional Info
It is regression, just not
pfxcan't be string due it is binary format