jdaugherty commented on PR #16673: URL: https://github.com/apache/grails-core/pull/16673#issuecomment-6293452916
Thanks for the thorough pass, including the local cases. Everything below is folded into the single commit (now b9423fb2d1); the description is updated to match. ### 1. Cancelling a push run mid-publish Fixed, but not by restoring the pull-request-only cancel: we do want a new commit to stop the build and test jobs of the superseded run, so the publish chain is exempted from the cancel instead. - `publishGradle`, `publish`, `publishForge`, `verifyWrapper` and `docs` now run on `always()` with the explicit result checks (`publishGradle` had no status function at all before; it uploads too). GitHub re-evaluates the `if` of every running job when a run is cancelled and stops only those that turn false, so a publish that has started completes. - A cancel that arrives before `publish` starts publishes nothing of that commit: the cancelled test jobs fail the explicit result checks. - The replacing run waits. The docs do not state it, so I checked last week's data: in all 890 cancelled-then-replaced pull-request runs, the new run's first job never started before the cancelled run's last job had completed, although the cancelled jobs took up to 93 s (90th percentile) and 38 min (maximum) to stop after the cancel request. The concurrency group is held until the cancelled run has finished every job, so two publishes of one branch cannot overlap. - The cost is that a manual cancel during a publish also lets it finish (15 to 35 min). A half-published snapshot set is never what the person wanted, so that is taken as a feature. - `publishGradle` still runs the moment the Gradle plugins are built, as it always has, so the window in which the plugin snapshot is one commit ahead of the core snapshot is unchanged in kind. ### 2. The below-the-default rule You are right about the split, and my "45%" in the reply to Søren lumped the side branches in with 7.x. Recut from the same week's data: | | Runner hours | Of which tests | Share of the week | |---|---|---|---| | Side-branch pull requests | 7,897 h | 7,077 h | 46.5% | | Pull requests into 7.x | 384 h | 332 h | 2.3% | | Pushes to 7.x | 279 h | 227 h | 1.6% | Of the 29 pushes to 7.x, 13 ran the tests at about 21 h each; the other 16 were `[skip tests]` merge-ups at under an hour each. The four weeks before had 19 to 29 pushes to 7.x a week. The rule stays for pushes, deliberately: the 7.x branches are in heavy backport mode, every backport is one push to 7.0.x plus a merge-up to 7.1.x and 7.2.x, and we already mark the merge-ups `[skip tests]` by hand. The tests belong on the pull request, and that is where this round makes them easy to ask for: - The **`run ci` label** now also runs the tests on a pull request whatever its base (7.x or a side branch), so a backport pull request is tested by labelling it before the push instead of rewriting its head commit. It is read when the run starts, like the draft gate, and a `[skip tests]` head commit still wins. - For the release commit, your fallback: `workflow_dispatch` of CI (and of the Groovy joint build) has a boolean **`run-tests`** input, default off, and RELEASE.md gains the step with the `gh workflow run ... -f run-tests=true` command. A dispatch in `apache` without the input follows the branch rule as before. - The merge box: the policy job now also emits `::notice title=CI policy::Tests did not run: <reason>`, so the run summary says it without opening the job. The merge push itself stays untested unless the squashed commit's title starts with `[run tests]`; that is the intended trade. ### 3. Smaller points - **Release Drafter exception**: named in rule 14, the CI policy section, CONTRIBUTING.md and the description. - **Branch lists**: 7.1.x added; the RAT and CodeQL sentence now says they run on pull requests into release branches only, since their triggers list only those bases. - **`no ci` as a cancel**: you are right, my "one-click cancel" was wrong. The docs now say that adding `no ci` does not stop a run in flight. - **Notice when tests are skipped**: added, with only fixed text and branch names in it; the local harness checks that it appears exactly when `run-tests` is `false`. - **Verification section**: rewritten. The pull request now carries the `run ci` label, so the run at the new head exercises the label path on a pull request into 7.0.x; the build-only path ran at 86e2ae7c28. The publish chain first runs for real on the merge push to 7.0.x, and I will check `publishGradle`, `publish`, `publishForge`, `verifyWrapper` and `docs` on it. - **Merge-up**: agreed, same person, right after the merge; the conflicts you listed and the `groovy-joint-workflow.yml` deletion on 8.0.x are in the follow-ups. - The local harness is up to 23 cases (the new ones: label with and without `[skip tests]`, dispatch input with and without it, a fork dispatch, 7.1.x, an empty message, two and five failed reads), and a structural check over every workflow confirms that each `always()` job checks the result of every job it needs and that every job of every pull-request workflow other than `release-notes.yml` is gated by `policy` or the label-and-draft `if`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
