codeconsole commented on code in PR #16726:
URL: https://github.com/apache/grails-core/pull/16726#discussion_r4349501293
##########
.github/workflows/gradle.yml:
##########
@@ -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.
Review Comment:
`buildRerunTasks` is a test job, but it isn't in `publish`'s `needs`. 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:
- `publish` started 236.7 minutes in, right after the last `Build
Grails-Core` leg finished.
- `Build Grails-Core Rerunning all Tasks` kept 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, `casSecurityConfig` is missing
from `publish`'s `needs` in the same way: it was added in 155a845537, after
ee2b4817da made the test jobs gate the publish.
I'd add `buildRerunTasks` to `publish`'s `needs` here, with the same
`success || skipped` check, and add `casSecurityConfig` in 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.
##########
.github/workflows/gradle.yml:
##########
@@ -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 ]
Review Comment:
This merges into 8.0.x and 9.0.x without conflicts, but both branches also
have `validateBomProperties`, `buildLogic` and `validateGithubBranch`, and none
of them is in these `needs`. In 39644767954 all three finished well before
`buildForge` (at 7.3, 15.3 and 7.0 minutes, against 55.9), so the cancel
behaviour still holds there. The problems are elsewhere:
- A commit whose BOM property validation failed would still be published.
- On those branches, "every build job (buildGradle, buildForge and
validateDependencies)" in the comment at the top would no longer be true.
It's worth adding the three jobs to `needs` and to the check in the merge-up
commit.
--
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]
