| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Review requested:
|
Sorry, something went wrong.
|
One difference from the previous manual steps in doc/contributing/maintaining-root-certs.md is that the automation collapses the two commits into a single commit to fit in with the existing tools workflow. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM once issues flagged by linter are resolved.
Sorry, something went wrong.
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.
Fixed now. |
Sorry, something went wrong.
|
Manually dispatched the workflow using this branch: https://github.com/nodejs/node/actions/runs/4622343859 |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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 main. |
Sorry, something went wrong.
| rm temp-output | ||
| - id: root-certificates | ||
| subsystem: crypto | ||
| label: crypto, notable-change |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
#45490, #40280 and #35546 were not labelled semver-minor PRs that contain new features and should be released in the next minor version. but were labelled notable-change PRs with changes that should be highlighted in changelogs. . I kind of feel that listing the removed and/or added certificates should be in the release notes (hence notable-change PRs with changes that should be highlighted in changelogs. ).
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
To clarify my stance on notable-change PRs with changes that should be highlighted in changelogs. -- I'm thinking that when certificates are removed (often for security reasons) that at least noting that will help anyone running into issues because of it.
Sorry, something went wrong.
There was a problem hiding this comment.
I'm in the semver-minor camp but with only one toe and it's not even my big toe. Call it +.1
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM, and good idea.
Sorry, something went wrong.
| rm temp-output | ||
| - id: root-certificates | ||
| subsystem: crypto | ||
| label: crypto, notable-change |
There was a problem hiding this comment.
I'm in the semver-minor camp but with only one toe and it's not even my big toe. Call it +.1
Sorry, something went wrong.
|
I may not be around much over the Easter weekend (Friday and Monday are public holidays here). I've added the commit-queue Add this label to land a pull request using GitHub Actions. label so this can land after the wait period (and no objections). Not labelling the updates semver-minor PRs that contain new features and should be released in the next minor version. (the current state of this PR) is what we've been doing, which I am persuadable to change. If we do decide to make these updates semver-minor PRs that contain new features and should be released in the next minor version. by default, that's easily done in a follow up PR. |
Sorry, something went wrong.
There was a problem hiding this comment.
RSLGTM
Sorry, something went wrong.
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>
| Back | FazBrowse Home | New Git URL |
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:
Details