ym0506 commented on PR #38352:
URL: https://github.com/apache/shardingsphere/pull/38352#issuecomment-4010034035
> > Thanks. I updated the checkout ref selection as suggested.
> > Current behavior in all 5 `actions/checkout` steps is now: `ref: ${{
github.event_name == 'pull_request' && github.ref || github.sha }}`
> > So:
> >
> > * `pull_request` keeps using the PR merge ref
> > * non-PR runs stay SHA-pinned and preserve the previous deterministic
checkout behavior
> >
> > The patch is pushed in commit `2df1dcf9`.
>
> Hi @ym0506 Thanks for the contribution! However, I’ve analyzed the root
cause and believe this specific change might not fully resolve the `not our
ref` error. Here is a breakdown:
>
> ### 1. Analysis
> * **`github.ref`**: In a `pull_request` event, this defaults to
`refs/pull/XXX/merge`. This is a "synthetic" commit created by GitHub to
simulate the merge result.
>
> ### 2. Why the error persists
> The error `fatal: remote error: upload-pack: not our ref` occurs because
these synthetic merge commits are volatile. If a Matrix job takes time to
initialize or if the PR is updated/synchronized during the run, GitHub may
garbage-collect the old merge ref, causing late-starting jobs to fail when they
try to fetch it. Since `actions/checkout` already uses `github.ref` by default,
explicitly declaring it doesn't prevent the ref from disappearing.
>
> ### Summary
> To ensure stability across Matrix jobs, we should consider pinning the
checkout to the actual head of the PR branch instead of the unstable merge ref:
`ref: ${{ github.event.pull_request.head.sha || github.sha }}`
>
> This ensures that all jobs in the matrix point to a persistent commit that
won't be deleted during the workflow execution.
>
> But this is not preferred solution.
>
> Could you help to investigate `Solution 2. Take source code snapshot and
reuse it in matrix jobs (preferred) ` in [#38298
(comment)](https://github.com/apache/shardingsphere/issues/38298#issuecomment-3983951285)
?
Thanks for the clarification. That makes sense.
I agree that the current change only addresses the determinism concern for
non-PR events, and does not fundamentally eliminate the volatility of
`refs/pull/<number>/merge` for matrix jobs.
I also agree that switching to `github.event.pull_request.head.sha` would
likely improve stability, but it is probably not the preferred final direction
because it weakens merged-with-base validation.
I’ll look into Solution 2 next:
- create a source snapshot in `prepare-e2e-artifacts`
- reuse that snapshot in the E2E-SQL matrix jobs instead of running
`actions/checkout` again in each job
The current workflow already reuses Maven and Docker artifacts, so I’ll
check whether a source snapshot can be integrated cleanly into the same flow.
If it looks feasible, I’ll update this PR in that direction. If not, I’ll
summarize the constraints and tradeoffs here before proposing a different
approach.
--
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]
