Conversation
beeequeue
left a comment
beeequeue
left a comment
There was a problem hiding this comment.
Looks good to me, no bad changes from my branch 😎
|
This worked perfectly on my self-hosted runner 👍 Thanks! |
Co-authored-by: Steven <steven@ceriously.com>
|
Seems like there are merge conflicts so I will rebase. Hopefully this gets accepted soon. EDIT: wait, the code has changed significantly. The |
|
👍 this would be very helpful |
|
Would be amazing if that was merged :) |
|
Seems like I will need to rewrite this pr since the codebase has changed significantly. Since no maintainer is responding, I don't know if I should even rework my code. |
Maybe @DanRigby from the team can help us? |
|
works like a charm on my self-host, thanks ! |
|
@styfle the PR has merge conflicts since setup-node had an update. Will you be able to merge the PR if I fix these issues? |
|
@SayakMukhopadhyay I don't have permission. But the merge conflicts will need to be fixed before anyone can merge. I also posted in #531 to get more feedback. |
|
@styfle I don't want to put the effort for rewriting the PR unless I get some confirmation from the maintainers that they intend to accept this. |
|
@SayakMukhopadhyay I may have some time this weekend to go through and rework your solution to match how the code has been restructured. Since you already have some good tests and a reasonable API, I plan on forking your branch and doing the work from there. I can submit a PR back to your branch if you'd like so we can leave this PR open. |
|
Thanks @brianespinosa , if in the end it turns out you have less time that planned, I would eventually like to help. Kind regards. |
|
@neolectron my concern here is that I have not heard from the initial author of this PR, so I am not sure forking the fork and submitting a PR back to the fork is the right path as that means we are waiting on action from the initial author, plus hoping the core maintainers actually decide to to anything with this (they have been silent so far). I suspect nothing will ever happen with this as I have seen multiple folks try to get a thumbs up or down from a maintainer and nothing... so I think I would be wasting my time. If you wanted to take a shot at this, I would fork this branch that has great testing from @SayakMukhopadhyay and use that testing to validate the refactor, then submit a new PR. Good luck! |
|
Ok thank you, I will make a new PR and take a shot at this. |
|
@brianespinosa @neolectron I would be happy for people to upgrade my PR to work with the latest codebase. Unfortunately I don't have the bandwidth right now to work on this. I might get some time on my hands in a couple of weeks and if nothing comes off till then, I will look into updating. |
|
New PR updated with main branch - #901 |
|
Is there a blocker for this PR? |
|
|
||
| export async function enableCorepack(input: string): Promise<void> { | ||
| let corepackArgs = ['enable']; | ||
| if (input.length > 0 && input !== 'false') { |
There was a problem hiding this comment.
It is enough to check if (input.length && input !== 'false')
| } | ||
|
|
||
| export async function enableCorepack(input: string): Promise<void> { | ||
| let corepackArgs = ['enable']; |
There was a problem hiding this comment.
It is unnecessary to initialize this variable before the if condition.
| const packageManagers = input.split(' '); | ||
| corepackArgs.push(...packageManagers); | ||
| } | ||
| await exec.getExecOutput('corepack', corepackArgs, { |
There was a problem hiding this comment.
please, use getCommandOutput or getCommandOutputNotEmpty instead of directly calling exec.getExecOutput
| auth.configAuthentication(registryUrl, alwaysAuth); | ||
| } | ||
|
|
||
| const corepack = core.getInput('corepack') || 'false'; |
There was a problem hiding this comment.
In order the input to be recognized it must be defined in action.yml
|
Hi @SayakMukhopadhyay, I've reviewed the PR but i am not ready to approve it, mainly because of it lacks e2e tests. Moreover, i added few changes requests which must be resolved prior to further actions. |
15 participants
Description:
Adds the
corepackoption, which iftruewill runcorepack enableto enable corepack. This option is by defaultfalsewhich ensures that there is no breaking change. Moreover, we can use a string of package manager names in place oftruewhich will indicate to runcorepack enable <string of package manager names>.Related issue:
Closes #531
Check list: