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

[skip changelog] Enable Codecov comments on PRs from forks #1819

Merged
merged 1 commit into from
Aug 1, 2022
Merged

[skip changelog] Enable Codecov comments on PRs from forks #1819

merged 1 commit into from
Aug 1, 2022

Conversation

per1234
Copy link
Contributor

@per1234 per1234 commented Jul 31, 2022

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • [N/A] Tests for the changes have been added (for bug fixes / features)
  • [N/A] Docs have been added / updated (for bug fixes / features)
  • [N/A] UPGRADING.md has been updated with a migration guide (for breaking changes)

What kind of change does this PR introduce?

Infrastructure enhancement

What is the current behavior?

Versions of the codecov/codecov-action GitHub Actions action prior to 1.0.6 required the use of a token provided by Codecov in order to upload coverage data to Codecov. This token was stored in a secret in the Arduino CLI repository and used in the test workflow.

For security reasons, secrets are not accessible when a workflow is triggered by an event generated by a fork of the repository. This meant that it was impossible to upload coverage data for the test runs triggered by PRs from forks (codecov/codecov-action#29). A conditional was added to the upload step of the workflow to cause it to only run on push event triggers, which effectively prevented its failure for runs on PRs from forks (#388).

The token requirement was removed in the 1.0.6 release of codecov/codecov-action, but the now pointless conditional was never removed from the workflow. This prevented PRs from forks from receiving the automated code coverage report comments that would otherwise encourage those contributors to resolve coverage deficiencies and facilitate the review process.

What is the new behavior?

The harmful conditional is removed from the coverage data upload steps of the workflow and PRs from forks will now receive coverage report comments, just as PRs from branches do already.

Does this PR introduce a breaking change

No breaking change.

Other information

The configuration of the coverage data upload step proposed here is aligned with our standardized "template" workflow:

https://github.com/arduino/tooling-project-assets/blob/d9f73eb4e5b16c0141e184d7f04493cd62221ea4/workflow-templates/test-go-task.yml#L104

That form of the workflow is already in use in several other Arduino tooling projects. For example:

Versions of the `codecov/codecov-action` GitHub Actions action prior to 1.0.6 required the use of a token provided by
Codecov in order to upload data to Codecov. This token was stored in secret in the Arduino CLI repository and used in
the test workflow.

For security reasons, secrets are not accessible when a workflow is triggered by an event generated by a fork of the
repository. This meant that it was impossible to upload coverage data for the test runs triggered by PRs from forks. A
conditional was added to the upload step of the workflow to cause it to only run on `push` event triggers, which
effectively prevented its failure for runs on PRs from forks.

The token requirement was removed in the 1.0.6 release of `codecov/codecov-action`, but the now pointless conditional
was never removed from the workflow. This prevented PRs from forks from receiving the automated code coverage report
comments that would otherwise encourage those contributors to resolve coverage deficiencies and facilitate the review
process.

The harmful conditional is hereby removed from the coverage data upload steps of the workflow and PRs from forks will
now receive coverage report comments, just as PRs from branches do already.
@per1234 per1234 added type: enhancement Proposed improvement topic: infrastructure Related to project infrastructure labels Jul 31, 2022
@per1234 per1234 self-assigned this Jul 31, 2022
@codecov
Copy link

codecov bot commented Jul 31, 2022

Codecov Report

Merging #1819 (6fa9645) into master (5332ffd) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1819   +/-   ##
=======================================
  Coverage   35.53%   35.53%           
=======================================
  Files         231      231           
  Lines       19543    19543           
=======================================
  Hits         6945     6945           
  Misses      11773    11773           
  Partials      825      825           
Flag Coverage Δ
unit 35.53% <ø> (ø)

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


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5332ffd...6fa9645. Read the comment docs.

Copy link
Contributor

@MatteoPologruto MatteoPologruto left a comment

Choose a reason for hiding this comment

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

Good job for spotting, highlighting and solving the problem! The PR's message is very clear.

@per1234 per1234 merged commit a55df0d into arduino:master Aug 1, 2022
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.

2 participants