Github Action pull_request behaviour

[ptheywood]

tldr

• pull_request CI runs are semi-useful at best, spam at worst.
• Do we want to reduce the frequency of them by either:
  1. Do not run on pushes, only running on open/repoen and when ready_for_review
  2. Run on pushses to pr branches (sync), open/reopen/ready, but only for non-draft PRs.
  3. Remove pull_request events for some or all workflows
  4. setup manual dispatch triggers for each CI, which if enabled runs the ci on the pull request version. Should be possible, might not. (Edit: simple to add, difficult to make it nice. )

I think I like option 2 after typing this out.

Ideally this should also include building the test suite on windows, which is not enabled on push event CI

Edit

The decision so far is to disable pull_request workflows (part of #644), and in the less urgent future find an alternative way of manually invoking checks for a pull request, against the potential merge.

• [ ] Add manually invoked (or otherwise infrequent) checks on the potentially merged pr.
  • Don't run if draft (will show lots of skips)
  • dynamic build matrix step, which may skip.
  • Bot-like approach, via comments from allowed authors, and using gh / api calls to manually invoke runs and register them as checks on the PR?

Edit 2024-01-08

current push only events do not get triggerd for pull requests from forks, so no ci gets run on them (it would be guarded behind maintainer approval otehrwise. See https://github.com/FLAMEGPU/FLAMEGPU2/issues/658#issuecomment-1880791965) Should also change the org/repo setting to require approval on all third party contributors, not just first time.

Much longer than planned description

Currenlty, our regular ci options (lint, docs, Ubuntu, Windows) are executed when any branches or tags are pushed, and on the default pull_request events.

As of #644, the regular CI will run for :

• any branch push events, unless they only modify irrelevant files in the .github directory.
  • I.e. a push which only changes Windows.yml or install_cuda_windows.sh will not trigger a Ubuntu.yml workflow run.
• Any default pull_request operation, including synchronisation (pushes to the branch).

The pull_request jobs build the code as if the pull request had been merged at that time. This is useful because it can help prevent issues post-merge, however if the base branch has been changed since the CI workflow executed, then it is out of date information, and potentially not super useful. I.e. if we merge 5 PRs during a meeting, only the first PR to be merged had relevant pull_request CI information. This process also results in twice the number of CI runs per branch push, whcih in general isn't an issue but if we have many active branches each running multiple 30 minute+ ci jobs it can lead to some queuing (pushing to 5 pr branches within a 30 minute window woudl probably cause this).

We could reduce the number of CI runs that run for every branch push, where the branch is in a pull request by changing the types of pull request events it is triggered by, to not include the syncrhonisation default option, but none of the options are perfect, hence we currently use the defaults.

Pull request trigger docs

The default types are:

    types: [opened, synchronize, reopened]

There are a few options:

Replace synchronize with ready_for_review, so the head of the branch when the PR is opened, reopened or when the state changes from draft to not draft.

    types: [opened, reopened, ready_for_review]

Alternatively, we could just add ready_for_review, as well as us job conditions statements to only run on non-draft PRs. Github no longer shows skipped runs as failures on commits which makes this viable.

    types: [opened, synchronize, reopened, ready_for_review]

jobs:
  job_name:
    # Run if not triggered by a pull request, or by a non-draft PR
    if: ${{ github.event.pull_request == null || github.event.pull_request.draft == false }}

This would cause the pull_request jobs to only run on non-draft pull requests, but then run on sync updates

Alternatively we could just remove pull_request workflows entirely, or remove them from some workflows (where merging is unlikely to cause issues, such as docs / )

[ptheywood]

For now, the decision is to disable the pull_request workflows, adding a less frequent / opt-in way of running these jobs on the post-merge target.

This could be via manual dispatch with arguments to specify the branches to try, or potentially we could use a pr comment worfklow to allow users with manual write access to invoke this (i.e. a bot-like workflow). I've experimented with both of these before but it would need a bit of work.

Another alternative would be to prefix each workflow with a very simple job to decide if that workflow should run, and if so generate the matrix of actual jobs to run, but this would get quite soupy.

[ptheywood]

One downside to our current exclusive use of push events to trigger CI, is that if you push multiple commits at once, this only triggers for the HEAD commit. If you then open a PR from one of the other commits in the push, it doesn't seem to trigger the CI as it's not a new commit? (unless github is just having an actions outage I don't know about).

e.g.

git commit -am "a"
...
git commit -am "b"
... 
git commit -am "c"

git push -u origin PR-c # CI triggered

git checkout -b PR-b 
git reset --hard HEAD~1
git push -u origin PR-c # CI not triggered

[ptheywood]

Currently our CI is all push event based, which does not trigger for pull requests from forks (since #644).

This limits its usefulness for third-party contributions (but avoids security concerns with executing third party code, although approval would be required for first-time contributors).

Ideally we should have some pull_request event's that trigger this, but having push and pull_request events on all our workflows with their job matrices would create 40 workflows per push to an internal branch which has a pull request, and 20 per non pr push or 20 per approved third party pull request.

This would need some thought to do nicely (ideally something with reusable workflows, and dynamic build matrices would let us find a good balance between thorough-ness and ci spam)