-
Notifications
You must be signed in to change notification settings - Fork 215
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
feat: fallback to tokenless on use_oidc if we can't find IDToken #1406
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1406 +/- ##
==========================================
+ Coverage 91.29% 91.31% +0.02%
==========================================
Files 4 4
Lines 310 311 +1
Branches 84 84
==========================================
+ Hits 283 284 +1
Misses 27 27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
`Codecov: Failed to get OIDC token with url: ${url}. ${err.message}`, | ||
true, | ||
`Codecov: Failed to get OIDC token with url, uploading using tokenless: ${url}. ${err.message}`, | ||
false, |
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 dont think we should silently fail this and go to tokenless. If anything, we should follow the failCi
set.
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.
Actually on second thought, I don't know why we don't fail if we don't find OIDC. This is an opt-in feature.
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.
this makes sense for public repos that use OIDC for CI that run on branches on that repo, but may also receive forks from external contributors that won't have an id token, and will need to use tokenless
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.
@joseph-sentry please see #1404
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.
oh, missed that, that works, I'll close this one then!
Fixes: codecov/engineering-team#1574
Related: codecov/codecov-api#533