fix(pipeline): cache yarn deps with corepack #2405
Merged
+133
−20
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vscaiceanu-1a
force-pushed
the
bugfix/cache-yarn-deps
branch
from
b909057
to
3375620
Compare
fpaul-1A
previously approved these changes
Nov 5, 2024
| path: | | ||
| .yarn/cache | ||
| .yarn/unplugged | ||
| .pnp.cjs |
There was a problem hiding this comment.
what if yarn1 or yarn with node_modules and not pnp ?
kpanot
reviewed
Nov 5, 2024
...ages/@o3r/pipeline/schematics/ng-add/templates/github/__dot__github/actions/setup/action.yml
Show resolved
Hide resolved
vscaiceanu-1a
force-pushed
the
bugfix/cache-yarn-deps
branch
from
3375620
to
b8e09f7
Compare
kpanot
reviewed
Nov 6, 2024
vscaiceanu-1a
force-pushed
the
bugfix/cache-yarn-deps
branch
2 times, most recently
from
07ebae1
to
180663b
Compare
kpanot
reviewed
Nov 6, 2024
vscaiceanu-1a
force-pushed
the
bugfix/cache-yarn-deps
branch
from
180663b
to
029f331
Compare
kpanot
reviewed
Nov 6, 2024
| - name: Cache dependencies | ||
| uses: actions/cache@6849a6489940f00c2f30c0fb92c6274307ccb58a # v4.1.2 | ||
| with: | ||
| path: ${{ steps.yarn-cache-dir-path.outputs.dir }} |
There was a problem hiding this comment.
This is not missing the following files:
- .yarn/unplugged
- .pnp.cjs
- .pnp.loader.mjs
?
There was a problem hiding this comment.
not sure those are needed. I followed the recommendations from https://github.com/actions/cache/blob/main/examples.md#node---yarn
kpanot
reviewed
Nov 6, 2024
| @@ -41,7 +58,7 @@ function ngAddFn(options: NgAddSchematicsSchema): Rule { | |||
| if (!options.npmRegistry) { | |||
| return tree; | |||
| } | |||
| if (packageManager === 'yarn') { | |||
| if (yarn2) { | |||
| const yarnrcPath = '/.yarnrc.yml'; | |||
| if (!tree.exists(yarnrcPath)) { | |||
vscaiceanu-1a
force-pushed
the
bugfix/cache-yarn-deps
branch
2 times, most recently
from
eea1cd3
to
51f858e
Compare
| tree.overwrite(yarnrcPath, dump(yarnrcContent, {indent: 2})); | ||
| } | ||
| } else if (packageManager === 'npm') { | ||
| const yarnrcContent = load(tree.readText(yarnrcPath)) as any; |
There was a problem hiding this comment.
Suggested change
| const yarnrcContent = load(tree.readText(yarnrcPath)) as any; | |
| const yarnrcContent = load(tree.readText(yarnrcPath)) as { npmRegistryServer?: string }; |
Comment on lines
15
to
16
| const yarnrcContent = load(tree.readText(yarnrcPath)) as any; | ||
| const yarnPath = yarnrcContent?.yarnPath; |
There was a problem hiding this comment.
Suggested change
| const yarnrcContent = load(tree.readText(yarnrcPath)) as any; | |
| const yarnPath = yarnrcContent?.yarnPath; | |
| const { yarnPath } = (load(tree.readText(yarnrcPath)) || {}) as { yarnPath?: string }; |
| - name: Get yarn cache directory path | ||
| shell: bash | ||
| id: yarn-cache-dir-path | ||
| run: echo "dir=$(<% if (yarn2) { %>yarn config get cacheFolder<% } else { %>yarn cache dir<% } %>)" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Shouldn't we update our pipeline as well ?
There was a problem hiding this comment.
We could but with a feature request so that the setup action support npm, yarn 1, etc.
Definitely not in a fix on an RC branch.
There was a problem hiding this comment.
Can you open the request then please :)
vscaiceanu-1a
force-pushed
the
bugfix/cache-yarn-deps
branch
from
51f858e
to
64c749e
Compare
vscaiceanu-1a
force-pushed
the
bugfix/cache-yarn-deps
branch
from
64c749e
to
8be2a93
Compare
matthieu-crouzet
approved these changes
Nov 7, 2024
kpanot
approved these changes
Nov 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Proposed change
In
node-setupGitHub action, passingcache: yarndoesn't work well with corepack (see issues below).The goal is to do the cache manually for yarn and to let
node-setupdo it fornpm.Related issues
actions/setup-node#531
actions/setup-node#182