Skip to content
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

Add Option to Enable corepack #546

Closed
wants to merge 15 commits into from
Closed

Conversation

beeequeue
Copy link

@beeequeue beeequeue commented Jul 17, 2022

Description:
Adds the corepack option, which if true will run corepack enable to enable corepack.

You can see a test run of it here: https://github.com/BeeeQueue/arm-server/runs/7380286080?check_suite_focus=true

Related issue:
Closes #531
#480 - @dmitry-shibanov says that this change would not bring enough value to users, but I agree with @shellscape that being able to move most if not all of the node setup into one step is quite nice 🙂
#482 - That PR adds automatic support for it which could potentially be breaking, this PR adds an option which should not break anything

Check list:

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

@beeequeue beeequeue requested a review from a team July 17, 2022 21:16
@beeequeue beeequeue changed the title Add Option to Automatically Enable corepack Add Option to Enable corepack Jul 17, 2022
@beeequeue
Copy link
Author

I can't figure out why the test is failing based on the other tests... 😕

@wuzzeb
Copy link

wuzzeb commented Jul 22, 2022

Is it possible to completely remove pnpm/action-setup with this? What I mean is

- uses: actions/node-setup@v3
  with:
    node-version: "18"
    corepack: true
    cache: "pnpm"

would be enough with no other actions so no pnpm installed at all before node-setup? I would think so because node is installed, corepack enable is run, and then after that the caching is setup. Thus the first time node-setup calls pnpm for the caching, corepack should install pnpm.

If so, I think the documentation in advanced-usage.md should be updated, perhaps add a section to the caching showing just this configuration without pnpm/action-setup.

__tests__/installer.test.ts Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
docs/advanced-usage.md Outdated Show resolved Hide resolved
@yarinsa
Copy link

yarinsa commented Sep 1, 2022

@beeequeue Thank you for this contribution! When do you think this will get merged? If so?

Copy link

@yarinsa yarinsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@RavenK1ll
Copy link

I also have a 32G dcim that i user at beginning of build. Would that he of user?

@RavenK1ll
Copy link

Should i switch dcim cards?

@beeequeue
Copy link
Author

Presumably after the test starts passing, and I can't get it to on my Windows machine 😢

@RavenK1ll
Copy link

So what an I looking

@RavenK1ll
Copy link

So what an I looking at here?

src/main.ts Outdated Show resolved Hide resolved
@jtbandes
Copy link

@dsame @brcrista Since #531 was rejected, but it's clear that many people are interested in this feature, could you please provide some feedback on this PR and/or #482 which are two proposals to add corepack support? Currently it seems the PRs have received no feedback from maintainers.

docs/advanced-usage.md Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
Copy link

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@dsame dsame self-assigned this Oct 20, 2022
@yarinsa
Copy link

yarinsa commented Oct 30, 2022

@beeequeue What's your ETA on merging this?

@brcrista
Copy link
Contributor

@jtbandes it's not rejected; I just said it's not a priority yet: #531 (comment)

It looks like some of the tests were added are failing. @beeequeue I don't know a lot about this domain, but are you sure corepack is installed on the hosted runner?

When I have an issue with an Actions workflow I can't reproduce locally, I usually end up having to add some console.log calls to find the problem.

@beeequeue
Copy link
Author

beeequeue commented Oct 31, 2022

None of the test errors seem to be because of the PR, and would probably pass if they run again which I can't do

I manged to reproduce it, but got no idea why it's happening

I'm not very motivated to try to figure out why the tests fail after the rebase, nothing is wrong and it works perfectly so I'm just gonna use my branch in my projects.

@beeequeue
Copy link
Author

beeequeue commented Jan 16, 2023

Superseded by #651

@beeequeue beeequeue closed this Jan 16, 2023
deining pushed a commit to deining/setup-node that referenced this pull request Nov 9, 2023
…ctions#546)

Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 6.5.0 to 6.6.0.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v6.6.0/packages/parser)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/parser"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Corepack Support
8 participants