-
-
Notifications
You must be signed in to change notification settings - Fork 307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CI: add CI concurrency #2868
CI: add CI concurrency #2868
Conversation
I intentionally inserted an unnecessary comment in commit 7f11088, which I used to test the cancelling with a following commit that removed the same. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While an explicit "is this a PR" check is missing compared to #2170, the code makes sense in comparison to my original draft (I did not check the documentation now).
The CIs may be bogged down after a swift sequence of merged PRs, checking every commit (again, after all they are checked on PR before merge) on main doesn't seem to be absolute priority. Possibility of cancelling on main if needed is deliberate. |
Oh, no, then I'm against this. I think we want the confidence of checking every commit on main because that's actually only place where multiple PRs are combined together. The PRs are not updated to the latest code on main. There is a settings to require that, but we don't have that enabled right now. (See also merge queue which makes enabling it little more feasible.) Even if we would enforce updating PRs before merge, I want to be able to check that main is okay without tracking down whether the corresponding PR passed. I want to see green check boxes on main and not to investigate if the red x means failed, canceled with timeout or canceled by new priority (which now I only get in Details > Summary). Please, consider canceling only for PRs. |
I don't agree. I'd say it is generally quite easy to track down eventual main failures, given a limited number of merges/commits on main. The advantages however – say on a sprint tempo collective work, with loads of merges and active PRs in work – are in my opinion outweighing any lack of green tick next to every commit. |
In corner cases, it is always possible to re-run jobs. |
Enabling the required update to base branch and merge queue would be than prerequisite for merging this, then. Otherwise, the actual combined code is potentially never checked before hitting main and then not checked even there when canceled. |
In general and in particular given the latest changes on CI checks, it would be bad practice not to update (rebase/merge) to main on older PRs before merge. Perhaps Always suggest updating pull request branches wouldn't be such a bad option anyway, but I don't know how to make that required (if at all possible).
It is tested together with the overriding commit. If that fails and the reason cannot be the later commit, the canceled commit checks can be re-run. |
Now we have this enabled for main:
Now each old PR has a button to update branch to base by merge or rebase which is nice. |
I did not enable the merge queue, that needs more study. |
Great! |
Shall we hava a go with this then? |
+1 for merging this from me. One caveat here is that more runs will be potentially done on PRs now because they need to be updated to latest base branch.
I think I have seen really sophisticated selections at WordPress or Django, but I was never able to tease out what would be the things to skip in our case. Our HTML is part of the build, Python code is processing HTML, Super-Linter checks more than one thing... But I think that's also where I got the idea of using concurrency on PRs but not main. |
I am not in favor of updating each PR to main, this will be annoying because you need to wait for all the checks and while you are waiting, main branch can be changed and you need to wait again. So I would prefer the solution @wenzeslaus suggests if possible, cancel the CI runs only for PRs, I think that's the main source of CI waiting anyway. If we want the PRs updated to main, I would much rather use the merge queue (once somebody figures it out). |
Looking a bit on this, it looks very promising, probably what is needed instead of req. rebase. Some related links: https://github.blog/changelog/2023-02-08-pull-request-merge-queue-public-beta/ https://blog.mergify.com/what-is-a-merge-queue/ https://discuss.python.org/t/enabling-github-merge-queue/23736 |
With merge queues (even older) PRs may be “merged”, i.e sent to the queue where it will be tested against main, before finally be actually merged (if passed). At least no need for maintainer to wait as @petrasovaa mentioned. |
I’m a bit afraid of the timeouted tests though… |
This reverts commit b5f6bd7.
Been giving this some further thought. The "Require branches to be up to date before merging" setting is for GRASS worse than CI runners getting bogged down. Having to wait for approx. 2 hrs between every merge is more than annoying. Please turn that off, it is a question for another PR/discussion. I updated this PR to cancel jobs only in pull requests. For the record I must add that I do this reluctantly, because I believe it to be a waste of CI runs, but it's still a great improvement to present situation. |
Done. |
Thanks all for your input! |
I see the jobs being canceled on main, too, e.g., b38d109. |
Just noticed, looking at it. |
You might want to take a look at the "environment" option in workflows, where only one of the workflows can execute at a time, it is quite similar to a mutex in fact. |
Seems there is no easy way for this. What we have is "cancel-in-progress" = false for main. This means already started jobs are NOT cancelled, but jobs that are pending are cancelled and there is apparently no way to set this (see eg. https://github.com/orgs/community/discussions/49729). |
...using cancel in progress, but isn't that what concurrency group is supposed to take care of? In #2170, I used: group: ${{ github.workflow }}-${{ github.event_name == 'pull_request' && github.head_ref || github.sha }} which was posibly broken, but the idea was that commit outside of PR has its own group. I think that's what the |
I think that's the idea in this WordPress code too: .github/workflows/coding-standards.yml#L36-L41(WordPress/wordpress-develop@8624529) |
Cancels jobs in process in pull requests only.
Cancels jobs in process in pull requests only.
Enables CI concurrency setting
cancel-in-progress
for most of the workflows.Liberates a huge amount of CI processing in situations with a lot of added commits "upon" each other, e.g. in pull requests that are in practice WIP.