-
Notifications
You must be signed in to change notification settings - Fork 30.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tools: add root certificate update script #47425
Conversation
Review requested:
|
One difference from the previous manual steps in |
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.
LGTM once issues flagged by linter are resolved.
Automates the steps from `doc/contributing/maintaining-root-certs.md`. Extend "Tools and deps update" workflow to use the new script to update the root certificates.
1ec9c79
to
d264ff6
Compare
Fixed now. |
Manually dispatched the workflow using this branch: https://github.com/nodejs/node/actions/runs/4622343859 |
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.
LGTM
This opened #47429. The second commit in that looks correct. The first commit looks odd, but is probably a side-effect of running the workflow from this branch which hasn't been merged. |
Ah, it's probably because this branch doesn't have #47339. Anyway that shouldn't be an issue when this is merged and the workflow run from |
@@ -167,13 +167,22 @@ jobs: | |||
cat temp-output | |||
tail -n1 temp-output | grep "NEW_VERSION=" >> "$GITHUB_ENV" || true | |||
rm temp-output | |||
- id: root-certificates | |||
subsystem: crypto | |||
label: crypto, notable-change |
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.
Do we usually do these as semver-minor
? I'm not sure.
label: crypto, notable-change | |
label: crypto, notable-change, semver-minor |
I'm also not sure if it's notable.
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.
#45490, #40280 and #35546 were not labelled
semver-minor
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.
Do we usually do these as
semver-minor
? I'm not sure.
@nodejs/releasers @nodejs/crypto Thoughts? We haven't been labelling root certificates updates as semver-minor, but maybe we should be?
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.
I would say no, but I understand if people think differently. To me this is more a bug fix (a certificate that would error before is then accepted). It doesn't add anything to the public API.
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.
To clarify my stance on
notable-change
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.
I'm in the semver-minor camp but with only one toe and it's not even my big toe. Call it +.1
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.
LGTM, and good idea.
@@ -167,13 +167,22 @@ jobs: | |||
cat temp-output | |||
tail -n1 temp-output | grep "NEW_VERSION=" >> "$GITHUB_ENV" || true | |||
rm temp-output | |||
- id: root-certificates | |||
subsystem: crypto | |||
label: crypto, notable-change |
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.
I'm in the semver-minor camp but with only one toe and it's not even my big toe. Call it +.1
I may not be around much over the Easter weekend (Friday and Monday are public holidays here). I've added the
commit-queue
|
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
Landed in a75871a |
Automates the steps from `doc/contributing/maintaining-root-certs.md`. Extend "Tools and deps update" workflow to use the new script to update the root certificates. PR-URL: #47425 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Automates the steps from `doc/contributing/maintaining-root-certs.md`. Extend "Tools and deps update" workflow to use the new script to update the root certificates. PR-URL: #47425 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Automates the steps from `doc/contributing/maintaining-root-certs.md`. Extend "Tools and deps update" workflow to use the new script to update the root certificates. PR-URL: nodejs#47425 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Automates the steps from
doc/contributing/maintaining-root-certs.md
.Extend "Tools and deps update" workflow to use the new script to update
the root certificates.
I attempted to test the workflow changes over in https://github.com/nodejs/node-auto-test but it looks like the tokens/permissions are not set up for that repository: https://github.com/nodejs/node-auto-test/actions/runs/4621889241
Running the new update script locally updates to NSS 3.89: