Considerations for the responsible use of GitHub Actions

[jamesmbaazam]

We've had a lot of internal discussions on the responsible use of GitHub Actions. Some points have been laid out in this article by an RSE team at Imperial. A lot of these are already implemented here. However, more can be done.

EpiNow2 still has some really long CI runtimes and this is continually being addressed. See the most recent change in #722, for example.

This issue is to gather ideas on what more can be done to improve our CI usage. Here are a few ideas that have come up:

1. Manual triggering OR configuring CI to only trigger when a PR is marked ready for review. This will require us to adopt using draft reviews as part of our process. There are ways this can be done, for example, triggering CI on PR's with a set label
2. Using pre-commit hooks more religiously (noting here that this sometimes introduces changes not relevant to the current PR)
3. Being more sensible about changes in files that trigger CI (e.g. changes to workflow files shouldn't trigger R CMD checks)
4. Running R CMD check locally although it takes a while with EpiNow2, which means one has to wait for it to be completed or buy a second work computer 😆 .

Thoughts? @sbfnk @seabbs @Bisaloo

Current agreements

• Don't run CI on draft PRs; trigger tests only when PRs are marked ready for review:
  - [ ] Requires changing trigger to "pull_request".
• Enforce the use of pre-commit hooks for lining, roxygenizing, etc. These already exist and just need to be done as part of the contribution workflow.
• Run R CMD Check locally before pushing changes.
• Use the GitHub Action tag's ([ci skip], [skip ci], [no ci], etc) that signal to skip the CI like discussed here

[sbfnk]

1. Manual triggering OR configuring CI to only trigger when a PR is marked ready for review. This will require us to adopt using draft reviews as part of our process. There are ways this can be done, for example, triggering CI on PR's with a set label

I think this is a good idea - i.e., trigger checks only when a PR is either marked ready for review or already ready for review. Draft PRs don't trigger any checks. This can be done using the pull_request events.

2. Using pre-commit hooks more religiously (noting here that this sometimes introduces changes not relevant to the current PR)

This seems generally a good idea especially for the faster actions, e.g. linting (see also (4)).

3. Being more sensible about changes in files that trigger CI (e.g. changes to workflow files shouldn't trigger R CMD checks)

I think looking at changes to which files should trigger an R CMD check and codecov makes a lot of sense.

4. Running R CMD check locally although it takes a while with EpiNow2, which means one has to wait for it to be completed or buy a second work computer 😆 .

I don't really see the issue here as I can't imagine delaying a push/PR by half an hour is often an issue (except e.g. if running out of battery).

[sbfnk]

Manual triggering OR configuring CI to only trigger when a PR is marked ready for review. This will require us to adopt using draft reviews as part of our process. There are ways this can be done, for example, triggering CI on PR's with a set label

Another option here would be e.g. to run the R CMD check workflows only on one platform (e.g. macos) and then on all platforms upon entering the merge queue (also would require setting up a merge queue)

[jamesmbaazam]

1. Manual triggering OR configuring CI to only trigger when a PR is marked ready for review. This will require us to adopt using draft reviews as part of our process. There are ways this can be done, for example, triggering CI on PR's with a set label

I think this is a good idea - i.e., trigger checks only when a PR is either marked ready for review or already ready for review. Draft PRs don't trigger any checks. This can be done using the pull_request events.

Draft PRs currently trigger checks. See #695 for example.

2. Using pre-commit hooks more religiously (noting here that this sometimes introduces changes not relevant to the current PR)

This seems generally a good idea especially for the faster actions, e.g. linting (see also (4)).

Then we need to fix all the liting issues in main asap.

3. Being more sensible about changes in files that trigger CI (e.g. changes to workflow files shouldn't trigger R CMD checks)

I think looking at changes to which files should trigger an R CMD check and codecov makes a lot of sense.

I'm a little wary of this approach as it could cause loop holes in the checks.

4. Running R CMD check locally although it takes a while with EpiNow2, which means one has to wait for it to be completed or buy a second work computer 😆 .

I don't really see the issue here as I can't imagine delaying a push/PR by half an hour is often an issue (except e.g. if running out of battery).

[sbfnk]

Draft PRs currently trigger checks. See #695 for example.

Yes, sorry I wasn't clear - that was meant as part of the desired future state.

[jamesmbaazam]

I'm updating the description with things we are agreeing on. These can be logged as issues later when we have a final list.

[jamesmbaazam]

Another thing that is noted in the linked article is that some Runners are faster than others. So we should benchmark all the workflows between Ubuntu-latest vs macoS latest. The key ones are:

• test-coverage.yaml (addressed in coverage on macos #722)
• synthetic-validation.yaml
• stan-model-benchmark.yaml
• R-CMD-check.yaml
• R-CMD-check-as-cran.yaml

[seabbs]

I don't really see the issue here as I can't imagine delaying a push/PR by half an hour is often an issue (except e.g. if running out of battery).

I think for this point and all other need to stop and think about the burden placed on new contributors (if you want any).

[seabbs]

We are all using precommit here: https://github.com/cdcgov/Rt-without-renewal

and its working really well but I don't think when we spin it out we will require all contributors do so (we will just keep it running in the CI as we do now)

[jamesmbaazam]

Is another option to use the CI tag's ([ci skip], [skip ci], [no ci], etc) that signal to skip the CI like discussed here?

[sbfnk]

Yes, and we do use these at times. However, it is again that is probably only available to those who are already package developers.