codeconsole commented on code in PR #16673:
URL: https://github.com/apache/grails-core/pull/16673#discussion_r4332495023
##########
RELEASE.md:
##########
@@ -27,6 +27,14 @@ Prior to starting the release process, ensure that any other
dependent library i
Due to a limitation with GitHub, private groups cannot be used as approvers
for an environment. For this reason, prior to performing the release, add
GitHub username to asf.yaml in the environment section for approvers. Only 6
approvers may exist on a given environment.
+A push to a release branch below the default branch builds and publishes
without running the tests (see the CI policy in `AGENTS.md`), so the commit a
release is cut from may never have been tested as a whole on that branch.
Before creating the release, run the `CI` workflow manually on the release
branch with its `run-tests` input ticked and wait for it to pass:
Review Comment:
`gradle.yml` now cancels the run in progress for every event, and a dispatch
shares its concurrency group (`CI-refs/heads/7.0.x`) with pushes to that
branch. So a merge to 7.0.x while this run is going cancels its test jobs, and
the push's own run skips the tests. It's worth a sentence here: if the run ends
up cancelled, run it again once nothing else is being merged to the branch.
##########
.github/workflows/gradle.yml:
##########
@@ -19,14 +19,37 @@ on:
branches:
- '[0-9]+.[0-9]+.x'
pull_request:
+ # ready_for_review is not a default type: a draft runs nothing until it is
ready or labelled "run ci".
+ types: [ opened, synchronize, reopened, ready_for_review ]
workflow_dispatch:
-# Queue jobs - cancel in-progress PR runs when new commits pushed, but allow
branch builds to complete
+ inputs:
+ run-tests:
+ description: "Run the test jobs where the branch rule would skip them
(a release branch below the default); a [skip tests] head commit still skips
them"
+ type: boolean
+ default: false
+# A new commit on the same pull request or branch cancels the build and test
jobs of the run in flight: their
+# 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
Review Comment:
This holds when the tests ran, but not on a build-only run, and a build-only
run is now what every push to 7.x gets.
On a build-only run the test jobs are `skipped`, not `cancelled`, and
`skipped` passes `publish`'s checks. So once `buildGradle` has succeeded, a
cancel doesn't stop the publish chain:
- `publishGradle` and `publish` are `always()` jobs whose needs all
succeeded or were skipped, so they run in full after the cancel. `docs` follows
`publish`.
- `buildForge` is still running, so it's cancelled, and `publishForge` and
`verifyWrapper` are skipped.
In 39304647851, the last build-only push to 7.0.x, `buildGradle` finished 2
minutes in, `publish` started at 5 and took 15 minutes, and `buildForge` ran
until minute 12. So when a second merge lands on 7.0.x more than two minutes
after the first:
- The superseded commit's plugins, core and docs are published anyway, and
the replacing run waits behind them.
- The snapshot repository holds that commit's core with the previous
commit's Forge and CLI until the replacing run's `publishForge`. Before this
change the first run finished, so its Forge was published too.
Two ways to fix it:
- Add `buildForge` and `validateDependencies` to the `needs` of
`publishGradle` and `publish`, with the same explicit `success` checks. A
cancel before the builds finish then publishes nothing of that commit, whether
or not the tests ran. A build-only publish starts about 10 minutes later, and a
commit whose Forge build or dependency validation failed is no longer
published. A tested run publishes at the same time as now, since `publish`
already waits for tests that take longer than either build.
- Or keep the graph and correct the text: on a build-only run, a cancel
after the plugin build still publishes the commit, without its Forge. The same
claim is in `publish`'s comment (line 590) and in the CI policy section of
AGENTS.md (line 258).
##########
CONTRIBUTING.md:
##########
@@ -272,6 +272,25 @@ Grails makes use of [Release
Drafter](https://github.com/release-drafter/release
commit messages are important. They should follow the project's rules. While a
change can be incrementally made under
many commits, pull requests should be squashed into a single, meaningful
commit message.
+### Continuous Integration
+
+Every push and pull request runs the GitHub Actions workflows, and runner time
is shared with every other Apache
+project, so two controls limit what runs:
+
+* A draft pull request runs no workflow unless it carries the `run ci` label;
marking it ready for review starts CI.
+ A pull request labelled `no ci` runs no workflow at all, draft or not. (The
Release Drafter still labels every
+ pull request into a release branch; it runs no build.) Labels are read when
a push arrives, so add `run ci` (or
+ remove `no ci`) and then push, or mark the draft ready, to get a run.
Pushing to a draft still cancels the run in
+ flight for it, as every push to a pull request or a branch does.
Review Comment:
"as every push to a pull request or a branch does": on a branch, only CI and
the Groovy joint build cancel the run in flight. Code Style, Build Health,
CodeQL and RAT still cancel only on pull requests. AGENTS.md says "through the
concurrency groups of the test workflows", and the same qualifier would work
here.
##########
AGENTS.md:
##########
@@ -246,6 +247,17 @@ Rules:
- Do not use a newer third-party SHA, tag, or major version until it is on the
allowlist. If you need a new pin, open a PR against
`apache/infrastructure-actions` (`actions.yml`, not the generated
`approved_patterns.yml`).
- Before adding or bumping a third-party `uses:` line, search
`approved_patterns.yml` for that action and copy an approved SHA.
+### CI policy
+
+GitHub-hosted runner time is shared with every other Apache project, so the
workflows run tests only where the result is worth the runner time:
+
+- A draft pull request runs nothing unless it carries the `run ci` label, and
a pull request labelled `no ci` runs nothing at all: every workflow that reacts
to a pull request skips all of its jobs, except the Release Drafter
(`release-notes.yml`), which keeps reacting to every pull request into a
release branch, draft or not, because its autolabeler must label drafts; it
runs no build. Marking a draft ready for review starts CI (the workflows list
the `ready_for_review` trigger type, which is not a default). Labels and the
draft flag are read when a run starts, so set them before pushing; adding `run
ci` or removing `no ci` takes effect on the next push, or when the draft is
marked ready, and adding `no ci` does not stop a run in flight. A push to a
draft, or to a `no ci` pull request, still cancels the run in flight for that
pull request, as every push to a pull request or a branch does through the
concurrency groups of the test workflows; the run that replaces it does
nothing. A
new workflow with a `pull_request` trigger lists the same `types` as its
siblings and gates its jobs the same way.
+- Every workflow that runs tests calls `.github/workflows/ci-policy.yml` as
its first job, `policy`, and gates its test jobs (and the test steps of its
build jobs) on `needs.policy.outputs.run-tests`. Never test
`github.event.head_commit.message` in a workflow: a `pull_request` payload has
no such field, so that test silently ran the tests on every pull request. The
policy reads the head commit of the push, of the pull request, or of the
dispatched branch, and matches the marker against the start of the message's
first line. The read is retried with a growing pause; when it still fails, the
tests run, as they did before the policy existed, and a warning annotation and
the step summary say so, so an API outage costs runner time rather than a test
result.
+- The tests run by default only on the default branch and the release branches
above it, and there a commit message starting with `[skip tests]` skips them.
Everywhere else, that is on a release branch below the default (7.0.x, 7.1.x
and 7.2.x while 8.0.x is the default) and on a pull request whose base is a
side branch rather than a release branch, the tests run only on request: when
the commit message starts with `[run tests]`, when the pull request carries the
`run ci` label, or when a manual run (`workflow_dispatch`) in
`apache/grails-core` has its `run-tests` input ticked. Otherwise the run
builds, and on a push publishes snapshots, without tests, exactly like a `[skip
tests]` commit; a `[skip tests]` commit also wins over the label and the input.
The label is the way to test a backport pull request without rewriting its head
commit, and the input is how the commit a release is cut from gets tested (see
RELEASE.md); both are read when the run starts. A marker anywhere but at th
e start of the first line is ignored. The rule follows the default branch:
when a new one is promoted, the branches below it stop running tests by default
without an edit. The older release branches are kept quiet on purpose: a
backport lands as one push to the oldest branch and a merge-up to each branch
above it, and its tests belong on the pull request, labelled `run ci`, not on
every push after it. Side branches are unreviewed development work (see the
review process in CONTRIBUTING.md); their tests run when the branch is proposed
to a release branch. In a fork, where the runner time is the fork owner's and
not the ASF's, a manual run runs the tests unless the commit message starts
with `[skip tests]`.
+- The jobs that run no tests (dependency validation, the build-without-tests
steps, code style, build health, and the snapshot publish) still run on every
push to a release branch and on every non-draft pull request; RAT and CodeQL
run on every push to a release branch and on every non-draft pull request into
one, since their `pull_request` triggers list only release-branch bases. Only
the `no ci` label, or a draft without `run ci`, stops them.
Review Comment:
The snapshot publish doesn't run on pull requests: `publishGradle`,
`publish`, `publishForge` and `verifyWrapper` need a `push` or
`workflow_dispatch`, and `docs` needs a `push`. So "and on every non-draft pull
request" is true of everything in the list except it.
##########
.github/workflows/ci-policy.yml:
##########
@@ -0,0 +1,142 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Decides, once per run of a calling workflow, whether that workflow's test
jobs run. Every workflow that
+# runs tests calls this workflow as its first job, `policy`, and gates its
test jobs on the run-tests
+# output, so the rules live here and nowhere else; see the "CI policy" section
of AGENTS.md for the
+# reasons behind them.
+#
+# The tests run when the first line of the commit message starts with [run
tests] and do not run when it
+# starts with [skip tests]; a marker anywhere else is ignored. Without a
marker, an explicit request runs
+# them: the "run ci" label on a pull request, or the run-tests input of a
manual run. Without either, the
+# target branch decides: the default branch and every release branch above it
run the tests; a release
+# branch below the default (7.0.x while 8.0.x is the default) and a branch
that is not a release branch
+# (the base of a feature-to-feature pull request) build and publish without
them. The commit is the head
+# commit of the push, of the pull request, or of the dispatched branch. Two
exceptions: a manual run in a
+# fork, whose runner time is the fork owner's and not the ASF's, runs the
tests unless the message says
+# [skip tests]; and when the commit cannot be read from the API after several
attempts, the tests run,
+# as they did before this policy existed, so an API outage costs runner time
rather than a test result.
+#
+# The "no ci" label and the draft rule (a draft runs nothing without the "run
ci" label) are not handled
+# here: the calling job carries them in its `if`, so such a pull request runs
no job at all, not even this one.
+# When the tests do not run, a notice annotation on the run says so, so an
all-green pull request shows it.
+name: "CI policy"
+on:
+ workflow_call:
+ outputs:
+ run-tests:
+ description: "'true' when the calling workflow runs its test jobs,
'false' when it runs only its build and publish jobs"
+ value: ${{ jobs.decide.outputs.run-tests }}
+permissions:
+ contents: read
+jobs:
+ decide:
+ name: "Decide whether the tests run"
+ runs-on: ubuntu-24.04
+ outputs:
+ run-tests: ${{ steps.decide.outputs.run-tests }}
+ steps:
+ - name: "⚖️ Apply the CI policy"
+ id: decide
+ # The commit message is untrusted input (a pull request may come from
any fork), so it reaches the
+ # script only through the environment, never through expression
interpolation.
+ env:
+ GH_TOKEN: ${{ github.token }}
+ EVENT_NAME: ${{ github.event_name }}
+ DEFAULT_BRANCH: ${{ github.event.repository.default_branch }}
+ TARGET_BRANCH: ${{ github.base_ref || github.ref_name }}
+ HEAD_SHA: ${{ github.event.pull_request.head.sha || github.sha }}
+ PUSH_MESSAGE: ${{ github.event.head_commit.message }}
+ REPOSITORY_OWNER: ${{ github.repository_owner }}
+ RUN_CI_LABEL: ${{ contains(github.event.pull_request.labels.*.name,
'run ci') }}
+ DISPATCH_RUN_TESTS: ${{ github.event.inputs.run-tests }}
+ run: |
+ set -euo pipefail
+ message_read=true
+ if [[ "$EVENT_NAME" == push ]]; then
+ message="$PUSH_MESSAGE"
+ else
+ # A pull_request or workflow_dispatch payload carries no commit
message, so read the head commit,
+ # retrying with a growing pause: a transient API error must not
decide the run.
+ message_read=false
+ for pause in 2 4 8 16 0; do
+ if message="$(gh api
"repos/${GITHUB_REPOSITORY}/commits/${HEAD_SHA}" --jq '.commit.message')"; then
+ message_read=true
+ break
+ fi
+ if (( pause > 0 )); then
+ echo "Reading commit ${HEAD_SHA} failed; retrying in ${pause}s"
+ sleep "$pause"
+ fi
+ done
+ fi
+ if [[ "$message_read" == true ]]; then
+ subject="${message%%$'\n'*}"
+ subject="${subject#"${subject%%[![:space:]]*}"}"
+ else
+ subject="(the commit message could not be read)"
+ fi
+
+ run_tests=true
+ if [[ "$message_read" != true ]]; then
+ reason="the commit message could not be read from the GitHub API
after 5 attempts, and then the tests run"
+ echo "::warning title=CI policy::Could not read commit ${HEAD_SHA}
from the GitHub API after 5 attempts; the tests run."
+ elif [[ "$subject" == '[skip tests]'* ]]; then
Review Comment:
Before this change a pull request always ran the tests, because
`head_commit.message` is empty on `pull_request`. Now a `[skip tests]` head
commit skips them on a pull request into 8.0.x and above too, and on a fork
pull request the author picks that commit. The skipped test checks count as
passing in the merge box.
The push after the merge doesn't always catch it either. The repository's
squash title setting is `COMMIT_OR_PR_TITLE`, so squashing a single-commit pull
request gives the merge commit the same `[skip tests]` title.
The notice annotation helps, but it only shows on the run page. Could `[skip
tests]` on a `pull_request` event count only when
`github.event.pull_request.author_association` is `OWNER`, `MEMBER` or
`COLLABORATOR`? Committers would keep the marker for docs-only and CI-only pull
requests, and a contributor's pull request into a tested branch would always
run the tests. Not blocking.
--
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]
