Proposal on consistent approach for running GitHub actions #3961
Conversation
|
Just to add a bit of extra background: the practical motivation for eliminating the duplicate runs is that in at least one repo (govuk-developer-docs) they've been exacerbating flakiness because of expensive tests exhausting quotas and this has been a non-negligible time sink for Replatforming team because we're trying to be helpful and make lots of updates to existing documentation in that repo. |
There was a problem hiding this comment.
Thanks! This looks good to me 👍
Definitely worth getting @theseanything's input as well.
source/manual/test-and-build-a-project-with-github-actions.html.md
Outdated
Show resolved
Hide resolved
source/manual/test-and-build-a-project-with-github-actions.html.md
Outdated
Show resolved
Hide resolved
| ``` | ||
|
|
||
| We previously recommended CI jobs to run on `[push, pull_request]` as per | ||
| [RFC-123][] however this caused an appearance of duplicate runs of a CI job |
There was a problem hiding this comment.
It definitely runs real duplicate jobs, unfortunately. Disappointing that GitHub doesn't just solve this out of the box, as it's a sore point for so many of its users :(
| [RFC-123][] however this caused an appearance of duplicate runs of a CI job | |
| [RFC-123] however this caused duplicate runs of a CI job |
There was a problem hiding this comment.
Worth updating the PR description and commit message too, as they still reference "appearance of", @kevindew
source/manual/test-and-build-a-project-with-github-actions.html.md
Outdated
Show resolved
Hide resolved
|
Happy to help with rolling this out btw! |
|
Thank you @theseanything and @sengi ⭐ . I'll make amends to this later and seek a wider input before merging. |
|
Thanks @theseanything and @sengi, I think I've got all your suggestions in now, do shout if I've got any wrong. I'll go ask in #govuk-developers |
.github/workflows/ci.yml
Outdated
| workflow_dispatch: | ||
| inputs: | ||
| ref: | ||
| description: 'The branch, tag or SHA to checkout' |
There was a problem hiding this comment.
Nit: we can omit these quotes I think.
| tested and because `pull_request` merges the changes with | ||
| the repository default branch. |
There was a problem hiding this comment.
I don't understand this last bit. Don't GitHub actions run against the branch as-is, whether or not the trigger was push or pull_request?
There was a problem hiding this comment.
So I only found out about the details of this recently myself (thanks to @kevindew for putting me onto it) — pull_request uses the refs/pull/<pr_number>/merge reference that GitHub uses for setting up the merge. (I believe this is essentially the PR branch rebased onto the destination branch, but don't quote me on that.) Compare https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request with https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#push
If the PR branch is up-to-date with main then there should be no practical difference, but if they've diverged significantly and "Require branches to be up-to-date before merging" isn't enabled (which it shouldn't normally need to be — in fact I'd guess that's why the pull_request trigger uses the merge ref rather than the PR branch as-is) then it could mean you're testing against an old version of the rest of the codebase.
There was a problem hiding this comment.
Thanks for the explanation! Perhaps we should amend that sentence to link back to your comment? 🙏
This has been produced following the discussiona in: #3950 and #3816 This is to try resolve a consistency issue we have in GOV.UK where we have an agreed approach to using GitHub Actions documented which doesn't reflect the configurations used across GOV.UK. Since we agreed the approach of running CI GitHub Action on `[push, pull_request]` and adopted it has become clear that this is not unanimously popular and, due to the appearance of duplicate jobs, can imply misconfiguration. The motivation to document a new approach is to try form consensus on an approach that can be good enough for most cases and cover common concerns. This can then reduce the time we spend debating how to configure individual projects. Aspects I've wanted to cover in this are: - Stay broadly consistent with approach replatforming popularised since that is what is predominantly in use - Remove the appearance of duplicate runs by default in pull requests - Allow CI for external contributors (our [current process for dealing with them][1] seems more of a legacy Jenkins concern) - Offer an approach for running CI without opening a PR, as that seems to be a common point of contention (it's a key part of some workflows, yet not a concern for others). A risk of this approach is that the use of workflow_dispatch is somewhat ignored and thus unreliable. This doesn't seem a huge concern to me as I imagine this would be fixed if it's used frequently or be a sign that it can be removed. [1]: https://docs.publishing.service.gov.uk/manual/merge-pr.html#a-change-from-an-external-contributor
|
Thanks all! |
This changes the approach to how this GitHub Action is run to reflect the change in standards in GOV.UK [1] [1]: alphagov/govuk-developer-docs#3961
This changes the approach to how this GitHub Action is run to reflect the change in standards in GOV.UK [1] [1]: alphagov/govuk-developer-docs#3961
This utilises the guidance introduced in: alphagov/govuk-developer-docs#3961
This utilises the guidance introduced in: alphagov/govuk-developer-docs#3961
This utilises the guidance introduced in: alphagov/govuk-developer-docs#3961
This updates the approach to running this workflow to reflect the approach in: alphagov/govuk-developer-docs#3961
This updates the approach to running this workflow to reflect the approach in: alphagov/govuk-developer-docs#3961
This updates the approach to running this workflow to reflect the approach in: alphagov/govuk-developer-docs#3961
This utilises the guidance introduced in: alphagov/govuk-developer-docs#3961
This has been produced following the discussiona in: #3950 and #3816
This is to try resolve a consistency issue we have in GOV.UK where we have an agreed approach to using GitHub Actions documented which doesn't reflect the configurations used across GOV.UK.
Since we agreed the approach of running CI GitHub Action on
[push, pull_request]and adopted it has become clear that this is not unanimously popular and, due to the appearance of duplicate jobs, can imply misconfiguration.The motivation to document a new approach is to try form consensus on an approach that can be good enough for most cases and cover common concerns. This can then reduce the time we spend debating how to configure individual projects.
Aspects I've wanted to cover in this are:
A risk of this approach is that the use of workflow_dispatch is somewhat ignored and thus unreliable. This doesn't seem a huge concern to me as I imagine this would be fixed if it's used frequently or be a sign that it can be removed.