borinquenkid commented on PR #16673:
URL: https://github.com/apache/grails-core/pull/16673#issuecomment-6277276534
I read the full diff and the resulting `gradle.yml` /
`groovy-joint-workflow.yml`. The approach looks right to me, and replacing the
`head_commit.message` test fixes a real bug (that field is empty on
`pull_request` events). `publish` already accepts `skipped` for every test job,
so the build-only path still publishes snapshots on a push. The commit message
only reaches the script through `env`, which is the right call. A few
suggestions to make it less fragile:
1. **`policy` is a single point of failure.** `gh api .../commits/$HEAD_SHA`
has no retry. A transient 502 fails `policy`, and then every job that `needs`
it is skipped, including `validateDependencies` and `buildGradle`, which don't
need the decision at all. Skipped reads as green for a required check. Suggest
a few retries around the API call, and falling back to `run-tests=true` (the
safe default) if it still fails, so an API blip costs runner time rather than
silently dropping tests.
2. **`workflow_dispatch` on a non-default branch runs no tests** unless the
head commit has `[run tests]`. Someone dispatching a run by hand usually wants
the tests. Consider a `run-tests` dispatch input, or treating a dispatch as
`true` unless the commit says `[skip tests]`.
3. **The label/draft `if` is copied about 15 times.** It is easy to forget
on the next job, and AGENTS.md rule 14 is the only guard. A
`validateRepositoryConventions` check ("every job of a workflow that triggers
on `pull_request` has this `if` or `needs: policy`") would make it mechanical.
4. **Labels are read when the run starts**, so adding `run ci` to a draft
does nothing until the next push. That is documented, but it will surprise
contributors. Is it worth keeping `labeled` out only for `run ci` / `no ci`
(i.e. a job-level filter on `github.event.label.name`) rather than for every
label?
5. **Concurrency:** a run where every job is skipped (a draft without `run
ci`) still enters the `github.workflow-github.ref` group with
`cancel-in-progress` on. Does pushing to a draft cancel an in-flight run on the
same ref that was started before the PR was converted to draft? If so, that may
be wanted, but it is worth a line in the docs.
6. **Labels must exist.** `no ci` / `run ci` need creating on merge (it is
in your follow-ups); until then `run ci` cannot be applied to a draft.
Context on why I care: we have a long stack of draft Java-to-Groovy PRs into
side branches, and under this policy they would be build-only unless a head
commit starts with `[run tests]`, which is what we want. Thanks for putting
numbers on the usage.
--
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]
