setup-node
setup-node copied to clipboard
actions
Option to enable corepack
Description:
Adds the corepack option, which if true will run corepack enable to enable corepack. This option is by default false which ensures that there is no breaking change. Moreover, we can use a string of package manager names in place of true which will indicate to run corepack enable <string of package manager names>.
Related issue: Closes #531
Check list:
- [x] Mark if documentation changes are required.
- [x] Mark if tests were added or updated to cover the changes.
Seems like there are merge conflicts so I will rebase. Hopefully this gets accepted soon.
EDIT: wait, the code has changed significantly. The installer.ts has been removed. Ugh, I will need to rewrite this huh?
https://github.com/microsoft/vscode-dev-containers/blob/main/containers/javascript-node/.devcontainer/base.Dockerfile
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.
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?
@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 https://github.com/actions/setup-node/issues/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!
@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 - https://github.com/actions/setup-node/pull/901
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.
@dsame Thank you for the review.
This PR has been abandoned and I have opened a newer one here - https://github.com/actions/setup-node/pull/901
I have applied the changes you requested in this PR. Would you be able to take a look?