-
Notifications
You must be signed in to change notification settings - Fork 199
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: pass tokenless value as branch override #1511
base: main
Are you sure you want to change the base?
Conversation
src/buildExec.ts
Outdated
let token = core.getInput('token'); | ||
if (!token && isPullRequestFromFork()) { | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I believe this needs to be wrapped as a Promise still
src/buildExec.ts
Outdated
const token = await getToken(); | ||
const [tokenAvailable, token] = await getToken(); | ||
if (!tokenAvailable) { | ||
overrideBranch = token; |
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 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
a3454c1
to
30c7aa0
Compare
src/buildExec.ts
Outdated
let token = core.getInput('token'); | ||
if (!token && isPullRequestFromFork()) { | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please write a test for this case
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1511 +/- ##
==========================================
+ Coverage 91.66% 93.27% +1.60%
==========================================
Files 5 5
Lines 324 327 +3
Branches 86 87 +1
==========================================
+ Hits 297 305 +8
+ Misses 25 22 -3
+ Partials 2 0 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -45,12 +45,12 @@ const isPullRequestFromFork = (): boolean => { | |||
return (baseLabel.split(':')[0] !== headLabel.split(':')[0]); | |||
}; | |||
|
|||
const getToken = async (): Promise<string> => { | |||
const getToken = async (): Promise<string | null> => { |
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 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
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.
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?
@@ -42,7 +42,7 @@ jobs: | |||
file: ./coverage/coverage-final.json | |||
flags: version,${{ matrix.os }} | |||
name: codecov-version | |||
version: v0.6.0 | |||
version: v0.7.3 |
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 see below that you're leaving the env var in for now for backcompat which is a good call. i think we will still want to push a CLI release with codecov/codecov-cli#475 and update this to v0.7.4. if someone wants to use tokenless from a non-fork, they should be able to use the regular override_branch
action parameter and they'll need 0.7.4 for that to work
const token = await getToken(); | ||
if (!overrideBranch && token == null) { | ||
overrideBranch = context.payload.pull_request?.head.label; |
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 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
instead of only passing the tokenless branch value as an environment variable we want to pass it as the branch value to the CLI