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

Fix branch conflation (PR from fork's main to upstream's main shouldn't overwrite coverage) #661

Closed
matt-codecov opened this issue Oct 10, 2023 · 7 comments · Fixed by codecov/codecov-cli#331
Assignees
Labels
bug Something isn't working

Comments

@matt-codecov
Copy link

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 definitely happens for GitHub, didn't check other providers

the fix needs to be written for both codecov-cli and uploader. you can see relevant areas of the code in the following PRs:

however, a different approach is favored over the one in those PRs: fetch PR information from the git provider API to determine if the PR is from a fork. if so, name the branch "fork-slug:branch". otherwise just name it "branch"

in addition to making the fix for GitHub, check if it's necessary for the other providers and fix if so. this isn't the only issue in codecov's conceptual model of git, but it's probably the most visible and disruptive one.

@matt-codecov matt-codecov added the bug Something isn't working label Oct 10, 2023
@trent-codecov
Copy link
Contributor

Dana, assigning this to you as you will likely be fixing this as part of the wider tokenless changes.

@dana-yaish
Copy link

this depends on #733

@dana-yaish
Copy link

dana-yaish commented Nov 9, 2023

After making the CLI aware of PRs made from forks, we'll be amending the branch that's sent to the API to follow a specific format that can represent a PR made in forks. Since colons are prohibited in branch names, we'll follow how github/gitlab displays branch names in their UI in PRs made fro a fork repo to the upstream one. branches are displayed as fork-slug:branch-name.

@dana-yaish
Copy link

Change for the CLI is done, but we still need to fix it in the uploader and I don't have enough time to do that, so I'll list below the process we did for the CLI and we should do the same in the uploader
The idea is as the following:
1- if token is missing, check if the PR is coming from a fork. Currently we're only doing this for github. We can use the get_pull endpoint for that. If pull's head repo is different from pull's base repo, then it's a PR from a fork.
2- If it's from a fork, override the branch to be something link forked-repo-slug:branch-name
3- send the overridden branch to codecov instead of the old one

One thing we need to check before releasing this:

  • if we actually use the branch saved in the db for making requests to gh/gl/bb. If we actually do that, further changes need to be done. We'll have to create a new column 'fork' which indicates if a commit is from a fork. we'll need to check the branch in the API and set the 'fork' to true, and change the branch to something that the REST API of the provider will accept

@matt-codecov
Copy link
Author

matt-codecov commented Dec 13, 2023

this is still in progress on the API side, reopening and passing to gio

@matt-codecov
Copy link
Author

no hooky it's not done

@matt-codecov matt-codecov reopened this Dec 13, 2023
@giovanni-guidini
Copy link

I believe this is fixed by codecov/worker#217

@codecov-hooky codecov-hooky bot closed this as completed Mar 5, 2024
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

Successfully merging a pull request may close this issue.

4 participants