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

Added codecov token to solve code-coverage upload issues #2129

Merged
merged 2 commits into from
Mar 29, 2023

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Mar 27, 2023

Please check if the PR fulfills these requirements

See how to contribute

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • configuration.schema.json updated if new parameters are added.

What kind of change does this PR introduce?

Added codecov token to solve code-coverage upload issues

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change, and is titled accordingly?

Other information

See codecov/codecov-action#557

@cmaglie cmaglie added type: enhancement Proposed improvement topic: infrastructure Related to project infrastructure labels Mar 27, 2023
@cmaglie cmaglie self-assigned this Mar 27, 2023
@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.09 🎉

Comparison is base (074844d) 62.47% compared to head (bc7014b) 62.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2129      +/-   ##
==========================================
+ Coverage   62.47%   62.56%   +0.09%     
==========================================
  Files         231      231              
  Lines       19593    19610      +17     
==========================================
+ Hits        12240    12269      +29     
+ Misses       6250     6238      -12     
  Partials     1103     1103              
Flag Coverage Δ
unit 62.56% <ø> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 12 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be noted that PRs from forks do not have access to repository secrets, so this fix only applies to PRs from branches of the arduino/arduino-cli repository. The same intermittent spurious workflow run failures will continue to occur for PRs from forks (such as this PR).

For example, check the workflow run logs for this PR:
https://github.com/arduino/arduino-cli/actions/runs/4534782144/jobs/7989648996?pr=2129#step:7:27

[2023-03-27T17:36:03.699Z] ['info'] -> No token specified or token is empty

The alternative solution would be to add the token in plaintext directly in the workflow. The security implications of that approach are described here:

https://community.codecov.com/t/upload-issues-unable-to-locate-build-via-github-actions-api/3954

Public repositories that rely on PRs via forks will find that they cannot effectively use Codecov if the token is stored as a GitHub secret. The scope of the Codecov token is only to confirm that the coverage uploaded comes from a specific repository, not to pull down source code or make any code changes.

For this reason, we recommend that teams with public repositories that rely on PRs via forks consider the security ramifications of making the Codecov token available as opposed to being in a secret.

A malicious actor would be able to upload incorrect or misleading coverage reports to a specific repository if they have access to your upload token, but would not be able to pull down source code or make any code changes.

@cmaglie
Copy link
Member Author

cmaglie commented Mar 28, 2023

I see, let's try to publish it, if we find abuses we could regenerate it from the CodeCov website.

.github/workflows/test-go-task.yml Outdated Show resolved Hide resolved
.github/workflows/test-go-task.yml Outdated Show resolved Hide resolved
.github/workflows/test-go-task.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working as intended now:

https://github.com/arduino/arduino-cli/actions/runs/4551550129/jobs/8026020631#step:8:29

[2023-03-29T08:05:58.070Z] ['info'] ->  Token found by environment variables

Thanks Cristian!

cmaglie added a commit to arduino/pluggable-discovery-protocol-handler that referenced this pull request Dec 5, 2023
The Codecov upload token will be no longer optional in v4 of the action,
so it will be necessary to adjust the workflow to provide the
token to the action at the same time as the bump. Doing so is a good
idea anyway because, although supported in v3, the lack of a token
was causing periodic spurious coverage data upload failures. That
was done already in the Arduino CLI repo (arduino/arduino-cli#2129)
and the approach used there has passed the test of time with flying
colors.

The workflow does currently use the token input of the action, but
it uses the flawed approach of storing it in a secret, which would
cause the workflow run to fail on any PR submitted from a fork after
the action bump. So the workflow must be updated to use the system
implemented in the Arduino CLI workflow.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: infrastructure Related to project infrastructure type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants