This repository has been archived by the owner on Jan 25, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: add pnpm to build image #845
feat: add pnpm to build image #845
Changes from all commits
39279bc
9284f5e
650ad5b
bfeba2c
6cf6067
f95c85c
7a60201
bde926b
5aa1932
4b2ff31
4713c0c
37ef6ad
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
per default we install already yarn + pnpm with corepack
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.
if corepack is not available (then the user has switched to a different - older - node version and we have to install yarn with the old way
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.
Could be farteched but I'm thinking on the following scenario:
NODE_VERSION
,v18.x
for example (they have corepack).which corepack
will return a non-error exit codeyarn
binary won't exist soyarn --version
will blow up? π€Would this be possible? Should we check for the presence of the yarn executable before too? If so maybe we could move the
which yarn
assertion to the top and use that across the function via ayarn_exists
flag maybe?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.
That's not the case corepack installs yarn for each node version which is super awesome:
you can test it by:
which yarn
(should fail)corepack enable
corepack prepare yarn@1.22.19 --activate
which yarn
should worknvm install 18
(has corepack but no yarn installed)which yarn
(should fail)corepack enable
which yarn
should work π₯³I have added an extra test case testing this scenario :)
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.
Ah nvm, I was thinking about the scenario where your node installation was previously cached, however we always go through the
install_node
function which does thecorepack enable
step π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.
This is not used anymore:
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.
This fixes: #789
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.
moved that to it's own function to have it better testable