-
Notifications
You must be signed in to change notification settings - Fork 9
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
Codecov fails, action passes? #266
Comments
Oh god... 🤦 For real? Anyways combined with codecov/codecov-action#1242 that is pretty dumb, especially considering dependabot/renovate coming by to upgrade a lot of external repos that just will look fine... Closing it, as well, it is documented behavior... |
Actually, I've decided to re-open this, as I think this is debatable. From the docs: The part that makes me think this is an actual issue, is:
However, in the above case (and the one laid out in codecov/codecov-action#1242), it isn't an upload issue at all. It never got to that part. In my opinion, there needs to be a difference between a program error that isn't recoverable and an error during the upload of test coverage. The latter case, yeah, I could see why someone would not want to fail on that (not debating the default on this one, as I still think it is a dumb default, but... not the point). In the case here, this is a non-recoverable program error that should have made the action fail regardless of this setting. In conclusion, I think the CLI needs to throw different exit codes that can be handled accordingly based on that setting. This would allow this specific case in the screenshot of the OP to actually fail, regardless of the |
The action should indeed fail if a token is not present, for example. This is also semantically different from
This. This is bad. |
Interestingly, I'm seeing something similar with a |
The problem with failing when codecov upload fails is that I've seen CodeCov's infrastructure not be very stable. It's frustrating to have CI blocked because CodeCov is having issues. At least, that's what I encountered when I last tried having this option on. Checking coverage is important but not critical enough to block everything. |
@bmulholland Agree, the point here is: It also just passes on a critical error (e.g., configuration errors). That has nothing to do with uploading at all. |
Oh yeah, good point, agree. |
@rohan-at-sentry, we should take a look into this |
I could not find my issue when I wanted to look up the status. Turned out it was moved to feedback? I'm not sure why this issue was moved to feedback. This ain't feedback, this is an error case, a behavior that doesn't work as documented and thus: unexpected. Such things should be fixed. Either this bug should be fixed in code, or expected behavior and in that case the documentation should be adjusted. Please advise on what to do now. Can you move the issue back? Or should I create a new issue in the codecov-upload action repository where this reported issue applies to? ../Frenck |
@frenck - this behavior is already a part of Codecov (we'll investigate how to improve this) , so it's likely better suited to triage within our |
Ok, closing this for now in that case. ../Frenck |
For more context, here's the rationale for existing behavior. We do not want to fail builds due to issues related with uploading to Codecov. For example, consider a test run that takes about 20 min, then the run fails because Codecov had a hiccup. similar to what was raised here --> #266 (comment) That would be suboptimal. |
Yes, that was already stated above... That is also not what this was about. That comment, made me realize closing this issue is the best path forward. ../Frenck |
@frenck A few issues may have gotten confused here. In your case, was |
This action clearly should be failing, considering the output:
However, as you can see, it passes...
All that is in the action:
The text was updated successfully, but these errors were encountered: