Checking out wrong commit?

[jwilner]

I just introduced a failing test into the main branch of a project; the failing tests (introduced in the commit triggering the build) appeared not to have run in the PR build.

When I look at the actions/checkout output, I am surprised by what I see.

Specifically, the build is running on commit 2dca06e -- however, actions/checkout appears to be merging the prior commit -- c0121d5 -- (without the tests) into the project's main branch.

Is this expected?

The project's checkout configuration is completely vanilla: https://github.com/golangci/golangci-lint/blob/master/.github/workflows/pr.yml#L46

[jwilner]

Local discussion of issue: golangci/golangci-lint#1229

[AustinShalit]

This is similar to #237

[kenorb]

Same happens when using with ad-m/github-push-action, related:

• Extra commit when commiting ad-m/github-push-action#20
• Push issue ad-m/github-push-action#44

I've described more details at #237 (comment)

[ericsciple]

@andymckay fyi

[medikoo]

This looks quite serious (allows to sneak not validated code as valid)

@ericsciple is there any specific timeline for this?

[tjanez]

Also happened for oasisprotocol/oasis-core-ledger#16:
https://github.com/oasisprotocol/oasis-core-ledger/pull/16/checks?check_run_id=902853072

[tjanez]

I was able to use the following work-around:

[... trimmed ...]
      - name: Checkout code
        uses: actions/checkout@v2
        with:
          # Check out pull request's HEAD commit instead of the merge commit to
          # work-around an issue where wrong a commit is being checked out.
          # For more details, see:
          # https://github.com/actions/checkout/issues/299.
          ref: ${{ github.event.pull_request.head.sha }}
[... trimmed ...]

[ruz]

This affects us, we found this documentation: https://developer.github.com/v3/pulls/#response-1. I searched code in this repository for mergeable and merge_commit_sha, but couldn't find anything. Feels like it's related.

[ssantichaivekin]

I'm having the same problem in this pull request run: https://github.com/ssantichaivekin/empress/runs/900765234

It seems that this can be fixed by making a commit that is up to date with the current origin/master. See ssantichaivekin/empress#184

[lethosor]

We just ran into what I think is another instance of this issue in https://github.com/DFHack/scripts/runs/910104637?check_suite_focus=true#step:4:375
This check ran on DFHack/scripts@5177a12, but ended up running on a merge between master and the previous commit on that branch (DFHack/scripts@1ec3d9b, as seen in the actions/checkout output), which produced a lot of irrelevant errors that the newer commit was intended to address. This issue repeated itself when we re-ran the check, so it appears to be reproducible in this case, but I'm not seeing an obvious reason why - the newer commit in that PR was pushed well after the checks run on the older commit failed, and we haven't seen this in other PRs. Update: we've seen this in at least two PRs so far.

About 2 minutes after re-running the check reproduced the issue, I ran this locally, which resulted in fetching a correct merge, so I'm pretty sure the issue is on the action's end:

$ git fetch origin refs/pull/159/merge
$ git show FETCH_HEAD 
commit 5be19d326c828ac38ce307eb6db068e691504a7e
Merge: ad67c79 5177a12

[CaptainGlac1er]

Seeing this issue as well with the v2 of checkout.
Timeline

• Committed and push code to a branch that failed a test *hashA
• Committed code that fixed the test however didn't push *hashB
• Merged in main then pushed *hashC

The commit *hashC still checked out *hashA code to run tests on.

[alalazo]

+1 I had the same issue with this run and noticed because the commit was introducing a modification to the workflow YAML file that was not honored. A few of our contributors reported the same too, for instance in this PR spack/spack#17755

If it matters, in my case the issue was with a force pushed commit on a PR where tests on previous commits were still running.

[ericsciple]

@hashtagchris should i set you as the assignee?

[jcornaz]

@ericsciple which issue should we follow this one (#299) or #237?

[andymckay]

I believe this is a dupe of #237, please see my comment here: #237 (comment)

[dkarlovi]

This seems to happen for me still, my action is checking out the wrong branch, static analysis is complaining about code which it's not supposed to be testing. 🤔

[wchargin]

I had the same problem in the pull-request.

Fix with:

- uses: actions/checkout@v2
  with:
    ref: ${{ github.event.pull_request.head.sha }}

Beware: this checks out the current head of the branch, whereas by
default the action checks out the merge commit. By using this
workaround, you will no longer catch conflicting changes in the base
branch.

Is there a way to check out the correct merge commit without race
conditions? There’s the merge_commit_sha field, but the docs
indicate that that may be null if mergeability has not yet been
computed.

[tonyhallett]

@wchargin
Was you running on pull_request_target ? The ref and sha is different to pull_request

You can wait for the merge_commit_sha

- name: wait for mergeable
        id: wait-for-mergeable
        uses: tonyhallett/ReleaseManager/wait_for_mergeable@main
        env:
            GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
      - name: checkout
        uses: actions/checkout@v2
        with:
            ref: ${{ steps.wait-for-mergeable.outputs.mergeCommitSha}}

I have not released my actions from ReleaseManager yet so here is the typescript code.

import * as core from '@actions/core'
import * as github from '@actions/github'
import {PullRequest} from '@octokit/webhooks-definitions/schema'
import {getProcessEnv} from './helpers/getProcessEnv'
import {getNumberInput} from './helpers/inputHelpers'

async function run(): Promise<void> {
  try {
    const initialWait = getNumberInput('initialWait')
    const waitTime = getNumberInput('wait')
    const maxWait = getNumberInput('timeout')

    const pullRequest = github.context.payload.pull_request as PullRequest
    const pullNumber = pullRequest.number

    const githubToken = getProcessEnv('GITHUB_TOKEN')
    if (!githubToken) {
      core.error('no token')
    } else {
      let timedOut = false
      let mergeable: boolean | null = null
      let mergeableState: string | undefined
      let mergeCommitSha: string | null = null
      const octokit = github.getOctokit(githubToken)

      await wait(initialWait)
      const start = new Date().getTime()
      // eslint-disable-next-line no-constant-condition
      while (true) {
        const response = await octokit.pulls.get({
          ...github.context.repo,
          pull_number: pullNumber
        })
        mergeable = response.data.mergeable
        if (mergeable != null) {
          mergeCommitSha = response.data.merge_commit_sha
          mergeableState = response.data.mergeable_state
          break
        }
        await wait(waitTime)
        const now = new Date().getTime()
        if (now - start > maxWait) {
          timedOut = true
          break
        }
      }

      core.setOutput('timedOut', timedOut)
      core.setOutput('mergeable', mergeable)
      core.setOutput('mergeableState', mergeableState)
      core.setOutput('mergeCommitSha', mergeCommitSha)
    }
  } catch (e) {
    core.error(e)
  }
}

const wait = async (interval: number): Promise<void> => {
  return new Promise<void>(resolve => {
    setTimeout(() => {
      resolve()
    }, interval)
  })
}

run()

[wchargin]

@tonyhallett: This is also incorrect, because it fetches the most
recent merge commit rather than the merge commit for the commit being
tested. Consider the following sequence:

1. Push commit A to PR 123.
2. CI queued on commit A. Merge commit not yet computed.
3. Push commit B to PR 123 (same PR).
4. GitHub computes the new merge commit to merge B into master.
5. CI run for commit A unqueues and starts running.

If at point (5) you fetch the current merge commit for PR 123, you
will be running on the code in commit B, not commit A. This would mean
that when you click the CI status indicator on commit A, it actually
shows you tests that ran at a different commit, which is obviously
unacceptable.

[ethomson]

Closing, this is a dup of #237 and was resolved there. If you believe that you're seeing any problems here, please contact support.