Clone specific software-layer-commit and implement CI to check merged status - #1353
Clone specific software-layer-commit and implement CI to check merged status#1353casparvl wants to merge 34 commits into
Conversation
|
Let's do a small test build to see if the new |
|
TODO: I guess in this same PR we should still add a check that verifies that the SHA-checksum of |
…casparvl/software-layer into improve_software_layer_scripts_workflow
|
Perfect. As expected, after changing the
Changing the
|
|
Ok, as expected, both Let's test again by changing the sha checksum... |
|
Ok, on bee1d29 we again have the expected failure:
Let's change the sha checksum back, and now change something in |
…f this causes the associated CI job to fail
|
Again, we get the expected failure:
|
| # This workflow verifies that the correct version of software-layer-scripts is used. | ||
| # | ||
| # First, check_bot_build_checksums checks if the bot/build.sh code that clones software-layer-scripts is untouched, | ||
| # as this normally shouldn't change (a change could mean a contributor is trying to inject something |
There was a problem hiding this comment.
This should be toned down, changing the bot/build.sh script doesn't necessarily imply that something malicious is going on.
It's weird/unusual, sure, but let's not brand every possible change to bot/build.sh script as malicious up front
There was a problem hiding this comment.
It's especially ironic that this very PR is also making changes to bot/build.sh ;-)
There was a problem hiding this comment.
I think this is the first change since we've split off software-layer-scripts from software-layer, well over a year ago. Plus, the wording is 'could', it doesn't say it is malicious.
The point is: there is very little reason to modify bot/build.sh going forward. Builds from feature branches are already possible, as long as they are open PRs to EESSI/software-layer-scripts. If bot/build.sh is modified, we want it to stand out really well, since it is a potential risk to do builds on a PR that uses some arbitrary bot/build.sh.
I know never to say never, so it can still happen - and that's ok. It'll just require a checksum update in the CI as well :)
| with: | ||
| fetch-depth: 1 # We only need the current revision to read bot/commit_sha | ||
| - name: Checkout software-layer-scripts (full history) | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
This should also use specific commit, as above
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: EESSI/software-layer-scripts | ||
| path: upstream-scripts |
There was a problem hiding this comment.
Why make it difficult, just call this EESSI-software-layer-scripts ?
| TOPDIR=$(dirname $(realpath $0)) | ||
|
|
||
| # Clone a the commit from software-layer-script that corresponds to `bot/commit_sha` | ||
| commit_sha=$(cat ${TOPDIR}/commit_sha) |
There was a problem hiding this comment.
Please rename this file so it's clear from the filename what this refers to:
| commit_sha=$(cat ${TOPDIR}/commit_sha) | |
| software_layer_scripts_commit=$(cat ${TOPDIR}/software_layer_scripts_commit) |
This is the critical point for me: as long as the approach we're taking here doesn't affect most contributors, it's OK imho. And I would say that's indeed the case here: only if you need updated hooks (or other scripts), you need to take action. I like the approach being proposed here, as it removes a great deal of uncertainty: no more guessing which commit of the scripts was used, it's crystal clear. I wouldn't hurt to make We should also update our docs to reflect these changes, with a specific subsection that outlines the procedure if changes to hooks are required. |
|
@casparvl Where is this stuck? |
|
At a technical level, I don't know if it is even stuck - I think this worked. But back then I felt we lacked the commitment to actually adopt this approach - though it keeps popping up that our existing workflow is annoying. I would like to get back to this, so please leave it open. I think at a technical level, the one thing that would be needed is an accompanying bot PR that makes the bot print which checksum was used. I think we discussed at some point how to do this in a generic way, and the plan - if I remember correctly - was to have a configurable item in app.cfg that points to a file in the repo that - if it exists - is read, and the information is added to the bot's feedback in the PR (and then also to the summary table). |
|
Made a PR for the bot to report the commit SHA of the |
|
TODO:
This means that if a PR changed One caveat: if someone forks, and doesn't sync with main, builds will happen with a somewhat older version of Note that even after the above TODO item is tackled, it is still the reviewers that are responsible for checking that they only set a deploy label after the CI "Verify software-layer-scripts / check_software_layer_scripts_commit" passes, as this ensures that the builds are done from a merged |
|
I thought about it some more, and maybe we can do better.
======= Create trigger on repo A (software-layer):
You’ll use these in repo B to start a pipeline in repo A. CI on repo B (software-layer-scripts): CI on repo A (software-layer) ======= The downside is that this requires tokens that have direct write access to your main branch. It should be possible to do this in a controlled way in the GH CI space (masked & protected tokens), but if that makes us uncomfortable, the alternative would be:
That solution only requires write tokens on feature branches. |
|
Some more interaction with my AI tells me that I need to use |
|
Ok, to avoid stalling this over discussion, let's phase the approach: First I'll implement:
This should be totally safe, and only requires the 2nd GH action to have write access to a dedicated feature branch which I'll call |
…against the latest commit on EESSI/software-layer-scripts's main branch, to see if it points to the latest commit
|
Perfect, https://github.com/EESSI/software-layer/actions/runs/31018598477/job/92349130424?pr=1353 goes green immediately (and takes only 6s to run). Let me change the checksum to an older merge commit. |
|
Excellent, CI https://github.com/EESSI/software-layer/actions/runs/31018815032/job/92349881283?pr=1353 goes red immediately, reporting: Now, let's move to an unmerged commit, check that our other CI also goes red in that case. |
|
Perfect, the other CI also fails https://github.com/EESSI/software-layer/actions/runs/31019123923/job/92350942842?pr=1353 with: Exactly as itended. This CI always runs, and provides reviewers with a red flag that this thing is NOT ready to deploy, as it's not pointing to a merge commit. |
…supported CPU targets for that EESSI version. If there is a checksum, it'll verify that all checksums are the same for all tarballs
…mplement this in a seperate PR EESSI#1564
…. We don't want to bother contributors who don't know about this file that they have to update stuff - for that we will create a seperate GH Action to automtically create PRs that update this. With this CI, we only ensure that contributors that DO change the commit, ensure that we don't merge an older commit into our software-layer/main branch (even if they built from an older one, which is totally fine)
This PR is an initial step in creating a workflow where we can use PRs to
software-layer-scriptsdirectly, and then once they are merged, just update the SHA checksum to a (Github-signed) merge commit, rebuild, and be done :)Edit: better description...
This PR contains three changes:
bot/build.shnow clones a particular commit, which is specified inbot/commit_shaVerify software-layer-scripts / check_bot_build_checksumverifies the checksum ofbot/build.shagainst a reference hardcoded in the workflow file. This way, a malicious contributor would have to modify bothbot/build.shand the workflow file, which would (hopefully) stand out to a reviewer.Verify software-layer-scripts / check_software_layer_scripts_commitwhich check thatbot/commit_shais a commit that is part of the upstream https://github.com/EESSI/software-layer-scripts , is on themainbranch (i.e. has been merged), is a merge commit, and is signed with the public Github GPG for the web interface.We might need to update the
commit_shaalready (not sure if there have been more merges to software-layer-scripts since I started this) before we merge this to make sure this PR doesn't actually revert us to using an older version.