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 pnpm caching support #278

Merged
merged 12 commits into from Jul 20, 2021
Merged

Add pnpm caching support #278

merged 12 commits into from Jul 20, 2021

Conversation

@jacobwgillespie
Copy link
Contributor

@jacobwgillespie jacobwgillespie commented Jun 30, 2021

This PR extends the excellent caching work at #272 with support for pnpm.

The global module store is stored at ~/.pnpm-store by default, unless otherwise configured. pnpm get store returns the custom store path, or the string undefined if no custom store is configured, so this PR adds a defaultCacheFolder option that is used as a fallback if the getCacheFolderCommand does not return a path. Note that the cache directory path is sent ultimately to @actions/glob, and that module handles the expansion of ~/ to the current home directory, so it's okay that the path is not expanded here.

Note: I've opened a PR on pnpm/pnpm to add a new command that returns the current module store directory, including the default if no custom path is set, but even if that PR is accepted, I believe we'd still need to support the defaultCacheFolder fallback here as current and past versions of pnpm would not support the new command.

I've added tests and Actions workflow jobs to match the existing npm and yarn 1 / 2 tests.

@maxim-lobanov
Copy link
Collaborator

@maxim-lobanov maxim-lobanov commented Jul 13, 2021

Hey @jacobwgillespie , thank you for your contribution, Your changes look awesome!
Please give us some time to review and validate it properly.

Did you have a chance to validate it on any real project? You can reference your fork branch via uses: jacobwgillespie/setup-node@cache-pnpm in your YAML file for e2e validation.

@jacobwgillespie
Copy link
Contributor Author

@jacobwgillespie jacobwgillespie commented Jul 13, 2021

@maxim-lobanov no problem, take your time, and let me know if there's anything I can do to help as you're reviewing!

I did validate it on some internal projects - if you'd like a public example, I just updated the styled-icons repo to use the forked action, you can see an example of it loading the pnpm cache at that link.

Copy link
Collaborator

@maxim-lobanov maxim-lobanov left a comment

Everything looks good to me. I have left minor comment to add additional validation.

I will ping more engineers to review it from our side and also we will do basic validation from our side.

src/cache-utils.ts Outdated Show resolved Hide resolved
__tests__/cache-save.test.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@maxim-lobanov
Copy link
Collaborator

@maxim-lobanov maxim-lobanov commented Jul 15, 2021

@bryanmacfarlane , could you please review and approve this PR when you have a chance? If you don't have objections - we can release new action version next week with these changes.

action.yml Outdated Show resolved Hide resolved
@maxim-lobanov
Copy link
Collaborator

@maxim-lobanov maxim-lobanov commented Jul 16, 2021

Hello @jacobwgillespie , just an update.
We have got all necessary approvals for this PR and finished with validation from our side.

We will be ready to cut new action release at the begin of next week.

@jacobwgillespie
Copy link
Contributor Author

@jacobwgillespie jacobwgillespie commented Jul 16, 2021

Awesome! 🎉 Thank you so much, excited to see this released!

@MaksimZhukov MaksimZhukov merged commit aa759c6 into actions:main Jul 20, 2021
94 checks passed
@MaksimZhukov
Copy link
Collaborator

@MaksimZhukov MaksimZhukov commented Jul 20, 2021

Hello @jacobwgillespie!
We have released actions/setup-node@v2.3.0 and updated the v2 tag. Thank you for the contribution!

Would you like to create a PR in the github/docs repository to update GitHub Docs in accordance with your changes?

@jacobwgillespie
Copy link
Contributor Author

@jacobwgillespie jacobwgillespie commented Jul 20, 2021

@MaksimZhukov perfect, thanks for the merge and release!

I've created a docs PR here: github/docs#8389

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants