Skip to content

Include codecov on GitHub Actions #6421

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

Closed
wants to merge 1 commit into from
Closed

Conversation

nicoddemus
Copy link
Member

Follow up from #6420

@nicoddemus
Copy link
Member Author

nicoddemus commented Jan 7, 2020

Hmmm indeed, secrets are only available on forks (as I remember have read in the docs anyway).

It seems currently there's no easy way either to accomplish this with GitHub actions: https://github.community/t5/GitHub-Actions/Make-secrets-available-to-builds-of-forks/td-p/30678

Any hints on how to proceed? Seems having coverage only on the main repository kind of defeats the purpose.

cc @asottile @blueyed @bluetech @Zac-HD @RonnyPfannschmidt @hugovk

@asottile
Copy link
Member

asottile commented Jan 8, 2020

if you're exposing the value to forks it's not really a secret at that point (someone could print(rot13(secret)) in a test or whatever)

might as well put it inline 🤷‍♂

@hugovk
Copy link
Member

hugovk commented Jan 8, 2020

If you put it inline, builds on forks (even if no PR is made) will be sent to the pytest's own Codecov, which clutters up the logs somewhat, and fork users cannot see their own coverage before creating PRs.

We solved temporarily worked around this for Pillow (python-pillow/Pillow#4266) by adding a "Prepare coverage token" step before the "Upload coverage" step:

    - name: Prepare coverage token
      if: success() && github.repository == 'python-pillow/Pillow'
      run: cp .github/codecov-upstream.yml .codecov.yml

    - name: Upload coverage
      if: success()
      uses: codecov/codecov-action@v1
      with:
        token: ${{ secrets.CODECOV_TOKEN }}
        name: ${{ matrix.os }} Python ${{ matrix.python-version }}

https://github.com/python-pillow/Pillow/blob/ef4a0b2f4c9346db37140f102e80068abc280167/.github/workflows/test.yml#L94-L103

.github/codecov-upstream.yml is the same as .codecov.yml, except it has the hardcoded upstream token for Codecov.

If you do want GHA coverage from your own fork's builds sent to your own fork's Codecov, fetch your token from https://codecov.io/gh/my-username/pytest/settings and add it as CODECOV_TOKEN at https://github.com/my-username/pytest/settings/secrets.

@nicoddemus
Copy link
Member Author

So are we all in agreement that even on Travis and Azure the current token is not really a secret, because malicious forks can push code to eventually read them somehow?

If so, I suggest we follow @hugovk's suggestion to avoid having forks uploading coverage by default.

@nicoddemus
Copy link
Member Author

I've pushed a commit that enables the entire matrix with full coverage, so we can see how long it takes in practice.

@nicoddemus
Copy link
Member Author

If you do want GHA coverage from your own fork's builds sent to your own fork's Codecov, fetch your token from codecov.io/gh/my-username/pytest/settings and add it as CODECOV_TOKEN at my-username/pytest/settings/secrets.

Hmm but won't the build fail in case someone doesn't define secrets.CODECOV_TOKEN in their fork (which will be the common case)? Seems this can be resolved with some condition check for the Upload coverage step, just thought I would ask.

@hugovk
Copy link
Member

hugovk commented Jan 9, 2020

If you do want GHA coverage from your own fork's builds sent to your own fork's Codecov, fetch your token from codecov.io/gh/my-username/pytest/settings and add it as CODECOV_TOKEN at my-username/pytest/settings/secrets.

Hmm but won't the build fail in case someone doesn't define secrets.CODECOV_TOKEN in their fork (which will be the common case)? Seems this can be resolved with some condition check for the Upload coverage step, just thought I would ask.

No, it shouldn't fail, it just won't upload any coverage.

Here's a recent PR from a new contributor to Pillow who (most likely) doesn't have any token set in their fork:

==> Uploading reports
    url: https://codecov.io
    query: branch=master&commit=c3232e509310e4bd555201c2cc5feb0e72e14a47&build=&build_url=&name=ubuntu-latest%20Python%20pypy3&tag=&slug=bluepython508%2FPillow&service=github-actions&flags=&pr=&job=
    -> Pinging Codecov
https://codecov.io/upload/v4?package=bash-20191211-b8db533&token=secret&branch=master&commit=c3232e509310e4bd555201c2cc5feb0e72e14a47&build=&build_url=&name=ubuntu-latest%20Python%20pypy3&tag=&slug=bluepython508%2FPillow&service=github-actions&flags=&pr=&job=
HTTP 400
Please provide the repository token to upload reports via `-t :repository-token`

Confirmation of no reports on their Codecov:


I do have my own token defined in my fork:

==> Uploading reports
    url: https://codecov.io
    query: branch=master&commit=ef4a0b2f4c9346db37140f102e80068abc280167&build=&build_url=&name=ubuntu-latest%20Python%20pypy3&tag=&slug=hugovk%2FPillow&service=github-actions&flags=&pr=&job=
    -> Pinging Codecov
https://codecov.io/upload/v4?package=bash-20191211-b8db533&token=secret&branch=master&commit=ef4a0b2f4c9346db37140f102e80068abc280167&build=&build_url=&name=ubuntu-latest%20Python%20pypy3&tag=&slug=hugovk%2FPillow&service=github-actions&flags=&pr=&job=
    -> Uploading
    -> View reports at https://codecov.io/github/hugovk/Pillow/commit/ef4a0b2f4c9346db37140f102e80068abc280167

The corresponding coverage report on my own Codecov:

@nicoddemus
Copy link
Member Author

nicoddemus commented Jan 9, 2020

Oh sure my bad, there's an option in the coverage action to fail the build or not. 😅

Wow seems like all builds took less than 8m or so (except for pypy3 for some reason)... I'm impressed, I thought adding code coverage would slow the whole process down significantly.

@blueyed
Copy link
Contributor

blueyed commented Jan 10, 2020

@nicoddemus

So are we all in agreement that even on Travis and Azure the current token is not really a secret, because malicious forks can push code to eventually read them somehow?

Codecov does not need a token on Travis.
And Travis secrets are not available with forks (Travis does not decrypt them there).
(mainly JFI, but maybe Github actions can be made to also not require a token? - might be necessary for codecov to do so then maybe though)

@hugovk
Copy link
Member

hugovk commented Jan 10, 2020

Yep:

The upload token is required for all uploads, except originating from public project using Travis-CI, Circle CI, or AppVeyor CI.

So Azure Pipelines and GitHub Actions are currently in the same boat.

Hynek Schlawack's "Python in Azure Pipelines, Step by Step":

Please note that this token can leak despite being a “secret” variable. A malicious user can open a PR and send it anywhere they want. However, the token is worthless for anything except uploading coverage and it’s easy to see when someone does it.

@nicoddemus
Copy link
Member Author

So Azure Pipelines and GitHub Actions are currently in the same boat.

That's my understanding too.

I would like an explicit yes from people before proceeding to commit the token to the repository, like it is done on python-pillow/Pillow#4266. To my understanding this is OK, but once done it is technically out there (although I'm sure we can revoke the token somehow if this turns out to be a bad decision).

@asottile @hugovk @blueyed @bluetech @RonnyPfannschmidt

@asottile
Copy link
Member

So Azure Pipelines and GitHub Actions are currently in the same boat.

That's my understanding too.

I would like an explicit yes from people before proceeding to commit the token to the repository, like it is done on python-pillow/Pillow#4266. To my understanding this is OK, but once done it is technically out there (although I'm sure we can revoke the token somehow if this turns out to be a bad decision).

@asottile @hugovk @blueyed @bluetech @RonnyPfannschmidt

send it!

@hugovk
Copy link
Member

hugovk commented Jan 10, 2020

Yes from me too!

If necessary, the token can be regenerated (and presumably revoked) at https://codecov.io/gh/pytest-dev/pytest/settings

nicoddemus added a commit that referenced this pull request Jan 10, 2020
This overwrites the `codecov.yml` file in the root of the repository with
`codecov-upstream.yml` file (which contains the code-cov token)´, so PRs
and branches on the repository can upload coverage.

Suggestion from here:

#6421 (comment)

Security concerns: the token might be misused, but only to upload bogus coverage
to `codecov.io`, so the team believe this is harmless. If we decide to fallback
from this decision , we just need to revoke the token.

Related to #6369
nicoddemus added a commit that referenced this pull request Jan 10, 2020
This overwrites the `codecov.yml` file in the root of the repository with
`codecov-upstream.yml` file (which contains the code-cov token)´, so PRs
and branches on the repository can upload coverage.

Suggestion from here:

#6421 (comment)

Security concerns: the token might be misused, but only to upload bogus coverage
to `codecov.io`, so the team believe this is harmless. If we decide to fallback
from this decision , we just need to revoke the token.

Related to #6369
@nicoddemus nicoddemus force-pushed the gh-actions-cov branch 2 times, most recently from f5ea935 to 0c80a8b Compare January 11, 2020 00:21
nicoddemus added a commit that referenced this pull request Jan 11, 2020
This overwrites the `codecov.yml` file in the root of the repository with
`codecov-upstream.yml` file (which contains the code-cov token)´, so PRs
and branches on the repository can upload coverage.

Suggestion from here:

#6421 (comment)

Security concerns: the token might be misused, but only to upload bogus coverage
to `codecov.io`, so the team believe this is harmless. If we decide to fallback
from this decision , we just need to revoke the token.

Related to #6369
nicoddemus added a commit that referenced this pull request Jan 11, 2020
This overwrites the `codecov.yml` file in the root of the repository with
`codecov-upstream.yml` file (which contains the code-cov token)´, so PRs
and branches on the repository can upload coverage.

Suggestion from here:

#6421 (comment)

Security concerns: the token might be misused, but only to upload bogus coverage
to `codecov.io`, so the team believe this is harmless. If we decide to fallback
from this decision , we just need to revoke the token.

Related to #6369
This overwrites the `codecov.yml` file in the root of the repository with
`codecov-upstream.yml` file (which contains the code-cov token)´, so PRs
and branches on the repository can upload coverage.

Suggestion from here:

#6421 (comment)

Security concerns: the token might be misused, but only to upload bogus coverage
to `codecov.io`, so the team believe this is harmless. If we decide to fallback
from this decision , we just need to revoke the token.

Related to #6369
@nicoddemus nicoddemus closed this Jan 11, 2020
@nicoddemus nicoddemus deleted the gh-actions-cov branch January 11, 2020 15:21
nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Jan 11, 2020
This overwrites the `codecov.yml` file in the root of the repository with
`codecov-upstream.yml` file (which contains the code-cov token)´, so PRs
and branches on the repository can upload coverage.

Suggestion from here:

pytest-dev#6421 (comment)

Security concerns: the token might be misused, but only to upload bogus coverage
to `codecov.io`, so the team believe this is harmless. If we decide to fallback
from this decision , we just need to revoke the token.

Related to pytest-dev#6369
@nicoddemus
Copy link
Member Author

Closed this in favor of #6441 (on my fork).

@hugovk hugovk mentioned this pull request Jan 30, 2020
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.

4 participants