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

use raw ref for PR branch to avoid confusion bw upstream and fork #237

Closed
wants to merge 1 commit into from

Conversation

matt-codecov
Copy link
Contributor

@matt-codecov matt-codecov commented Aug 18, 2023

same as codecov/uploader#1132 but for codecovcli

when we upload a report for CI in a PR, we associate the report with the destination repo but we take the branch name from the source repo. so if i make a PR from my fork's main branch to an upstream repo's main branch, we upload the coverage report for the upstream repo's main branch and update the coverage with junk from my un-merged PR

this PR instead will upload refs/pull/#/merge as the branch for PRs. it's slightly less nice for same-repo PRs because in order to populate coverage for your feature branches you'll need CI running on push as well as PR, but avoids invalidating coverage for projects that use a super common fork/merge workflow

docs on the environment variables used here: https://docs.github.com/en/actions/learn-github-actions/variables

Copy link
Contributor

@dana-yaish dana-yaish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for solving this bug, but I have a few questions
If I understand correctly, If a PR is opened from the fork repo to the upstream one, the sent slug is the upstream's repo slug, so the bug happens because of the mistaken slug, and the coverage report gets uploaded to the upstream's branch not the fork one. How is changing the branch to be refs/pull/#/merge distinguishes between the two repos?

@matt-codecov
Copy link
Contributor Author

@dana-yaish the bug is that the slug and branch name are from different repos, and i think it makes sense to send the upstream repo's slug because when you open a PR it's the upstream repo's CI that is running. so if the upstream repo's slug is right, the pseudo-branch name we use should also be from the upstream repo

i think this solution is a little awkward, but it fixes a glaring bug and i don't think it limits our options for improving how forks/branches are handled in codecov down the line

@Andrew-S-Rosen
Copy link

Thank you so much, @matt-codecov. This has been bugging me for ages! 😄

@matt-codecov
Copy link
Contributor Author

matt-codecov commented Aug 21, 2023

it may be a little while before a fix actually ships. i am away for a couple weeks soon and there are still a few things to work out:

  • is the same change needed for other CI providers?
  • is there something we can do on the backend to make this change transparent for same-repo PRs?
  • if not, what can we do to minimize disruption to that workflow? (doc updates, UI banners, etc)

that said, we're eager to fix this. if it's not solidified before i leave i'll see if any teammates have space to drive it home.

@scott-codecov
Copy link
Contributor

Could the CLI fetch info about the particular pull using the GitHub API? The action should have a token available implicitly via a GITHUB_TOKEN secret: https://docs.github.com/en/actions/security-guides/automatic-token-authentication

Parse the pull number out of refs/pull/{number}/merge and use it to make a request to https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#get-a-pull-request (repo slug available via GITHUB_REPOSITORY env var). If the head.repo.full_name is different than GITHUB_REPOSITORY then I think we can assume it's a pull from a fork. If the pull is from a fork, name the branch something like {forked-repo-slug}:{forked-branch-name}.

Unfortunately I think you'd need to explicitly map the secret into the env when configuring the workflow. i.e. something like:

env:
  GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

@thomasrockhu-codecov
Copy link
Contributor

@matt-codecov is this PR needed anymore?

@matt-codecov
Copy link
Contributor Author

ah no, i believe gio's tokenless implementation addressed this! closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants