ci: Reconfigure CodeCov action to ensure stability #1414

shahzadlone · 2023-04-27T16:03:35Z

Relevant issue(s)

Resolves #1413

Description

Note: The first 3 commits are house keeping stuff that I didn't want to open a separate issue for.

Ensures Codecov doesn't run on every push (code coverage reports will only generate for every PR, or pushes on master and develop). This also means contributors don't need to have codecov to have a build run successfully on their fork pushes.
If for whatever reason the code cov token doesn't exist, still run the action (but might be flakey - so retry until passes upto 5 times).

Future: Should probably rework this to do the pull_request -> trigger a workflow_run that will be passed the code coverage report that will then have the secrets in the second privileged action run. But until then hopefully this can get us by.

How has this been tested?

Fork pushes and this PR pushes.

codecov · 2023-04-27T16:14:47Z

Codecov Report

Merging #1414 (e795702) into develop (b04c423) will decrease coverage by 0.20%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1414      +/-   ##
===========================================
- Coverage    72.17%   71.98%   -0.20%     
===========================================
  Files          185      185              
  Lines        18295    18295              
===========================================
- Hits         13204    13169      -35     
- Misses        4050     4076      +26     
- Partials      1041     1050       +9

see 7 files with indirect coverage changes

AndrewSisley · 2023-04-27T16:25:44Z

Partial hits are considered hits, we tried to find a way to do this a while back but this feature didn't exist.

This sounds like us fudging the numbers, prioritising marketing over engineering - what are your reasons for wanting to do this?

AndrewSisley

I have two questions, one major, one minor that need resolving before merge please :)

.github/codecov.yml

shahzadlone · 2023-05-01T12:11:13Z

Partial hits are considered hits, we tried to find a way to do this a while back but this feature didn't exist.

This sounds like us fudging the numbers, prioritising marketing over engineering - what are your reasons for wanting to do this?

I disagree that this is fudging the numbers, I would argue that our test coverage right now as it stands is numbers fudged against us. I can share more thoughts in a discord thread as this seems like a good discussion point. It's probably also best to do this in a separate PR.

Note: Code cov actions might need to be reworked slightly before this PR can merge even if this is the only blocker. Will ping once the changes have been made.

AndrewSisley · 2023-05-01T15:17:49Z

I disagree that this is fudging the numbers, I would argue that our test coverage right now as it stands is numbers fudged against us. I can share more thoughts in a discord thread as this seems like a good discussion point. It's probably also best to do this in a separate PR.

Good call, can you please remove this from this PR? Even if we all agreed this is what we want I think it would be very good for it to have its own commit, and documentation on why we want to do this (if we do). A discord thread would also be good to figure out where the team stands on this.

shahzadlone · 2023-05-01T17:19:37Z

I disagree that this is fudging the numbers, I would argue that our test coverage right now as it stands is numbers fudged against us. I can share more thoughts in a discord thread as this seems like a good discussion point. It's probably also best to do this in a separate PR.

Good call, can you please remove this from this PR? Even if we all agreed this is what we want I think it would be very good for it to have its own commit, and documentation on why we want to do this (if we do). A discord thread would also be good to figure out where the team stands on this.

Yup totally aligned with that

shahzadlone · 2023-05-03T09:37:02Z

Introducing a retry mechanic to get by for now, seems like codcov team is aware of this bug that happens without a token. Until they push it we can rely on this retry mechanic.

In future should probably rework using a 2 step workflow where first work is not privileged and generates the report, then the report is passed to a privileged workflow run action that has the secrets to upload using the token.

AndrewSisley · 2023-05-03T14:40:41Z

.github/workflows/code-test-coverage.yml

+        env: 
+          codecov_secret: ${{ secrets.CODECOV_TOKEN }}
+        if: env.codecov_secret == ''
+        uses: Wandalen/[email protected]


todo: Can we please at least use a more popular dependency if we are going to introduce one? The one currently on this line only has 35 stars, whereas this one has 245 and was built at/for a well known company whereas wandalens appears to be purely a personal project.

Which one are you recommending? I just saw many people use this one to fix the code cov flakey stuff. Happy to look into another one that might be more reputable.

lol, I forgot to link it. I had it open and ready for you sorry. This is the one I meant to suggest: https://github.com/marketplace/actions/retry-step

It seems to be intended to replace the run step commands not action uses. LMK if you see an example for retries upon an action use.

More info:
https://stackoverflow.com/a/71583518

You should be able to run the code cov action as a command no?

I'm just very uncomfortable introducing such an unused personal project into our CI system, and I think if I had to choose between using that action, or having a flaky code cov action, I would probably go for the flaky code cov action.

I would rather use the official codecov github action than the shell command as it takes into account some built-in github metadata when it uploads the data to codcov.

I disagree that it is an unused personal project as it has thousands of uses already: https://github.com/search?q=Wandalen%2Fwretry.action+path%3A.github%2Fworkflows%2F+language%3AYAML&type=Code&s=indexed&o=desc

Some of the users are shopify, open-metadata, Isar Database, Airbyte, Immich

We shouldn't not solve the problem just because we have to use a 3rd party action. We already use a lot of 3rd party actions, even when we weren't open-sourced.

Also keep in mind, this will mostly be a fallback case when a token isn't available (ASFAIK it will always be there). So majority of times this won't even be hit or used once the rework of this action is done.

I think it's pretty safe to use the one Shahzad has at the moment. Specially that it's set on a version so future changes won't affect us unless we update without checking what the changes are.

fredcarle

LGTM!

answered

## Relevant issue(s) Resolves sourcenetwork#1413 ## Description - Ensures Codecov doesn't run on every push (code coverage reports will only generate for every PR, or pushes on `master` and `develop`). This also means contributors don't need to have codecov to have a build run successfully on their fork pushes. - If for whatever reason the code cov token doesn't exist, still run the action (but might be flakey - so retry until passes up to 5 times). Future: Should probably rework this to do the `pull_request` -> trigger a `workflow_run` that will be passed the code coverage report that will then have the secrets in the second privileged action run. But until then hopefully this can get us by. ## How has this been tested? Fork pushes and this PR pushes.

shahzadlone added the ci/build This is issue is about the build or CI system, and the administration of it. label Apr 27, 2023

shahzadlone added this to the DefraDB v0.5.1 milestone Apr 27, 2023

shahzadlone requested a review from a team April 27, 2023 16:03

shahzadlone self-assigned this Apr 27, 2023

AndrewSisley requested changes Apr 27, 2023

View reviewed changes

.github/codecov.yml Outdated Show resolved Hide resolved

shahzadlone added the action/no-benchmark Skips the action that runs the benchmark. label Apr 27, 2023

shahzadlone force-pushed the lone/ci/make-code-cov-less-flakey branch from 55d0a9e to 51c4286 Compare May 1, 2023 12:12

shahzadlone force-pushed the lone/ci/make-code-cov-less-flakey branch 3 times, most recently from d84ecb0 to bd02af6 Compare May 3, 2023 09:28

shahzadlone requested review from AndrewSisley and a team May 3, 2023 09:33

shahzadlone mentioned this pull request May 3, 2023

possible bug that doesn't update PR comment and coverage on force push with no new commits codecov/codecov-action#972

Closed

AndrewSisley previously requested changes May 3, 2023

View reviewed changes

shahzadlone requested a review from AndrewSisley May 3, 2023 22:33

shahzadlone force-pushed the lone/ci/make-code-cov-less-flakey branch 2 times, most recently from 32b6306 to 3d1632e Compare May 3, 2023 22:36

shahzadlone requested a review from a team May 3, 2023 22:55

fredcarle approved these changes May 13, 2023

View reviewed changes

shahzadlone force-pushed the lone/ci/make-code-cov-less-flakey branch from 3d1632e to 3024d50 Compare May 15, 2023 19:18

shahzadlone added 4 commits May 17, 2023 10:28

PR: Fix semver tag-push patterns.

8169809

PR: Restrict to matching pull request base.

8d6e19e

PR: make name consistent and remove whitespace.

ddd4c6f

PR: Restrict codecov to not run on all pushes.

a75098f

PR: Split codecov action into with & without token with retry logic

e795702

shahzadlone force-pushed the lone/ci/make-code-cov-less-flakey branch from 3024d50 to e795702 Compare May 17, 2023 14:36

shahzadlone merged commit 0621856 into sourcenetwork:develop May 17, 2023

shahzadlone deleted the lone/ci/make-code-cov-less-flakey branch May 18, 2023 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: Reconfigure CodeCov action to ensure stability #1414

ci: Reconfigure CodeCov action to ensure stability #1414

shahzadlone commented Apr 27, 2023 •

edited

Loading

codecov bot commented Apr 27, 2023 •

edited

Loading

AndrewSisley commented Apr 27, 2023

AndrewSisley left a comment

shahzadlone commented May 1, 2023

AndrewSisley commented May 1, 2023

shahzadlone commented May 1, 2023

shahzadlone commented May 3, 2023

AndrewSisley May 3, 2023

shahzadlone May 3, 2023

AndrewSisley May 3, 2023

shahzadlone May 3, 2023

shahzadlone May 3, 2023

AndrewSisley May 4, 2023

shahzadlone May 4, 2023 •

edited

Loading

shahzadlone May 4, 2023

fredcarle May 13, 2023

fredcarle left a comment

ci: Reconfigure CodeCov action to ensure stability #1414

ci: Reconfigure CodeCov action to ensure stability #1414

Conversation

shahzadlone commented Apr 27, 2023 • edited Loading

Relevant issue(s)

Description

How has this been tested?

codecov bot commented Apr 27, 2023 • edited Loading

Codecov Report

AndrewSisley commented Apr 27, 2023

AndrewSisley left a comment

Choose a reason for hiding this comment

shahzadlone commented May 1, 2023

AndrewSisley commented May 1, 2023

shahzadlone commented May 1, 2023

shahzadlone commented May 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone May 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

shahzadlone commented Apr 27, 2023 •

edited

Loading

codecov bot commented Apr 27, 2023 •

edited

Loading

shahzadlone May 4, 2023 •

edited

Loading