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: use raw ref for PR branch to avoid confusion bw upstream and fork #1132

Closed
wants to merge 2 commits into from

Conversation

matt-codecov
Copy link
Contributor

same PR as codecov/codecov-cli#237 but for the nodejs uploader

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

@thomasrockhu-codecov
Copy link
Contributor

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

Is this documented anywhere?

@matt-codecov
Copy link
Contributor Author

@thomasrockhu-codecov y'know, that's a great question. could you point me to how to update docs and maybe suggest a place for it if one comes to mind?

@thomasrockhu-codecov
Copy link
Contributor

@matt-codecov I think since this is a pretty big paradigm shift, I'd like to pull in @trent-codecov and/or @katia-sentry for their take

for those of you joining us, I don't know the impact this will have on users that don't already run CI on push AND pr events

@matt-codecov
Copy link
Contributor Author

PR comments should still work for same-repo PRs, i think their coverage will just appear stale in the Codecov website's branch UI because the coverage reports will be filed under refs/pull/#/merge instead of the branch name.

a relevant note from the corresponding codecov-cli PR:

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.

@thomasrockhu-codecov
Copy link
Contributor

@matt-codecov any update on this?

@matt-codecov
Copy link
Contributor Author

just got back from PTO. an alternate approach was noted on the corresponding CLI PR codecov/codecov-cli#237 which i'll look into when i get time again

@matt-codecov
Copy link
Contributor Author

not continuing with this; the issue is fixed in https://github.com/codecov/codecov-cli and we are working towards deprecating this uploader

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

2 participants