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

Stop failing CI upon codecov error 503 #3173

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Dec 25, 2024

#3172 took 5 attempts to merge.......

cc @jakkdl

Copy link

codecov bot commented Dec 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00000%. Comparing base (3de7996) to head (8e2bfa9).

Additional details and impacted files
@@               Coverage Diff               @@
##                 main        #3173   +/-   ##
===============================================
  Coverage   100.00000%   100.00000%           
===============================================
  Files             124          124           
  Lines           18458        18458           
  Branches         1215         1215           
===============================================
  Hits            18458        18458           

@webknjaz
Copy link
Member

This would make Codecov less reliable. It'd be better to figure out the version bump.

@CoolCat467
Copy link
Member

While I agree that it's unfortunate that this would make Codecov less reliable, I think stopping pull requests from going through just because Codecov is buggy is a worse in the end.

@A5rocks
Copy link
Contributor Author

A5rocks commented Dec 26, 2024

This would make Codecov less reliable. It'd be better to figure out the version bump.

Does a new uploader really decrease the amount of 503s codecov returns? I note that the failures I checked were due to that, not due to some uploader issue.

@webknjaz
Copy link
Member

Yes, in my experience, v4 eliminated this problem. I think the retries are better, but also the API is new. I don't really have the stats for v5, though.

The only problem is that my attempt to bump to v5 has some weird coverage computation issue: #3161. I haven't really looked into that. But it's weird that I haven't seen this behavior elsewhere, so my hunch is that it might be related to the structure of the automation in this repo.

@A5rocks
Copy link
Contributor Author

A5rocks commented Dec 27, 2024

Ok should we merge this as a temporary patch (and a comment saying as much), until someone gets the time to look at v5?

I'm willing to wait a few days just in case we do it quickly but after that I think it's better to be able to get PRs merged on the first attempt. (PR statuses are fine because individual jobs can be rerun, but merge queue needs to rerun every job)

@jakkdl
Copy link
Member

jakkdl commented Dec 27, 2024

I think we should merge this as a temp fix due to how massively painful it is to work with.

It's def suboptimal, but /shrug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants