Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misdetection of latest HEAD in PRs on Github Action CI #525

Closed
Flamefire opened this issue Nov 30, 2021 · 16 comments
Closed

Misdetection of latest HEAD in PRs on Github Action CI #525

Flamefire opened this issue Nov 30, 2021 · 16 comments
Labels
bug Something isn't working

Comments

@Flamefire
Copy link
Contributor

Describe the bug
I have a warning in the codecov comment on Github after running CI on Github Actions:

Current head <SHA-merge> differs from pull request most recent head <SHA-branch>. Consider uploading reports for the commit <SHA-branch> to get more accurate results

The mentioned SHAsums are those of the merge-commit-to-be and the latest commit on the branch.
I'm not sure if that is a bug in the uploader or the reporting later on so pass this on to the correct team if wrong.

To Reproduce
Steps to reproduce the behavior:

  1. Go to Improve coverage docs on GHA boostorg/boost-ci#114 (comment)
  2. See error

Expected behavior
No warning. It makes sense to that coverage for the merge-commit is uploaded, not that from the branch.

Additional context
The uploader is used "vanilla", i.e.

    curl -Os https://uploader.codecov.io/latest/linux/codecov
    chmod +x codecov
    ./codecov -f coverage.info

However I see that on Github actions the commit mentioned in the upload URL (log output) is the merge-SHA while on Drone CI it is the branch-SHA. So I'd say either one is wrong?

@Flamefire
Copy link
Contributor Author

Some more info: I see you use GITHUB_REF to extract the PR number at

const matches = prRegex.exec(envs.GITHUB_REF || '')

This shouldn't be done, and github.event.number property/env var should be used: https://github.community/t/github-ref-is-inconsistent/17728

@drazisil-codecov
Copy link
Contributor

Thanks for the report and research , @Flamefire

We'll see what we can do to resolve this. As a workaround, you should be able to pass the correct value to override_pr as an action key, or -P as a uploader flag.

PRs welcome! :)

@drazisil-codecov drazisil-codecov added the bug Something isn't working label Nov 30, 2021
@Flamefire
Copy link
Contributor Author

I'm not sure how to fix this as I don't know the correct value for the commit param.

Here he URL used for Drone:
https://codecov.io/upload/v4?package=uploader-0.1.13&token=*******&branch=master&build=17&build_url=&commit=20f52fe0905ca896125cf3be77a3af573559411d&job=&pr=114&service=drone.io&slug=boostorg%2Fboost-ci&name=&tag=&flags=&parent=
Here for GHA:
https://codecov.io/upload/v4?package=uploader-0.1.13&token=*******&branch=coverage-doc&build=1522657794&build_url=https%3A%2F%2Fgithub.com%2Fboostorg%2Fboost-ci%2Factions%2Fruns%2F1522657794&commit=97002113c2119eed98a8411df5955bda54d548c8&job=CI&pr=114&service=github-actions&slug=boostorg%2Fboost-ci&name=&tag=&flags=&parent=

As you can see they are fundamentally different. In both cases the PR is detected correctly, so the above mentioned thing is just incidental as it "may" cause issues too.

So basically it boils down to: What are the expected values for "branch" and "commit"?

@drazisil-codecov
Copy link
Contributor

ah, I misunderstood. In this case, I believe you want to only upload coverage. for pushes, not pull_request.

boostorg/boost-ci@20f52fe sats it's not part of any branch, which usually means it's either been squashed or otherwise deleted. This can cause issues, but it looks like it's showing correctly on Codecovm so it might be a caching issue with updating the PR. How long ago did it complete?

https://app.codecov.io/gh/boostorg/boost-ci/compare/114/commits

@Flamefire
Copy link
Contributor Author

Flamefire commented Dec 1, 2021

I believe you want to only upload coverage. for pushes, not pull_request.

Erm, no? Of course I want to have coverage info for PRs as that is part of CI testing: That a change does have decent coverage which is a condition for the merge.

boostorg/boost-ci@20f52fe sats it's not part of any branch

