tools: automate openssl update on v16#48377
Conversation
|
Review requested:
|
2b0271b to
e2ca567
Compare
|
for 3.0 stream at least, quictls isn't using releases to manage new versions so the edit: adding link to comment quictls/openssl#114 (comment) for context
so that we query the tags from quictls instead of releases I suspect a similar pattern is needed for the 1.x stream also |
I guess you refactored it to use fetch instead, so the logic needs modification from what I suggested, but the principal of checking for tags rather than releases is my main point. The fetch also results in non-quic and quic tag names, so more logic may be necessary to filter on those. |
baparham
left a comment
There was a problem hiding this comment.
I tried to more precisely capture what I mean in some specific review comments, hopefully I'm not way off here and this is at least a little bit helpful.
|
@baparham could you check again now? also I think this doesnt apply for v1, I dont see v1 in https://api.github.com/repos/quictls/openssl/tags |
|
They are deleting the releases, per that comment from @richsalz, so I don't think that's correct for the 1.1 stream. What's odd is I see tags here https://github.com/quictls/openssl/tags but not in the gh api call, like you pointed out. Perhaps they are just remnants of previous "releases" and not actual tags or something weird. Either way, I suspect that the |
|
The GH API is paginated and only returns a subset of results. For me at least the tags for 1.1.1 are on https://api.github.com/repos/quictls/openssl/tags?page=4. Alternatively git/matching-refs API, e.g. https://api.github.com/repos/quictls/openssl/git/matching-refs/tags/OpenSSL_1_1_1 will return tags beginning with "OpenSSL_1_1_1". |
I need a facepalm reaction on github comments, haha. I did not catch that it was paginated, thanks for pointing that out! the matching_refs endpoint is probably better to use then. |
|
I've used matching ref but its a bit hacky wdyt? |
5e55dc6 to
bd065ae
Compare
Commit Queue failed- Loading data for nodejs/node/pull/48377 ✔ Done loading data for nodejs/node/pull/48377 ----------------------------------- PR info ------------------------------------ Title tools: automate openssl update on v16 (#48377) Author Marco Ippolito (@marco-ippolito) Branch marco-ippolito:feat/automate-openssl-backport -> nodejs:main Labels meta, tools, author ready, commit-queue-squash Commits 3 - tools: automate update openssl v16 - fix: check tags instead of release for 3.0 - fix: use git ref Committers 1 - Marco Ippolito PR-URL: https://github.com/nodejs/node/pull/48377 Reviewed-By: Rafael Gonzaga Reviewed-By: Antoine du Hamel ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/48377 Reviewed-By: Rafael Gonzaga Reviewed-By: Antoine du Hamel -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - tools: automate update openssl v16 ⚠ - fix: check tags instead of release for 3.0 ⚠ - fix: use git ref ℹ This PR was created on Wed, 07 Jun 2023 12:36:03 GMT ✔ Approvals: 2 ✔ - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/48377#pullrequestreview-1467734115 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/48377#pullrequestreview-1469308861 ✔ Last GitHub CI successful ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/5280129740 |
|
Landed in 51ca71c |
PR-URL: nodejs#48377 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>
PR-URL: nodejs#48377 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>
PR-URL: nodejs#48377 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>
PR-URL: #48377 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>
PR-URL: nodejs#48377 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>
PR-URL: nodejs#48377 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>
|
This commit does not land cleanly on and will need manual backport in case we want it in v18. |
|
@marco-ippolito is the backport needed for v18 and v20? I assume no, right? We just need to revert it for |
|
I dont think it is needed since it will only run on main and then we can backport security patches. we can revert it since we dont support 16 anymore |
This PR allows updating openssl 1 on v16, for this to work the scripts (also utils.sh) need to be backported to v16