[
https://issues.apache.org/jira/browse/IGNITE-28827?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Anton Vinogradov updated IGNITE-28827:
--------------------------------------
Description:
h3. Problem
The GitHub Actions checks in .github/workflows/commit-check.yml (Code Style,
Abandoned Tests,
Javadocs, .NET, ducktape) are not dispatched for a pull request that conflicts
with the base
branch. The status checks never appear, so a contributor whose PR is
temporarily in conflict
gets no CI feedback until the conflict is resolved.
h3. Root cause
These checks are triggered by the \{{pull_request}} event, which GitHub
dispatches against the
*test-merge commit* (PR head merged into base). When the PR conflicts
(mergeable state DIRTY),
GitHub cannot compute that merge commit, so no run is created. This is
independent of what the
jobs check out — they already check out \{{pull_request.head.sha}}.
For comparison, the "Rolling Upgrade / Protected Classes" check runs fine on
conflicting PRs
because it is triggered by \{{pull_request_target}}, which runs in the
base-branch context and
does not depend on the test-merge commit.
h3. Proposed change
Switch commit-check.yml from \{{pull_request}} to \{{pull_request_target}}.
That event is the only
PR-triggered event not gated on the merge commit, so it is the only way to
dispatch these checks
on a conflicting PR. All jobs already check out \{{pull_request.head.sha}}.
h3. Security trade-off
These jobs build and *run* untrusted PR code (mvnw test, dotnet build, tox).
Running untrusted
code under pull_request_target is sensitive, so the workflow is hardened to
grant no more
privilege than the \{{pull_request}} event already did:
* the token is downscoped to \{{permissions: contents: read}} (no write);
* no \{{secrets.*}} are referenced;
* the workflow definition is taken from the base branch, so a PR cannot modify
it.
A code comment documents this invariant so that secrets / write permissions are
not added later
without revisiting the trade-off.
h3. Notes / follow-ups
* pull_request_target that builds and runs fork code goes against GitHub's
default guidance for
untrusted code; needs confirmation that ASF Infra policy permits it.
* Required checks / branch protection are unchanged (job names stay the same).
was:
h3. Problem
The "Code Style" / checkstyle GitHub Actions check is not dispatched for a pull
request that
has merge conflicts with the base branch. The status check simply never
appears, so a
contributor whose PR is temporarily in conflict gets no code-style feedback
until the conflict
is resolved.
h3. Root cause
The check lives in .github/workflows/commit-check.yml and is triggered by the
\{{pull_request}}
event. For \{{pull_request}}, GitHub dispatches the run against the test-merge
commit (the result
of merging the PR head into the base branch). When the PR conflicts with the
base branch
(mergeable state DIRTY), GitHub cannot compute that merge commit, so no
workflow run is created
at all. This is independent of what the job checks out (the job already checks
out
{\{pull_request.head.sha}}).
For comparison, the "Rolling Upgrade / Protected Classes" check runs fine on
conflicting PRs
because it is triggered by \{{pull_request_target}}, which executes in the
base-branch context and
does not depend on the test-merge commit.
Checkstyle does not conceptually need a clean merge — it only needs the PR
sources. The
limitation is inherited purely from the \{{pull_request}} event semantics.
h3. Proposed solution
Move the code-style + licenses checks into a dedicated workflow triggered by
{\{pull_request_target}} (plus \{{push}} for master/release branches, to keep
coverage on pushes),
so the check is dispatched regardless of merge conflicts.
To keep this as safe as the current \{{pull_request}} event, the new workflow:
* downscopes the token to \{{permissions: contents: read}};
* references no repository secrets;
* still checks out \{{pull_request.head.sha}} and only *compiles* the PR sources
(\{{mvnw test-compile ...}}) — it does not execute tests.
With write permissions and secrets removed, building untrusted fork code grants
no privilege
beyond what the existing \{{pull_request}} job already allows, so no new attack
surface is
introduced. The heavy steps that actually execute PR code (abandoned-tests
check) stay on the
{\{pull_request}} event and are intentionally left untouched.
h3. Notes / follow-ups
* The status check name changes from "Check java code on JDK 17" to
"Check java code style on JDK 17"; branch-protection required-checks must be
updated by Infra.
* \{{pull_request_target}} that builds fork code is sensitive; worth confirming
with ASF Infra.
> Run all PR checks even when the PR conflicts with the base branch
> -----------------------------------------------------------------
>
> Key: IGNITE-28827
> URL: https://issues.apache.org/jira/browse/IGNITE-28827
> Project: Ignite
> Issue Type: Task
> Reporter: Anton Vinogradov
> Assignee: Anton Vinogradov
> Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> h3. Problem
> The GitHub Actions checks in .github/workflows/commit-check.yml (Code Style,
> Abandoned Tests,
> Javadocs, .NET, ducktape) are not dispatched for a pull request that
> conflicts with the base
> branch. The status checks never appear, so a contributor whose PR is
> temporarily in conflict
> gets no CI feedback until the conflict is resolved.
> h3. Root cause
> These checks are triggered by the \{{pull_request}} event, which GitHub
> dispatches against the
> *test-merge commit* (PR head merged into base). When the PR conflicts
> (mergeable state DIRTY),
> GitHub cannot compute that merge commit, so no run is created. This is
> independent of what the
> jobs check out — they already check out \{{pull_request.head.sha}}.
> For comparison, the "Rolling Upgrade / Protected Classes" check runs fine on
> conflicting PRs
> because it is triggered by \{{pull_request_target}}, which runs in the
> base-branch context and
> does not depend on the test-merge commit.
> h3. Proposed change
> Switch commit-check.yml from \{{pull_request}} to \{{pull_request_target}}.
> That event is the only
> PR-triggered event not gated on the merge commit, so it is the only way to
> dispatch these checks
> on a conflicting PR. All jobs already check out \{{pull_request.head.sha}}.
> h3. Security trade-off
> These jobs build and *run* untrusted PR code (mvnw test, dotnet build, tox).
> Running untrusted
> code under pull_request_target is sensitive, so the workflow is hardened to
> grant no more
> privilege than the \{{pull_request}} event already did:
> * the token is downscoped to \{{permissions: contents: read}} (no write);
> * no \{{secrets.*}} are referenced;
> * the workflow definition is taken from the base branch, so a PR cannot
> modify it.
> A code comment documents this invariant so that secrets / write permissions
> are not added later
> without revisiting the trade-off.
> h3. Notes / follow-ups
> * pull_request_target that builds and runs fork code goes against GitHub's
> default guidance for
> untrusted code; needs confirmation that ASF Infra policy permits it.
> * Required checks / branch protection are unchanged (job names stay the same).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