Just check the link above or the whole PR: boostorg/boost-ci#114
The commit 20f52fe is the commit HEAD of the PR source, i.e. from my repo fork of the org repo as is the usual workflow: Fork, create branch, commit, PR.

So again: What are the expected values for "branch" and "commit"? There has to be some specification somewhere.

I'd guess that the commit-sha for PR builds should be that of the merge commit. Which means the Drone "plugin" is wrong and the GHA one is correct.

Edit:
So for Drone to match GHA I'd say those variables should be used for PRs:
https://docs.drone.io/pipeline/environment/reference/drone-commit-after/
https://docs.drone.io/pipeline/environment/reference/drone-source-branch/
See the similar code for GHA at

if (envs.GITHUB_HEAD_REF && envs.GITHUB_HEAD_REF !== '') {
branch = envs.GITHUB_HEAD_REF
}

@Flamefire
Copy link
Contributor Author

@drazisil-codecov Can you comment on what the expected values for the "branch" and "commit" parameters are for PR builds? Or at least forward that question to someone who knows?
E.g. I have the following values for a single PR build on different CIs:

Appveyor
branch=master&commit=81618b81272d514a21cc16425371c96f5c7108c3
Drone
branch=master&commit=f1684814b9f2cf5d051dccb935a91f1c602c694b
GHA
branch=feature&commit=81618b81272d514a21cc16425371c96f5c7108c3

@Flamefire
Copy link
Contributor Author

Ok some update: Checking the code for Github Actions:

if (pr && pr !== '' && !args.sha) {
const mergeCommitRegex = /^[a-z0-9]{40} [a-z0-9]{40}$/
const mergeCommitMessage = childProcess
.spawnSync('git', ['show', '--no-patch', '--format="%P"'])
.stdout.toString()
.trimRight()
if (mergeCommitRegex.exec(mergeCommitMessage)) {
const mergeCommit = mergeCommitMessage.split(' ')[1]
info(` Fixing merge commit SHA ${commit} -> ${mergeCommit}`)
commit = mergeCommit
} else if (mergeCommitMessage === '') {
info(
'-> Issue detecting commit SHA. Please run actions/checkout with fetch-depth > 1 or set to 0',
)
}
}

This means an effort is made to get the branch HEAD commit SHA, i.e. not the merge commit SHA.
This means both GHA and appveyor are wrong. On appveyor you need to use APPVEYOR_PULL_REQUEST_HEAD_COMMIT if that is set.
On github it is more tricky. Following the code of _getSHA:

  • pr is set correctly as evident from the URL and the value of GITHUB_REF
  • args.sha is not set as I don't pass --sha
  • I echoed the output of git show --no-patch--format="%P" and got e.g. '089ed2f235c1bf407cda2d86d406ad569ee8d9f2 300c8558b1c8b00c7179caf85b33e82940415ef3' (w/o quotes) which should match the (slightly faulty) mergeCommitRegex
  • Hence I should get the "Fixing merge commit SHA" message

But I don't and the SHA is wrong. I can't tell why without further logging information.

Flamefire added a commit to Flamefire/uploader that referenced this issue Dec 3, 2021
Add logging information for the fix of merge commits or when it was not done/required

See codecov#525 for context
Flamefire added a commit to Flamefire/uploader that referenced this issue Dec 3, 2021
Add logging information for the fix of merge commits or when it was not done/required

See codecov#525 for context
@drazisil-codecov
Copy link
Contributor

@Flamefire

I was on vacation, sorry for the delay in responding.

The commit SHA and branch should be whatever is the current location of the commit you are uploading coverage for, with one exception:

In some CI pipelines, a "test merge" commit is made in CI to see what the changed merged would look like. In this case, Codecov will fail if we try to process a report uploaded on that commit since that SHA is never pushed to the repo from CI and thus doesn't exist. That is why you may see some weirdness around setting the commit in PRs for certain CI platforms.

Does that help?

@Flamefire
Copy link
Contributor Author

Flamefire commented Dec 16, 2021

Yes it helps. So:

This means both GHA and appveyor are wrong.

