matrei opened a new pull request, #16726:
URL: https://github.com/apache/grails-core/pull/16726
## Summary
This is a follow-up to #16673. It contains the review points that were still
open when it was merged: @codeconsole's change request and the second review
round.
### Publish chain after a cancel
**The problem.** `publishGradle` needed only `buildGradle`. Since #16673, a
newer push cancels the run in flight, and the publish jobs run on `always()`.
So on any push run, a cancel more than a few minutes in still published part of
the superseded commit:
- **Run with tests:** the Gradle plugins were published, but not the core.
8.0.x run 39638916842 shows this. Its `publishGradle` published at 17:13, and a
newer push cancelled the run at 17:26, before its tests had finished.
- **Run without tests** (every push to 7.x): the plugins, core and docs were
published, but not Forge, which was still building. @codeconsole found this
case.
**The fix.** `publishGradle` now also waits for `buildForge` and
`validateDependencies`, with the same explicit `success` checks. `publish` and
everything after it already wait for `publishGradle`, so:
- A cancel before the builds finish publishes nothing of that commit.
- A commit whose Forge build or dependency validation failed is no longer
published.
- A cancel after the builds still lets the chain publish what it can:
- on a run without tests, the whole chain;
- on a run whose tests are still running, only the Gradle plugins;
- on a run whose tests have all passed, the whole chain.
The comments in `gradle.yml` and the CI policy section of `AGENTS.md` now
describe this, instead of saying that a cancelled commit is never published.
**The cost** is that the plugin snapshot is published later. On a 7.0.x push
without tests it starts after the Forge build, about 10 minutes in instead of
about 5. On 8.0.x the Forge build takes 35 to 45 minutes, so the plugin
snapshot can appear up to about half an hour later. The core publish on a run
with tests is unchanged, because it already waits for tests that take longer
than either build.
### Docs
- **`AGENTS.md`:**
- The snapshot publish is no longer listed among the jobs that run on pull
requests. It runs on a push, and on a manual run except for the docs.
- The CI policy section names force-cancel (`gh api -X POST
repos/apache/grails-core/actions/runs/<run id>/force-cancel`) for a publish
that a normal cancel cannot stop. The upload's retry step lets a stuck publish
keep running for hours.
- **`CONTRIBUTING.md`:** no longer says that every push to a branch cancels
the run in flight. Only CI and the Groovy joint build do; for drafts, the
relevant case is a push to a pull request, which every workflow cancels.
- **`RELEASE.md`:** notes that a merge to the release branch cancels the
manual test run, because both share a concurrency group.
## Not changed here, for discussion
These are policy questions rather than bugs, so they are left for the weekly:
1. **`[skip tests]` on pull requests from forks.** Since #16673, a `[skip
tests]` head commit skips the tests on a pull request into 8.0.x and above too,
and on a fork pull request the author picks that commit. Should the marker
count on a pull request only when
`github.event.pull_request.author_association` is `OWNER`, `MEMBER` or
`COLLABORATOR`? (@codeconsole)
2. **`run ci` on drafts.** On a draft, `run ci` now also runs the full test
matrix, so a draft can't get a build-only run. A draft into a side branch that
is labelled to get any CI now runs everything. Should a separate `run tests`
label ask for the tests, so that `run ci` only means "run CI on this draft"?
## Verification
- **Job-graph check:**
- Every workflow parses, and in each one every `needs` resolves.
- Every job that reads a `needs` result lists that job, there is no cycle,
and every `always()` job checks the result of every job it needs.
- The same check passes on the result of merging this into 8.0.x, which
`git merge-tree` reports as conflict-free.
- **actionlint 1.7.7:** clean on the changed workflows. Its shellcheck notes
are all in scripts this change doesn't touch.
- **`./gradlew rat`:** passes.
- **Publish chain under cancels.** I modelled `gradle.yml`'s job graph with
GitHub's documented rules and cancelled it at every minute. The timings are
rounded from the 7.0.x merge push of #16673: builds end at 4, 5 and 11 minutes,
and tests at 61.
| Run | Cancel arrives | Before (publishGradle needs buildGradle) | After
(it needs every build) |
|---|---|---|---|
| without tests | before `buildGradle` ends | nothing | nothing |
| without tests | after `buildGradle`, before `buildForge` ends | plugins,
core, docs | nothing |
| without tests | after every build | everything | everything |
| with tests | before `buildGradle` ends | nothing | nothing |
| with tests | after `buildGradle`, before `buildForge` ends | plugins |
nothing |
| with tests | after the builds, tests running | plugins | plugins |
| with tests | after every test passed | everything | everything |
- **First real run.** The publish jobs only run on a push in `apache`, so
the merge push to 7.0.x is the first real run of this change. On that run,
`publishGradle` should start after `buildForge`, and `publish`, `publishForge`,
`verifyWrapper` and `docs` should follow as before.
--
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]
