Option to enable corepack #651

SayakMukhopadhyay · 2022-12-26T17:17:38Z

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:

Mark if documentation changes are required.
Mark if tests were added or updated to cover the changes.

BeeeQueue

Looks good to me, no bad changes from my branch 😎

markandrus · 2022-12-29T15:05:47Z

This worked perfectly on my self-hosted runner 👍 Thanks!

action.yml

docs/advanced-usage.md

src/installer.ts

Co-authored-by: Steven <steven@ceriously.com>

…k anyway

SayakMukhopadhyay · 2023-01-17T09:21:39Z

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?

etherscanio30012023220200 · 2023-02-01T02:02:07Z

https://github.com/microsoft/vscode-dev-containers/blob/main/containers/javascript-node/.devcontainer/base.Dockerfile

amacneil · 2023-03-28T06:32:40Z

👍 this would be very helpful

yarinsa · 2023-04-04T07:01:08Z

Would be amazing if that was merged :)

SayakMukhopadhyay · 2023-04-04T07:04:41Z

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.

yarinsa · 2023-04-04T07:11:53Z

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?

neolectron · 2023-08-01T12:26:03Z

works like a charm on my self-host, thanks !

styfle

Looks great, thanks!

SayakMukhopadhyay · 2023-08-01T16:25:18Z

@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?

styfle · 2023-08-01T16:37:44Z

@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.

SayakMukhopadhyay · 2023-08-02T06:35:30Z

@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.

brianespinosa · 2023-08-26T00:16:13Z

@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.

neolectron · 2023-09-13T21:12:01Z

Thanks @brianespinosa , if in the end it turns out you have less time that planned, I would eventually like to help. Kind regards.

brianespinosa · 2023-09-26T20:24:10Z

@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!

neolectron · 2023-09-26T21:18:51Z

Ok thank you, I will make a new PR and take a shot at this.

SayakMukhopadhyay · 2023-09-27T09:53:07Z

@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.

JP250552 · 2023-11-21T19:56:01Z

New PR updated with main branch - #901

anonrig · 2023-12-20T01:39:41Z

Is there a blocker for this PR?

dsame · 2023-12-20T08:11:22Z

src/installer.ts