Let's see what the logs show for GHA. For appveyor the fix is simply to use APPVEYOR_PULL_REQUEST_HEAD_COMMIT, see #534

drazisil-codecov pushed a commit that referenced this issue Dec 16, 2021
Add logging information for the fix of merge commits or when it was not done/required

See #525 for context
@mitchell-codecov
Copy link
Contributor

mitchell-codecov commented Dec 16, 2021

It looks like the documentation has recently been updated with new environment variables:1

Environment variable Description
GITHUB_REF_NAME The branch or tag name that triggered the workflow run.
GITHUB_REF_PROTECTED true if branch protections are configured for the ref that triggered the workflow run.
GITHUB_REF_TYPE The type of ref that triggered the workflow run. Valid values are branch or tag.
GITHUB_HEAD_REF Only set for pull request events. The name of the head branch.
GITHUB_BASE_REF Only set for pull request events. The name of the base branch.

Footnotes

  1. https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables

@Flamefire
Copy link
Contributor Author

What is new here? GITHUB_HEAD_REF is already used by codecov. What is missing/wrong is the SHA

drazisil-codecov pushed a commit that referenced this issue Dec 16, 2021
Add logging information for the fix of merge commits or when it was not done/required

See #525 for context
@multun
Copy link

multun commented Jan 6, 2022

@Flamefire you can get it with github.event.pull_request.head.sha when github.event_name == 'pull_request'

@Flamefire
Copy link
Contributor Author

@Flamefire you can get it with github.event.pull_request.head.sha when github.event_name == 'pull_request'

While this is true, it only works in the GHA context, i.e. in the yaml file as there is no environment variable with that value (yet, IIRC I requested that from the GHA team) So it can't be used for us here without requiring modifications to the workflows and is impossible in scripting (e.g. we have CI scripts in a central repo for all sub-projects)

@JCMais
Copy link

JCMais commented Jan 18, 2022

What is the quick fix for users here?

Detect if github.event_name == 'pull_request' and if that is the case pass -C ${{ github.event.pull_request.head.sha }} to the uploader?

Edit: I can confirm that doing the above fixed the warning message for me. I have added an environment variable with the commit and then used that:

            - name: Codecov
              env:
                  CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
                  COMMIT_SHA: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || env.GITHUB_SHA }}
              run: |
                  curl -Os https://uploader.codecov.io/latest/linux/codecov
                  chmod +x ./codecov
                  ./codecov -t ${CODECOV_TOKEN} -C ${COMMIT_SHA}

@JSv4
Copy link

JSv4 commented Aug 6, 2022

I'm having this same problem when trying to pass through my GitHub Action env variables to a Docker Container using Codecov's provided script:

ci_env=`bash <(curl -s https://codecov.io/env)`

I can see in the GitHub action logs that all of the env variables were passed into the container properly when I open a PR, but the Codecov uploader throws an error Unable to detect SHA and slug, please specify them manually. I had no issues when merging directly into the main branch, so I suspected something about the format of the SHA or slug for a PR was to blame.

Came across this issue and tried the fix suggested by @JCMais. Solved the problem for me.

Looks like this is still a live bug (or missing from the docs)?

@Flamefire
Copy link
Contributor Author

Flamefire commented Aug 9, 2022

This issue has been fixed since 0.1.18 with #649

@JSv4 Your issue seems to be different. Run the uploader in verbose mode to check for issues and open a new issue if there seems to be a bug with the uploader. To me it looks like the relevant variables are not passed to the script, e.g. not passed into your container if you run the script inside it. Anyway: As the issue is different please open a new issue providing all information required to help you.

multun added a commit to OpenRailAssociation/osrd that referenced this issue Nov 10, 2023
The reason why we were running CI on the unmerged commit in
the first place was a now fixed codecov issue.

See codecov/uploader#525

Reverts: 6c242e9
github-merge-queue bot pushed a commit to OpenRailAssociation/osrd that referenced this issue Nov 14, 2023
The reason why we were running CI on the unmerged commit in
the first place was a now fixed codecov issue.

See codecov/uploader#525

Reverts: 6c242e9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants