GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: move some X509Certificate stuff to ncrypto by jasnell · Pull Request #54241 · nodejs/node

Conversation

@jasnell

@jasnell jasnell commented Aug 7, 2024

Copy link
Copy Markdown
Member

Starting working on moving the core bits of X509Certificate to ncrypto... this is incomplete but it's a big set of changes so I'm splitting them up in to multiple chunks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with rocket emoji
All reactions
  • 🚀 1 reaction
jasnell added 7 commits August 5, 2024 16:24
@jasnell
src: move some X509Certificate stuff to ncrypto
31c55f3
@jasnell
src: shift more crypto X509 to ncrypto
ccdc65e
@jasnell
src: shift more x509 to ncrypto
3fa04b0
@jasnell
src: shift more x509 to ncrypto
19881d2
@jasnell
src: shift even more x509 to ncrypto
369a69c
@jasnell
src: shift more x509 to ncrypto
6643ae0
@jasnell
src: fixup crypto linting after x509 changes
907c7a8
@jasnell
jasnell requested review from anonrig and tniessen August 7, 2024 04:58
@nodejs-github-bot

nodejs-github-bot commented Aug 7, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/security-wg
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 7, 2024
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Aug 7, 2024
@jasnell jasnell added crypto Issues and PRs related to the crypto subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. dependencies PRs that add, update, or configure Node.js dependencies. labels Aug 7, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 7, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@codecov

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell
jasnell force-pushed the use-ncrypto-moar branch from 19a28e2 to 7dc9283 Compare August 7, 2024 14:11
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell
jasnell force-pushed the use-ncrypto-moar branch from 7dc9283 to dd0f080 Compare August 7, 2024 14:23
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell
src: fixup includes after x509 changes
6397861
@jasnell
jasnell force-pushed the use-ncrypto-moar branch from dd0f080 to 6397861 Compare August 7, 2024 14:32
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
anonrig
anonrig reviewed Aug 7, 2024
View reviewed changes
Comment thread deps/ncrypto/engine.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread deps/ncrypto/ncrypto.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/crypto/crypto_x509.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/crypto/crypto_x509.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/crypto/crypto_x509.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

@jasnell
src: fixups after code review
5c5074e
anonrig
anonrig reviewed Aug 7, 2024
View reviewed changes
Comment thread src/crypto/crypto_x509.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@jasnell
src: address nits
083bad7
anonrig
anonrig approved these changes Aug 7, 2024
View reviewed changes
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Aug 7, 2024 •
edited by jasnell
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60953/ 💛

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell jasnell added author ready PRs with CI started, the required approvals, and no outstanding review comments. and removed needs-ci PRs that need a full CI run. labels Aug 8, 2024
mertcanaltin
mertcanaltin approved these changes Aug 8, 2024
View reviewed changes
@anonrig
anonrig requested review from benjamingr and mcollina August 12, 2024 15:13
mcollina
mcollina approved these changes Aug 12, 2024
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM as long as CI passes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jasnell jasnell added the commit-queue PRs queued for automated landing through the Commit Queue. label Aug 12, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Aug 12, 2024
@nodejs-github-bot

