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

Do not upload coverage results to Codecov when run in a fork #963

Closed

Conversation

jdblischak
Copy link
Contributor

When I sync my fork of a repository that has set CODECOV_TOKEN to upload coverage results to Codecov (#834), the step to upload the coverage results to Codecov fails because my fork has not defined CODECOV_TOKEN.

This PR updates the Codecov upload step to be skipped when running in a fork. However, it will still perform a token-less upload in a PR submitted from a fork. I learned about the existence of the variable github.event.repository.fork from peaceiris/actions-gh-pages#153 (comment)

I've tested this update in two repositories I help maintain (Merck/simtrial#307, Merck/gsDesign2#496).

@gaborcsardi
Copy link
Member

gaborcsardi commented Jan 28, 2025

Thanks! The example workflows already ignore the upload failures in PRs if there is no token. Isn't that enough?

@jdblischak
Copy link
Contributor Author

The example workflows already ignore the upload failures in PRs if there is no token. Isn't that enough?

This PR addresses when I sync the upstream changes to my fork of the repository. When test-coverage.yaml is triggered by a push event in a fork that doesn't have CODECOV_TOKEN, it fails. Here is an example run:

==> Running command '/home/runner/work/_actions/codecov/codecov-action/v4/dist/codecov create-commit'
/home/runner/work/_actions/codecov/codecov-action/v4/dist/codecov create-commit --git-service github -Z
info - 2025-01-09 19:58:36,624 -- ci service found: github-actions
warning - 2025-01-09 19:58:36,633 -- Branch `main` is protected but no token was provided
warning - 2025-01-09 19:58:36,633 -- For information on Codecov upload tokens, see https://docs.codecov.com/docs/codecov-tokens
info - 2025-01-09 19:58:36,973 -- Process Commit creating complete
error - 2025-01-09 19:58:36,974 -- Commit creating failed: {"message":"Token required because branch is protected"}
Error: Codecov: Failed to properly create commit: The process '/home/runner/work/_actions/codecov/codecov-action/v4/dist/codecov' failed with exit code 1

@gaborcsardi
Copy link
Member

I see, so the event in

          fail_ci_if_error: ${{ github.event_name != 'pull_request' || secrets.CODECOV_TOKEN }}

is a push, not a pull_request.

We cannot solely rely on github.event.repository.fork == false because people do real development in forks.

@jdblischak
Copy link
Contributor Author

We cannot solely rely on github.event.repository.fork == false because people do real development in forks.

By this, do you mean that they do development in the fork that they never intend to submit upstream? In other words, you know of use cases where the user wants to maintain a separate Codecov account to track the code coverage only for the fork.

In that case, why don't we check for the existence of CODECOV_TOKEN in the fork? In other words, the logic would be:

Skip the Codecov upload step only if all of the following conditions are true:

  1. The repository is a fork
  2. This is not a Pull Request
  3. CODECOV_TOKEN is not defined

Anyone doing real development in a fork that wants to configure Codecov uploads from their fork will need to define CODECOV_TOKEN anyways, so this should work for their real development use case as well as my use case of simply preparing PRs in a fork.

@gaborcsardi
Copy link
Member

I don't really want to skip the upload completely, because it is supposed to work without a token as well, except in a PR.

I also don't want to skip in forks if there is no token, because I want to know about these cases, so I want an error that I can fix.

But these example workflows are only examples anyway, they are not going to be ideal for everyone. So feel free to adapt them to your needs.

@jdblischak
Copy link
Contributor Author

Thanks for considering my PR. Below I make my final arguments. If you're still unconvinced, then we can just close it.

I don't really want to skip the upload completely

My PR only skips it when running in a fork. And unless the user has installed the Codecov app on their fork, there is nowhere to upload to.

because it is supposed to work without a token as well

In my experience tokenless uploads from push events are rare. Across 3 actively developed GitHub repositories, Codecov upload has consistently failed for over 6 months with the error Rate limit reached. Please upload with the Codecov repository upload token to resolve issue.

except in a PR.

I believe this behavior has changed over time. Now codecov/codecov-action@v4 explicitly supports tokenless uploads for PRs from a fork (codecov/codecov-action#1395). This has been my experience too. Ever since switching to codecov/codecov-action@v4, the tokenless uploads to Codecov from my PRs have succeeded (including when I used github.event.repository.fork == false).

I also don't want to skip in forks if there is no token, because I want to know about these cases, so I want an error that I can fix.

I'm not sure if I fully follow. When running in a fork with no token, there is the error I have reported above: Commit creating failed: {"message":"Token required because branch is protected"}. Maybe this only happens if the default branch is protected? I'm not sure what would happen if the branch was unprotected, or how common it is for users to protect their default branch. But even if they don't get the error about the protected branch, they are still likely to get the error about the rate limit.

But these example workflows are only examples anyway, they are not going to be ideal for everyone. So feel free to adapt them to your needs.

I appreciate that. I have already fixed the workflows in the repositories that I regularly contribute to. I decided to submit this PR because I felt this would be a useful default for the R community. These example workflows are extremely popular, especially due to {usethis}, so an average R package developer will use the example workflow.

My view is that it is much more common for someone to fork a repository, make some changes, check that the CI passes, and then submit a PR. At least this is what I typically do. I've never configured Codecov to run on a fork. I feel that the use case of doing "real development" on a fork is the rarer option. How many R package developers fork a repository, install the Codecov app on the fork, and then define the repository secret CODECOV_TOKEN?

However, I think we could support both use cases (skipping uploads on a fork, or uploading if the user defines CODECOV_TOKEN) with something like the following:

- uses: codecov/codecov-action@v4
  if: github.event.repository.fork == false || secrets.CODECOV_TOKEN

@gaborcsardi
Copy link
Member

I'm not sure if I fully follow. When running in a fork with no token, there is the error I have reported above:

Yes, and I would prefer to keep that error, instead of skipping silently.

How many R package developers fork a repository, install the Codecov app on the fork, and then define the repository secret CODECOV_TOKEN?

You don't need to add a secret, the organization secret applies to the fork as well. I am not sure if there is anything special wrt forks in codecov, either, I don't think I had to do anything special to get coverage for https://app.codecov.io/gh/r-lib/nanoparquet

But this is actually not that rare, abandoned projects get forked all the time, I have at least 10 repos that are forks.

However, I think we could support both use cases (skipping uploads on a fork, or uploading if the user defines CODECOV_TOKEN)

I am not happy with skipping uploads on a fork, sorry. I think if you are pushing to a fork, then either:

  • you are doing active development on main, in which case you want a test coverage upload, otherwise you would not have a test-coverage workflow. Or
  • you are working on a PR, in which case you should not push to main, and then the example test-coverage workflow does not run in the first place, only when you submit a PR, which we handle specially already.

@jdblischak jdblischak closed this Jan 29, 2025
@jdblischak jdblischak deleted the no-cov-upload-from-fork branch January 29, 2025 19:51
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.

2 participants