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

Document how to separate codecov-action use into a separate job #300

Closed
briansmith opened this issue Apr 29, 2021 · 11 comments
Closed

Document how to separate codecov-action use into a separate job #300

briansmith opened this issue Apr 29, 2021 · 11 comments

Comments

@briansmith
Copy link

briansmith commented Apr 29, 2021

In the README.md of this action, the documented way of using codecov-action is to run it as a step of the same job that collects the coverage info. However, I think it would be better for security and reliability if CodeCov recommended using codecov-action in a separate job instead.

I have been investigating minimization of GITHUB_TOKEN permissions based on https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/ and similar. According to that blog post, it is only possible to control permissions on GITHUB_TOKEN on a per-workflow or per-job basis, not a per-step basis. It thus seems GitHub Actions treats the job as the unit of security. Consequently, it seems desirable to put tasks with different permission needs into separate jobs. In particular, I think people will start wanting to have separate jobs for collecting coverage and uploading coverage to Codecov. This way, the job that uploads to codecov wouldn't even need contents: read permission on the GitHub token and it wouldn't accidentally get access to the token through the unfortunate persist-credentials: true default of actions/checkout.

Further in issue #234 some users seem to have encountered some timeout issue when there is a long period of time between jobs that submit coverage to codecov when they don't use a codecov token. Codecov developers have suggested that using a codecov access token is a way to work around that problem. However, I think many people are like me and would like to avoid maintaining a GitHub secret just for the codecov token for a public [edit] repo. An alternative solution would be:

  1. Run all the jobs that collect coverage info and save the coverage data as an GitHub Actions artifacts.
  2. Have a separate job for uploading to codecov, that depends on the jobs that collect the coverage. This job would download all the artifacts from the coverage collection jobs and then submit them all at once to codecov.

Since the upload to codecov would happen in only one place, the timeout issue would seem to go away.

@briansmith
Copy link
Author

Also, having a separate job for the uploading would reduce the annoyance where the Codecov commenter comments about a reduction of code coverage before it has received all the coverage data.

@briantist
Copy link

This sounds great on both counts. The comments and premature failure before all jobs complete really is annoying.

@thomasrockhu thomasrockhu self-assigned this May 6, 2021
@thomasrockhu
Copy link
Contributor

@briansmith I wrote up an example here. I'll be writing a short post explaining it out better, but is this helpful?

@briantist
Copy link

I wrote up an example here. I'll be writing a short post explaining it out better, but is this helpful?

(I'm not the OP) @thomasrockhu I haven't tried it yet but this looks like a good example for splitting the jobs. I might give this a try for the failure-before-tests-are-finished issue at least, thanks for that.

I'd like to see more info on properly the limiting token permissions on that final codecov job to the minimum possible.

@briansmith
Copy link
Author

@thomasrockhu Thank you. My interest in this is twofold: One to reduce the false positive of Codecov reporting coverage decreasing when not all the uploads have finished yet, and also for security. I think your example does a good job of demonstrating the solution to the first problem. Could you modify it to add the permissions: fields at the top-level and at each job level so that it demonstrates the GitHub Token permissions required?

In particular, I am wondering what permission the GitHub actions/upload-artifact action requires to upload the artifact. It must require some kind of write permission, but what? contents: write? If so, then this cure would be worse than the disease for most people, because most people would be much safer if they didn't allow contents: write for any job.

@briansmith
Copy link
Author

I also wonder if the solution I propose is really the best one?

Another solution to the "codecov reports coverage loss when not all the uploads have completed yet" problem would be to add an option to codecov-action like "report: false" that would indicate that codecov shouldn't report anything yet in response to the upload. Then we'd have add another job that would use codecov-action with "report: true" (and possibly no input files) to indicate that it's time to report the coverage.

Especially if actions/upload-artifacts requires write permissions for the github token, which it must, then I think this alternate solution would be greatly preferably from a security standpoint, as it would allow us to completely minimize the permissions while still getting the intended delayed reporting behavior.

WDYT?

@briansmith
Copy link
Author

Indeed, assuming the comment actions/upload-artifact#197 (comment) is correct, I wouldn't feel comfortable using upload-artifact.

@thomasrockhu-codecov
Copy link
Contributor

@briansmith sorry, it has been a while since we talked about this and I've lost a bit of context on the need here. If I remember correctly, the ask is for a repo sending over 100 uploads per commit. Is this right? And you want to be able to separate out the upload step so that we can handle the number of reports

@fh-bscholer
Copy link

@thomasrockhu-codecov I'm not the issue opener but I don't see anything in here referencing an issue of 100+ commits. The OP seems to have all the context needed in it: separating the upload step serves two purposes:

  • separation of the job allows separation of GitHub token privileges
  • waiting to upload all the reports at once reduces the chance of codecov failing the CI because it hasn't yet processed all the reports (since some haven't been uploaded yet)

@thomasrockhu-codecov
Copy link
Contributor

@fh-bscholer you're right, the 100+ commits was from my example repo, and I got my streams crossed.

@fh-bscholer
Copy link

@thomasrockhu-codecov I see this was closed as completed, it would be helpful to link to requested documentation or explanation, thanks!

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

No branches or pull requests

5 participants