nodejs-github-bot commented Aug 12, 2024

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/54241
✔  Done loading data for nodejs/node/pull/54241
----------------------------------- PR info ------------------------------------
Title      src: move some X509Certificate stuff to ncrypto (#54241)
Author     James M Snell <[email protected]> (@jasnell)
Branch     jasnell:use-ncrypto-moar -> nodejs:main
Labels     crypto, c++, lib / src, author ready, dependencies
Commits    10
 - src: move some X509Certificate stuff to ncrypto
 - src: shift more crypto X509 to ncrypto
 - src: shift more x509 to ncrypto
 - src: shift more x509 to ncrypto
 - src: shift even more x509 to ncrypto
 - src: shift more x509 to ncrypto
 - src: fixup crypto linting after x509 changes
 - src: fixup includes after x509 changes
 - src: fixups after code review
 - src: address nits
Committers 1
 - James M Snell <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54241
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54241
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 07 Aug 2024 04:58:43 GMT
   ✔  Approvals: 2
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/54241#pullrequestreview-2225624836
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/54241#pullrequestreview-2233450678
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-08-07T22:13:52Z: https://ci.nodejs.org/job/node-test-pull-request/60953/
- Querying data for job/node-test-pull-request/60953/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 54241
From https://github.com/nodejs/node
 * branch                  refs/pull/54241/merge -> FETCH_HEAD
✔  Fetched commits as 624db509520a..083bad70e707
--------------------------------------------------------------------------------
[main 4aa5f71b56] src: move some X509Certificate stuff to ncrypto
 Author: James M Snell <[email protected]>
 Date: Mon Aug 5 16:24:51 2024 -0700
 2 files changed, 43 insertions(+), 1 deletion(-)
[main 8f81e293dd] src: shift more crypto X509 to ncrypto
 Author: James M Snell <[email protected]>
 Date: Tue Aug 6 19:28:20 2024 -0700
 4 files changed, 103 insertions(+), 20 deletions(-)
[main 9b78f096ba] src: shift more x509 to ncrypto
 Author: James M Snell <[email protected]>
 Date: Tue Aug 6 20:43:10 2024 -0700
 4 files changed, 210 insertions(+), 69 deletions(-)
[main 0e14dbdf2d] src: shift more x509 to ncrypto
 Author: James M Snell <[email protected]>
 Date: Tue Aug 6 21:12:59 2024 -0700
 6 files changed, 125 insertions(+), 85 deletions(-)
[main df19fced42] src: shift even more x509 to ncrypto
 Author: James M Snell <[email protected]>
 Date: Tue Aug 6 21:39:11 2024 -0700
 4 files changed, 138 insertions(+), 96 deletions(-)
[main 27eb278981] src: shift more x509 to ncrypto
 Author: James M Snell <[email protected]>
 Date: Tue Aug 6 21:54:26 2024 -0700
 4 files changed, 48 insertions(+), 49 deletions(-)
[main 6d166ebcb3] src: fixup crypto linting after x509 changes
 Author: James M Snell <[email protected]>
 Date: Tue Aug 6 21:55:02 2024 -0700
 2 files changed, 45 insertions(+), 45 deletions(-)
[main b268532cfd] src: fixup includes after x509 changes
 Author: James M Snell <[email protected]>
 Date: Wed Aug 7 06:59:26 2024 -0700
 4 files changed, 9 insertions(+), 14 deletions(-)
[main 947f98a9f2] src: fixups after code review
 Author: James M Snell <[email protected]>
 Date: Wed Aug 7 07:52:31 2024 -0700
 1 file changed, 1 insertion(+), 3 deletions(-)
[main 8272776189] src: address nits
 Author: James M Snell <[email protected]>
 Date: Wed Aug 7 08:43:42 2024 -0700
 2 files changed, 4 insertions(+), 4 deletions(-)
   ✔  Patches applied
There are 10 commits in the PR. Attempting autorebase.
Rebasing (2/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: move some X509Certificate stuff to ncrypto

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

[detached HEAD 138db0c245] src: move some X509Certificate stuff to ncrypto
Author: James M Snell <[email protected]>
Date: Mon Aug 5 16:24:51 2024 -0700
2 files changed, 43 insertions(+), 1 deletion(-)
Rebasing (3/20)
Rebasing (4/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: shift more crypto X509 to ncrypto

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

[detached HEAD d866d08687] src: shift more crypto X509 to ncrypto
Author: James M Snell <[email protected]>
Date: Tue Aug 6 19:28:20 2024 -0700
4 files changed, 103 insertions(+), 20 deletions(-)
Rebasing (5/20)
Rebasing (6/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: shift more x509 to ncrypto

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

[detached HEAD b95758585b] src: shift more x509 to ncrypto
Author: James M Snell <[email protected]>
Date: Tue Aug 6 20:43:10 2024 -0700
4 files changed, 210 insertions(+), 69 deletions(-)
Rebasing (7/20)
Rebasing (8/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: shift more x509 to ncrypto

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

[detached HEAD 31206a134f] src: shift more x509 to ncrypto
Author: James M Snell <[email protected]>
Date: Tue Aug 6 21:12:59 2024 -0700
6 files changed, 125 insertions(+), 85 deletions(-)
Rebasing (9/20)
Rebasing (10/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: shift even more x509 to ncrypto

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

[detached HEAD 6ea876450c] src: shift even more x509 to ncrypto
Author: James M Snell <[email protected]>
Date: Tue Aug 6 21:39:11 2024 -0700
4 files changed, 138 insertions(+), 96 deletions(-)
Rebasing (11/20)
Rebasing (12/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: shift more x509 to ncrypto

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

[detached HEAD 2cabbd704b] src: shift more x509 to ncrypto
Author: James M Snell <[email protected]>
Date: Tue Aug 6 21:54:26 2024 -0700
4 files changed, 48 insertions(+), 49 deletions(-)
Rebasing (13/20)
Rebasing (14/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: fixup crypto linting after x509 changes

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

[detached HEAD 5eecf9d324] src: fixup crypto linting after x509 changes
Author: James M Snell <[email protected]>
Date: Tue Aug 6 21:55:02 2024 -0700
2 files changed, 45 insertions(+), 45 deletions(-)
Rebasing (15/20)
Rebasing (16/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: fixup includes after x509 changes

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

[detached HEAD 1e73f13ae5] src: fixup includes after x509 changes
Author: James M Snell <[email protected]>
Date: Wed Aug 7 06:59:26 2024 -0700
4 files changed, 9 insertions(+), 14 deletions(-)
Rebasing (17/20)
Rebasing (18/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: fixups after code review

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

[detached HEAD d5c10d06ca] src: fixups after code review
Author: James M Snell <[email protected]>
Date: Wed Aug 7 07:52:31 2024 -0700
1 file changed, 1 insertion(+), 3 deletions(-)
Rebasing (19/20)
Rebasing (20/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: address nits

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

[detached HEAD eed729e2b2] src: address nits
Author: James M Snell <[email protected]>
Date: Wed Aug 7 08:43:42 2024 -0700
2 files changed, 4 insertions(+), 4 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/10357382875

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell added a commit that referenced this pull request Aug 12, 2024
@jasnell
src: move some X509Certificate stuff to ncrypto
5f230d2
PR-URL: #54241
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@jasnell

jasnell commented Aug 12, 2024

Copy link
Copy Markdown
Member Author

Landed in 5f230d2

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell jasnell closed this Aug 12, 2024
@jasnell jasnell removed the commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. label Aug 12, 2024
targos pushed a commit that referenced this pull request Aug 14, 2024
@jasnell @targos
src: move some X509Certificate stuff to ncrypto
b6927dd
PR-URL: #54241
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
v22.7.0 proposal #54452
Merged
@targos targos added the dont-land-on-v20.x label Sep 21, 2024
npaun pushed a commit to npaun/ncrypto that referenced this pull request Sep 18, 2025
@jasnell @npaun
src: move some X509Certificate stuff to ncrypto
8218bd2
PR-URL: nodejs/node#54241
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@anonrig anonrig anonrig approved these changes
@tniessen tniessen Awaiting requested review from tniessen
@benjamingr benjamingr Awaiting requested review from benjamingr
+1 more reviewer
@mertcanaltin mertcanaltin mertcanaltin approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. dependencies PRs that add, update, or configure Node.js dependencies. lib / src Issues and PRs involving general changes in the lib/ or src/ directories.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@jasnell @nodejs-github-bot @mcollina @anonrig @mertcanaltin @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.