Repository navigation
Publish nothing of a CI run cancelled before its builds finish, and correct the CI policy docs #16726
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
Publish nothing of a CI run cancelled before its builds finish, and correct the CI policy docs #16726
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,9 +31,15 @@ on: | |
| # results would be for a superseded commit, and the run that replaces it publishes the newer one. The publish | ||
| # jobs (publishGradle, publish, publishForge, verifyWrapper, docs) run on always() with explicit result checks | ||
| # instead, so a publish that has started completes: an interrupted upload would leave the snapshot repository | ||
| # with modules from two commits. Nothing of the cancelled commit is published when the cancel arrives before | ||
| # publish starts, because a cancelled test job fails the explicit checks. The replacing run is pending until | ||
| # the cancelled run has finished every job, so two publishes of the same branch never overlap. | ||
| # with modules from two commits. publishGradle waits for every build job (buildGradle, buildForge and | ||
| # validateDependencies), so nothing of the cancelled commit is published when the cancel arrives before the | ||
| # builds have finished. A later cancel lets the publish jobs whose needs have already succeeded or been skipped | ||
| # run: on a run without tests that is the whole chain, and on a run whose tests are still running only | ||
| # publishGradle, because a cancelled test job fails the explicit checks of publish and of everything after it. | ||
| # A cancel after every test has passed lets the whole chain run as well. The replacing run is pending until the | ||
| # cancelled run has finished every job, so two publishes of the same branch never overlap, and it then publishes | ||
| # the newer commit over whatever the cancelled run published. A normal cancel does not stop a running always() | ||
| # job either; the CI policy section of AGENTS.md says how to force one. | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
@@ -536,14 +542,18 @@ jobs: | |
| -PonlyHibernate5Tests | ||
| -PskipCodeStyle | ||
| publishGradle: | ||
| # always() with an explicit result check, see the concurrency comment at the top. | ||
| # always() with explicit result checks, see the concurrency comment at the top. It waits for every build job, | ||
| # not only buildGradle, so that a run cancelled before its builds have finished publishes nothing, and a | ||
| # commit whose Forge build or dependency validation failed is not published. | ||
| if: >- | ||
| ${{ always() && | ||
| github.repository_owner == 'apache' && | ||
| (github.event_name == 'push' || github.event_name == 'workflow_dispatch') && | ||
| needs.buildGradle.result == 'success' | ||
| needs.buildGradle.result == 'success' && | ||
| needs.buildForge.result == 'success' && | ||
| needs.validateDependencies.result == 'success' | ||
| }} | ||
| needs: [ buildGradle ] | ||
| needs: [ buildGradle, buildForge, validateDependencies ] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This merges into 8.0.x and 9.0.x without conflicts, but both branches also have
It's worth adding the three jobs to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. With publishGradle:
needs: [ buildGradle, buildForge, validateDependencies, validateBomProperties, validateGithubBranch, buildLogic ]
publish:
needs: [ publishGradle, build, buildRerunTasks, functional, security, securityConfig, casSecurityConfig, redis, hibernate5Functional, hibernate7Functional, mongodbFunctional, neo4jFunctional ]It also needs the matching result checks ( |
||
| runs-on: ubuntu-24.04 | ||
| steps: | ||
| - name: "Output Agent IP" # in the event RAO blocks this agent, this can be used to debug it | ||
|
|
@@ -588,17 +598,19 @@ jobs: | |
| name: grails-gradle-artifacts.txt | ||
| path: grails-gradle/build/grails-gradle-artifacts.txt | ||
| publish: | ||
| needs: [ publishGradle, build, functional, security, securityConfig, redis, hibernate5Functional, mongodbFunctional ] | ||
| needs: [ publishGradle, build, buildRerunTasks, functional, security, securityConfig, redis, hibernate5Functional, mongodbFunctional ] | ||
| # always() with explicit result checks, see the concurrency comment at the top. The results are checked | ||
| # explicitly because the implicit success() check also covers the skipped test jobs upstream, which would | ||
| # skip this job on a build-only run, and because a cancelled test job must not be published: nothing of a | ||
| # commit whose run was cancelled before this job started reaches the snapshot repository. | ||
| # skip this job on a build-only run, and because a cancelled test job must not be published: the core of a | ||
| # commit whose tests were cancelled never reaches the snapshot repository, although its Gradle plugins may. | ||
| # Every test job of this workflow belongs in these needs. | ||
| if: >- | ||
| ${{ always() && | ||
| github.repository_owner == 'apache' && | ||
| (github.event_name == 'push' || github.event_name == 'workflow_dispatch') && | ||
| needs.publishGradle.result == 'success' && | ||
| (needs.build.result == 'success' || needs.build.result == 'skipped') && | ||
| (needs.buildRerunTasks.result == 'success' || needs.buildRerunTasks.result == 'skipped') && | ||
| (needs.functional.result == 'success' || needs.functional.result == 'skipped') && | ||
| (needs.security.result == 'success' || needs.security.result == 'skipped') && | ||
| (needs.securityConfig.result == 'success' || needs.securityConfig.result == 'skipped') && | ||
|
|
||
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.
buildRerunTasksis a test job, but it isn't inpublish'sneeds. While it is the only test job still running, this sentence (and the same one at AGENTS.md:258) doesn't hold. Its result is never checked either, so the core is published even if it fails.The window shows up in real runs. In 8.0.x run 39644767954:
publishstarted 236.7 minutes in, right after the lastBuild Grails-Coreleg finished.Build Grails-Core Rerunning all Taskskept running until 260.3.A cancel during those 24 minutes publishes the whole chain, and a failure there wouldn't have stopped it either. On 8.0.x,
casSecurityConfigis missing frompublish'sneedsin the same way: it was added in 155a845, after ee2b481 made the test jobs gate the publish.I'd add
buildRerunTaskstopublish'sneedshere, with the samesuccess || skippedcheck, and addcasSecurityConfigin the merge-up. In 39644767954 that would have delayed the core snapshot by those 24 minutes. If that's too slow, narrow the wording to "a cancelled test job that publish needs" instead.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.
Good catch, thanks. I added
buildRerunTasksto theneedsand the checks ofpublishin dbf938c. I'd rather take the 24 minutes than publish a core whose--rerun-tasksbuild failed: it is the only core test run whose results can't come from the build cache.The same commit adds a line to the publish comment and a rule to
AGENTS.md: every test job goes into theneedsofpublish, and every job that builds or validates without tests goes into those ofpublishGradle. That should catch the nextcasSecurityConfig.