-
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
OpenSSL update instructions - feedback wanted #42395
Comments
I'm broadly in favour of adding more dockerisation of the process as an option -- I actually run the existing container (the non-Linux instructions) on Linux when I've done OpenSSL updates. From my POV the main complexity of the OpenSSL update process is that it is slightly different for the older release lines using upstream OpenSSL vs the more recent release lines that use the quic fork. |
@richardlau in terms of
A branch for those older versions which has the changes needed for the older versions is what I had in mind. |
@mhdawson I'm wondering if there is a chance to run those commands in a self-hosted CI, which may create the PR automatically. I don't know how much time would it take in a normal CI (I assume a lot), but, we can use dedicated machines for those work. Is it feasible? |
@RafaelGSS I had wondered about setting up CI to run/generate the PR. What I'm not sure of is how many times it will This was my comment above:
@richardlau what's your take on how often it should just work based on your past experience? |
@mhdawson In the past it has generally just worked but the last two updates did not and caused test failures.
|
If most of the past record is that it just works then trying to automate it makes sense to me. |
Sorry for the late response here. My 2 cents: I have contributed to the ssl updates a few times across 2 years and I would say that apart from a single time (there was a change in the file structure), the steps were pretty much consistent and therefore can be automated. Having a CI that creates a PR with the openssl update sounds neat. But I think we should also talk about the workflow in case the CI fails. I think in the event of failure, the pipeline should open a new issue making sure the relevant stakeholders are tagged to ensure necessary steps can be taken. |
FYI I'll be working on that action in the next coming days |
To help me get up to speed on the instructions to update OpenSSL in
maintaining-openssl.md
I ran through the instructions and built some docker files that would automate almost the full process.
These are in https://github.com/mhdawson/node-openssl-utils (one branch for main and one for 16.x so far)
What I like about them is that I can edit the version of openssl to pull in, start the process with
bash buildit.sh
and come back an hour later(on a big machine, likely longer on something more typical) to a docker container with the changes applied, including our standardize commit comments.It does not push a PR, as I'm not sure it would be successful enough for that to make sense but it keeps the results of running the tests and you can confirm that all modified files were included in the commits. If there are any additional changes required (like the changes o the OpenSSL tests in the last security release) you can cherry pick those over and re-run the tests easily/quickly It is also handy for doing a quick sniff check of a PR by confirming that the same number of files have been modified as expected.
I wondering if we'd want to do one of the following
Automating in an action would not quite be the same in my mind as you don't get the container which you can use to carry out the last few steps or cherry pick commits if necessary. If we think the complete process might work frequently enough then an action running similar steps could make sense.
They are not quite complete as I can see that I missed some of the externalization of the git email and user name but I'd do that if 1) or 2) above make sense.
@richardlau, @hassaanp since I know you have done some of the recently OpenSSL updates.
The text was updated successfully, but these errors were encountered: