Add code coverage to pull requests.

[k4leung4]

Summary

Add code coverage to pull requests.

Ticket Link

Fixes #675

Release Note

None

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (main@1530fb1). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #676   +/-   ##
=======================================
  Coverage        ?   47.48%           
=======================================
  Files           ?       65           
  Lines           ?     5600           
  Branches        ?        0           
=======================================
  Hits            ?     2659           
  Misses          ?     2651           
  Partials        ?      290           

---

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 1530fb1...7f4fa05. Read the comment docs.

[lukehinds]

I am not too keen on using this. It was only a couple of months ago codecov experienced a monumental hack where I was part of the clean up. I can see they have moved on from the bash uploader, however the new uploader is marked as a work in progress still.

Are there not some other less powerful options we could perhaps evaluate as well?

[cpanato]

i think if this does not block the merge we can try and see if will be useful. maybe we can use as a checkpoint

[lukehinds]

yep, I think having code coverage monitoring is immensely useful, it's just I am aware we are pulling in an increasing amount of github actions and yet positioning our project as securing the supply chain. If another of these GH actions are compromised again and it's one we pull into our dev process, it's not a good look for us. I would like to see perhaps some alternatives explored / due diligence performed before we settle on this.

[cpanato]

maybe we can run that and save the coverage in the build artifact and then we can download (if someone are interested) and review locally

[dlorenc]

Are there any decent alternatives?

[k4leung4]

Thanks for informing me about the issues codecov had.

An alternative is coveralls, https://coveralls.io/
Listed to be used by a number large companies.

[cpanato]

coverall had issues in the past exposing data and the service sometime does not work :/ but don't know if that improve

[k4leung4]

Any thoughts on "code climate"; https://codeclimate.com/ ?

[nsmith5]

Secrets in Github Actions are only exposed if they are set as an environment variable or input to a step right? There are no secrets in this workflow so its unclear what a breach of CodeCov would accomplish here.

Deployment pipelines should be treated more cautiously, but I can't see a security issue in adding third-party actions to workflow's that have only source code and don't produce any artifacts

[k4leung4]

Secrets in Github Actions are only exposed if they are set as an environment variable or input to a step right? There are no secrets in this workflow so its unclear what a breach of CodeCov would accomplish here.

Deployment pipelines should be treated more cautiously, but I can't see a security issue in adding third-party actions to workflow's that have only source code and don't produce any artifacts

I think we have to be careful, as the default permissions for the github token might be more permissive than it looks.

looking at a recent CI build run, https://github.com/sigstore/rekor/runs/5279675602?check_suite_focus=true
expanding "Set up job" and "GITHUB_TOKEN Permissions", it looks like the repo has defaulted to give write to all permissions other than metadata with read.

I think we should change the repo default action permissions to read only and be in the habit of specifying permissions explicitly, as any omitted permission will then set to none.

https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/

[dlorenc]

I think we should change the repo default action permissions to read only and be in the habit of specifying permissions explicitly, as any omitted permission will then set to none.

+1

[nsmith5]

Ah ok, I think we can simply add

permissions:
  contents: read

Or change the defaults like in the document you linked @k4leung4

With token privs dropped and no other secrets, is there any remaining threat?

[lukehinds]

How do we plan to utilise the action? I see a lot of projects implement code coverage, but never really do much with the stats it produces?

[cpanato]

How do we plan to utilise the action? I see a lot of projects implement code coverage, but never really do much with the stats it produces?

a few ways to use:

• block the PR if the coverage decreases or don't reach the desired coverage threshold
• or just inform what is the coverage for that particular change and for the main branch

[dlorenc]

• block the PR if the coverage decreases or don't reach the desired coverage threshold

Roughly this - it's not worth automating because of edge cases but it's good to know and see it during review.

[cpanato]

deferring to the project owners @lukehinds @bobcallaway

[lukehinds]

If other rekor/codeowners feel differently then the majority states we merge this, but I am still not seeing any great need for this.

I don't find it a challenge to work out code coverage of a patch from just looking at a pull request with my own eyeballs.

I feel like we are integrating this for something that is not even really that much of a problem in the first place, especially when considering the tool is marked as a work in progress.

[bobcallaway]

+1 to having a coverage tool, I don't have a strong allegiance to which one. I think it is more important to have reviewers actually think through what sufficient test coverage for a PR would look like and then evaluate the patch against that standard, taking into complexity and usefulness.

[k4leung4]

Out of the 4 sigstore/rekor-codeowners, 3 have expressed their opinions. @asraa do you have an opinion on whether it would be useful to add a coverage tool or not?

[lukehinds]

I've changed to an approve. Let's just keep an eye on the project as it is still WIP. I just want us to be careful of increasing our attack surface, but with the universal read-only it tempers down the risk more.