Implementation options of automated PR builds + tests for submodules

[t-lo]

Situation

Recently, work completed on automated Pull Request builds and tests via Github Actions for the "root" Scripts repository. The feature allows for automated and for manually triggered OS image builds + qemu test runs.

Manual builds can be triggered by dispatching the ci.yaml workflow, e.g. specifying a custom branch. Tests will be run automatically after the build was successful. Additionally, tests can be triggered separately and will then run on the artifacts of a previous build.
PRs will automatically be built on

• PRs going from "Draft" into "Ready for review" mode.
• PRs in "Ready for Review" mode when reviews are requested.

Cycling a PR back to "Draft" and immediately back to "Ready for review" can be used to re-build / re-test PRs after changes were made. Similarly, re-requesting review after a reviewer left comments but did not approve can also be used to re-build and re-test.

Problem

While the above feature exist for the scripts repo, PRs in the sub-modules coreos-overlay and portage-stable are not covered. Currently, to make use of builds+tests, one would need to

1. create a branch in the submodule repo, commit changes, create a PR on the submodule repo
2. create a branch in the scripts repo, update the respective submodule to include one's change, commit+push, and open a PR on the scripts repo
3. wait for the build/tests to conclude on the scripts PR. If build or tests fail
   a. push fix to the submodule PR branch
   b. update submodule ref and push to the scripts PR branch
   c. re-trigger build, goto 3.
4. merge the submodule repo PR
5. merge the scripts repo PR

Proposed solution

The path forward proposed here is to automate as much as possible of the manual workflow laid out above, and to run builds and tests directly on the submodule branch via Github reusable workflows (i.e. make ci-yaml reusable). User-visible / interactive workflow would be:

1. Create branch / PR in the submodule repo
2. Trigger a build + test run (when PR is either "Ready for review" or "review requested")
3. push changes to the submodule PR branch if necessary (build / test breakage) and trigger rebuilds via the submodule PR
4. Merge the PR in the submodule repo

Out of scope

PRs that require changes in both submodule repos are currently out of scope (though discussion of options in this thread is strongly encouraged!).

Automation / Implementation options

When a build/test is triggered in a submodule repo PR (i.e. step 2. above)

1. Automation creates a "tracking branch + tracking PR" in scripts if these do not exist yet. If these exist they are re-used.
   a. Target branch of the scripts PR is the same as for the submodule PR. If that branch does not exist in scripts then automation terminates.
   b. The scripts tracking PR is prefixed with [<SUBMODULE REPO NAME>] and the PR's summary line is copied verbatim from the submodule repo PR
   c. The scripts tracking branch is prefixed with <submodule repo name>/. It consists of a single commit which only includes one change: update the submodule ref. The ref is amended and force-pushed every time the submodule PR re-triggers a build+test.
2. Automation runs build + test on the submodule PR (not the scripts tracking PR) based on the scripts repo commit ref from above. This makes build / test feedback directly on the submodule PR.

When a submodule PR was merged

1. If a respective tracking PR / tracking branch exists in the scripts repo, that branch will be updated to point to the new tip of the respective submodule's branch. Then the PR will be merged, i.e. the respective submodule ref will be updated. PR is then closed and the tracking branch is deleted.

When a submodule PR is closed w/o merge

1. If a respective tracking PR exists in the scripts repo, that PR will be closed and the tracking branch deleted.

Challenges / pitfalls

1. To maintain reproducible builds (one-stop check-out and build) and also to make the devcontainer test succeed, any submodule change must have a corresponding commit in the scripts repo (e.g. in a branch). Hence the complicated
2. PRs to branches (e.g. maintenance branches) in the submodule must target branches of the same name in the scripts repo.
3. cherry-picks to maintenance branches need to be handled. These could e.g.
   a. bypass automated builds / tests entirely and be merged straight away
   b. cherry-pick script could be updated to create PRs instead of commits, causing the automation to jump into gear (though this could easily overwhelm our self-hosted runners)

Side effects

1. Updates to a submodule's main branch or to maintenance branches will now propagate to the respective branch in scripts immediately on merge instead of once a night via the nightly builds.

Alternatives

An alternative to the above automated cross-PRs is currently undergoing RFC here: flatcar/scripts#699 . This would merge both submodules right into scripts, making scripts the one and only repo for Flatcar builds. It would simplify automation significantly but would mean a noticeable up-front investment of effort (and also has a risk of causing build breakage as a side effect).

[gabriel-samfira]

Apparently, we can trigger a workflow based on issue comments/pr comments:

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issue_comment

The event object exposes the body and the user that created it. It may be possible to allow triggers such as commenting on a PR with something like:

/run-tests

from users that are added as collaborators to the repo or that are part of the org.

[t-lo]

Good find - we'd need to add an allow-list of users to the workflow, but I agree it's a better UX.

[t-lo]

I just ran into a very good reason to switch to a PR comment for triggering builds rather sooner than later. I put a PR into "Ready for review", which started a build. Then, almost 2h later, I requested reviews from other maintainers - that cancelled my ongoing build and started a new one. This can happen involuntarily and waste hours of work (well, hours of waiting at least).

TIL.

[pothos]

PRs without a review required don't seem to run tests, that's also a bit unexpected as it means that test results will only be available later while the reviewers already got notified.

[t-lo]

PRs should run tests also when "ready to review". I.e. either when creating a PR (and not setting it to "draft") or when transitioning it from draft state to "ready for review". If that doesn't work reliably then that's one more reason to switch to PR comments for triggering builds.

[pothos]

That works, I just forgot to request reviews