feat: pass tokenless value as branch override#1511
feat: pass tokenless value as branch override#1511joseph-sentry merged 11 commits intocodecov:mainfrom joseph-sentry:joseph/fix-tokenless
Conversation
| core.info('==> Fork detected, tokenless uploading used'); | ||
| process.env['TOKENLESS'] = context.payload.pull_request.head.label; | ||
| return Promise.resolve(''); | ||
| return [false, context.payload.pull_request?.head.label]; |
There was a problem hiding this comment.
can we do let someVar = true and then set it to false here? it will make it more obvious what is being returned
There was a problem hiding this comment.
Also, I believe this needs to be wrapped as a Promise still
| const token = await getToken(); | ||
| const [tokenAvailable, token] = await getToken(); | ||
| if (!tokenAvailable) { | ||
| overrideBranch = token; |
There was a problem hiding this comment.
I don't agree with this, we should set overrideBranch = context.payload.pull_request?.head.label here instead. We run the risk of overloading our vars otherwise
instead of only passing the tokenless branch value as an environment variable we want to pass it as the branch value to the CLI
| core.info('==> Fork detected, tokenless uploading used'); | ||
| process.env['TOKENLESS'] = context.payload.pull_request.head.label; | ||
| return Promise.resolve(''); | ||
| return null; |
There was a problem hiding this comment.
please write a test for this case
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1511 +/- ##
==========================================
+ Coverage 89.84% 93.67% +3.82%
==========================================
Files 5 5
Lines 325 332 +7
Branches 78 78
==========================================
+ Hits 292 311 +19
+ Misses 33 19 -14
- Partials 0 2 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| }; | ||
|
|
||
| const getToken = async (): Promise<string> => { | ||
| const getToken = async (): Promise<string | null> => { |
There was a problem hiding this comment.
I think a comment on what to expect from null is needed here. Also, you are returning a Promise to null here but further down you are returning null as is
matt-codecov
left a comment
matt-codecov
left a comment
There was a problem hiding this comment.
is index.js codegenned by the typescript compiler? looks like we could add dist/index.js linguist-generated=true to .gitattributes to hide it from review. will it be automatically regenerated or is there a risk of updating the TS but not the index.js?
| const token = await getToken(); | ||
| if (!overrideBranch && token == null) { | ||
| overrideBranch = context.payload.pull_request?.head.label; |
There was a problem hiding this comment.
i think we should move the "are we a fork" stuff out of getToken() and into a new getOverrideBranch() function. whether we're a fork doesn't really factor into what the token is, just whether we want to set an override branch. but since the fork check happens inside of getToken(), we have to smuggle the result out of getToken() by saying null means "fork" and "" means "no fork" so that the override branch code can decide what to do. we should probably move the fork check to where it's really needed instead
const getOverrideBranch = async (token: string, context: Whatever): Promise<string | null> => {
const overrideBranch = core.getInput("override_branch");
if (!overrideBranch && !token && isPullRequestFromFork()) {
return Promise.resolve(context.payload.pull_request?.head.label);
} else {
return Promise.resolve(overrideBranch);
}
}
const token = await getToken();
const overrideBranch = getOverrideBranch(token, context);
with this change, getToken() really just gets the token, and getOverrideBranch() contains all of the logic about what the override branch should be, including the fork check
tnt-sentry
dismissed
thomasrockhu-codecov’s stale review
Changes addressed and Tom is OOO
3 participants
instead of only passing the tokenless branch value as an environment variable we want to pass it as the branch value to the CLI