+
+export async function enableCorepack(input: string): Promise<void> {
+  let corepackArgs = ['enable'];
+  if (input.length > 0 && input !== 'false') {


It is enough to check if (input.length && input !== 'false')

dsame · 2023-12-20T08:12:11Z

src/installer.ts

@@ -604,3 +605,16 @@ export function parseNodeVersionFile(contents: string): string {
 function isLatestSyntax(versionSpec): boolean {
  return ['current', 'latest', 'node'].includes(versionSpec);
 }
+
+export async function enableCorepack(input: string): Promise<void> {
+  let corepackArgs = ['enable'];


It is unnecessary to initialize this variable before the if condition.

dsame · 2023-12-20T08:14:54Z

src/installer.ts

+      const packageManagers = input.split(' ');
+      corepackArgs.push(...packageManagers);
+    }
+    await exec.getExecOutput('corepack', corepackArgs, {


please, use getCommandOutput or getCommandOutputNotEmpty instead of directly calling exec.getExecOutput

dsame · 2023-12-20T08:16:43Z

src/main.ts

@@ -49,6 +49,9 @@ export async function run() {
      auth.configAuthentication(registryUrl, alwaysAuth);
    }

+    const corepack = core.getInput('corepack') || 'false';


In order the input to be recognized it must be defined in action.yml

dsame · 2023-12-20T08:20:19Z

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.

JP250552 · 2024-02-06T15:48:48Z

@dsame Thank you for the review.

This PR has been abandoned and I have opened a newer one here - #901

I have applied the changes you requested in this PR. Would you be able to take a look?

SayakMukhopadhyay added 2 commits December 26, 2022 22:29

tests: add test to check for corepack enable call

b6efa7f

feat: add logic to enable corepack on passing optional input

53234c2

SayakMukhopadhyay requested a review from a team as a code owner December 26, 2022 17:17

SayakMukhopadhyay added 2 commits December 26, 2022 22:50

feat: build distributable

41bb914

docs: add docs

872987f

BeeeQueue approved these changes Dec 26, 2022

View reviewed changes

joequincy mentioned this pull request Jan 13, 2023

Corepack Support #531

Open

BeeeQueue mentioned this pull request Jan 16, 2023

Add Option to Enable corepack #546

Closed

2 tasks

styfle reviewed Jan 16, 2023

View reviewed changes

action.yml Outdated Show resolved Hide resolved

styfle reviewed Jan 16, 2023

View reviewed changes

docs/advanced-usage.md Outdated Show resolved Hide resolved

styfle reviewed Jan 16, 2023

View reviewed changes

docs/advanced-usage.md Outdated Show resolved Hide resolved

styfle reviewed Jan 16, 2023

View reviewed changes

src/installer.ts Outdated Show resolved Hide resolved

fix: apply suggestions from code review

c7d2f5b

Co-authored-by: Steven <steven@ceriously.com>

SayakMukhopadhyay requested a review from a team as a code owner January 17, 2023 08:44

feat: remove package manager name check as it will be done by corepac…

0565a4f

…k anyway

dubzzz mentioned this pull request Feb 8, 2023

👷 Fully move towards corepack dubzzz/fast-check#3265

Closed

12 tasks

styfle mentioned this pull request Apr 5, 2023

Add automatic corepack support for node v14 + #482

Open

2 tasks

tmm mentioned this pull request Apr 19, 2023

chore: add packageManager field wevm/viem#389

Merged

styfle approved these changes Aug 1, 2023

View reviewed changes

lmilbaum mentioned this pull request Sep 6, 2023

feat: run GHA build job locally with act renovatebot/renovate#23959

Closed

6 tasks

tusbar approved these changes Oct 23, 2023

View reviewed changes

trivikr mentioned this pull request Oct 24, 2023

Project Status (experimental to stable?) nodejs/corepack#104

Open

cesar18pena approved these changes Nov 21, 2023

View reviewed changes

JP250552 mentioned this pull request Nov 21, 2023

Added option to enable corepack #901

Open

2 tasks

ghiscoding mentioned this pull request Nov 27, 2023

fix(browser): allow for pretty-format as a sibling dependency vitest-dev/vitest#4590

Merged

6 tasks

dsame suggested changes Dec 20, 2023

View reviewed changes

dsame self-assigned this Dec 20, 2023

Heidi1995 approved these changes Dec 30, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option to enable corepack #651

Option to enable corepack #651

SayakMukhopadhyay commented Dec 26, 2022

BeeeQueue left a comment

markandrus commented Dec 29, 2022

SayakMukhopadhyay commented Jan 17, 2023 •

edited

etherscanio30012023220200 commented Feb 1, 2023

amacneil commented Mar 28, 2023

yarinsa commented Apr 4, 2023

SayakMukhopadhyay commented Apr 4, 2023

yarinsa commented Apr 4, 2023

neolectron commented Aug 1, 2023

styfle left a comment

SayakMukhopadhyay commented Aug 1, 2023

styfle commented Aug 1, 2023 •

edited

SayakMukhopadhyay commented Aug 2, 2023

brianespinosa commented Aug 26, 2023

neolectron commented Sep 13, 2023

brianespinosa commented Sep 26, 2023

neolectron commented Sep 26, 2023

SayakMukhopadhyay commented Sep 27, 2023

JP250552 commented Nov 21, 2023

anonrig commented Dec 20, 2023

dsame Dec 20, 2023

dsame Dec 20, 2023

dsame Dec 20, 2023

dsame Dec 20, 2023

dsame commented Dec 20, 2023

JP250552 commented Feb 6, 2024

Option to enable corepack #651

Are you sure you want to change the base?

Option to enable corepack #651

Conversation

SayakMukhopadhyay commented Dec 26, 2022

BeeeQueue left a comment

Choose a reason for hiding this comment

markandrus commented Dec 29, 2022

SayakMukhopadhyay commented Jan 17, 2023 • edited

etherscanio30012023220200 commented Feb 1, 2023

amacneil commented Mar 28, 2023

yarinsa commented Apr 4, 2023

SayakMukhopadhyay commented Apr 4, 2023

yarinsa commented Apr 4, 2023

neolectron commented Aug 1, 2023

styfle left a comment

Choose a reason for hiding this comment

SayakMukhopadhyay commented Aug 1, 2023

styfle commented Aug 1, 2023 • edited

SayakMukhopadhyay commented Aug 2, 2023

brianespinosa commented Aug 26, 2023

neolectron commented Sep 13, 2023

brianespinosa commented Sep 26, 2023

neolectron commented Sep 26, 2023

SayakMukhopadhyay commented Sep 27, 2023

JP250552 commented Nov 21, 2023

anonrig commented Dec 20, 2023

dsame Dec 20, 2023

Choose a reason for hiding this comment

dsame Dec 20, 2023

Choose a reason for hiding this comment

dsame Dec 20, 2023

Choose a reason for hiding this comment

dsame Dec 20, 2023

Choose a reason for hiding this comment

dsame commented Dec 20, 2023

JP250552 commented Feb 6, 2024

SayakMukhopadhyay commented Jan 17, 2023 •

edited

styfle commented Aug 1, 2023 •

edited