-
Notifications
You must be signed in to change notification settings - Fork 261
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
ci: add codecov #866
ci: add codecov #866
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @FrankYang0529!
Re: the CODECOV_TOKEN... it looks like a separate token won't be necessary for this public spin repo, if I'm understanding correctly: https://github.com/codecov/codecov-action#usage
Have you been able to test this on your fork, by chance?
Once we have the token figured out/removed, I am curious whether installing Tarpaulin on the GitHub runner or using the Tarpaulin containers to execute the tests (like here: https://github.com/project-akri/akri/blob/main/.github/workflows/run-tarpaulin.yml) is faster. Either way it can take a long time to run |
462e275
to
99059ec
Compare
Yeah, it looks like we don't need it. I updated a version for it.
Yes, this one https://github.com/FrankYang0529/spin/actions/runs/3369212681/jobs/5588606673 |
I am not sure whether it's faster, but I would prefer not to use containers here, because:
|
99059ec
to
00b1f5a
Compare
Thanks for the updates @FrankYang0529. I see we upload the codecov report as an artifact on the workflow run; what do we envision the current flow for tracking these reports over time? Would it be possible to have the codecov results be a separate check in CI? I'm thinking it would be handy to see it alongside the other checks (currently the |
Yes, I think it's good to separate these in another check so that we can unify test commands in the
I would suggest we don't do this now because the result from the Ptrace engine with
I will do some surveys and test in my repo first. |
Hi @FrankYang0529, checking in to see if I can help on this one. There may be a bit of refactoring needed now that the build.yml workflow has been broken up into multiple jobs. If it isn't too complex, it would still be great to have the codecov checks run as their own job/check (ref #866 (comment)). Otherwise, I'm sure we could also get a version of this in and start refining/interating from there. |
Hi @vdice, sorry for the late update. I will update the PR this weekend. Thanks. |
00b1f5a
to
9200c95
Compare
Signed-off-by: Frank Yang <[email protected]>
9200c95
to
9faf121
Compare
Hi @vdice, I separated code-coverage to another job. I think we can merge this PR first and add Codecov GH App in a follow-up PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @FrankYang0529!
Added a few comments/questions for the broader team but otherwise the automation looks great to me.
use-tool-cache: true | ||
|
||
- name: Cargo Tarpaulin | ||
run: cargo tarpaulin --follow-exec --skip-clean -t 6000 --out xml --features openssl/vendored,default,e2e-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @lann or @radu-matei on if this cargo tarpaulin
command suits our needs per the original issue #367
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that's what I use.
|
||
- name: Upload to codecov.io | ||
uses: codecov/codecov-action@v3 | ||
if: ${{ github.ref == 'refs/heads/main' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we only interested in codecov results for main
at this time? cc @lann @radu-matei
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would imagine we'd like it for PRs too, but its easy enough to change 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, we can see how it works for us for main
and then potentially create a follow-up.
Thanks again for this contribution @FrankYang0529! |
resolve #367
Need to setup
CODECOV_TOKEN
. Thanks