tls: use options in getCACertificates() with X509Certificate#59349
tls: use options in getCACertificates() with X509Certificate#59349haramj wants to merge 29 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
doc/api/tls.md
Outdated
| trusted store. | ||
| * When [`NODE_EXTRA_CA_CERTS`][] is used, this would also include certificates loaded from the specified | ||
| file. | ||
|
|
There was a problem hiding this comment.
It seems the white spaces are still there? Can you remove them?
There was a problem hiding this comment.
Okay. I'll remove the white space
There was a problem hiding this comment.
@joyeecheung Removing that white space will cause a format error in the document..
|
@jasnell Thank you very much for the thorough and detailed reviews. Additionally, |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #59349 +/- ##
==========================================
- Coverage 89.68% 89.66% -0.03%
==========================================
Files 676 676
Lines 206710 206751 +41
Branches 39584 39590 +6
==========================================
- Hits 185398 185391 -7
- Misses 13441 13495 +54
+ Partials 7871 7865 -6
🚀 New features to boost your workflow:
|
|
The CI is failing due to a missing documentation anchor: |
2e19981 to
d978079
Compare
|
Hi, feedback has been addressed. |
|
If you have some time, could you please review this? @nodejs/net, @joyeecheung |
jasnell
left a comment
There was a problem hiding this comment.
LGTM! Great job. Let's ask @joyeecheung to also take a look since it was her TODO :-)
Thank you so much for the review and the LGTM! I really appreciate your time. During my final testing, I discovered an unexpected failure related to caching, and also found a minor inconsistency with the documentation. I would like to push a fix for those before the PR is ready to merge. I'll push the updated changes shortly. |
|
@jasnell @joyeecheung So I added the caching logic, but now other tests are failing. |
|
I've successfully implemented support for new formats by clearly separating the logic for object input while maintaining the existing function's behavior. All tests have passed, and I believe the PR is ready for review. Thank you. |
Co-authored-by: James M Snell <jasnell@gmail.com>
Co-authored-by: James M Snell <jasnell@gmail.com>
Co-authored-by: James M Snell <jasnell@gmail.com>
Co-authored-by: James M Snell <jasnell@gmail.com>
Co-authored-by: James M Snell <jasnell@gmail.com>
Co-authored-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
e2879a5 to
e787797
Compare
This PR implements the TODO(joyeecheung) note to support X509Certificate output in tls.getCACertificates(). It introduces a new format option, enhancing the function's flexibility and aligning its API with Node.js's broader crypto module.
The function's behavior is now extended as follows:
API Enhancement: Adds an optional format parameter to tls.getCACertificates() to specify the output format.
Output Options: