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

use the latest pnpm/action-setup and don't define pnpm version if packageManager is defined #127

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

mansona
Copy link
Member

@mansona mansona commented Jul 17, 2024

No description provided.

@mansona mansona force-pushed the use-latest-pnpm-action branch from 6db6f57 to 7ed32cc Compare July 17, 2024 13:10
@mansona mansona added the enhancement New feature or request label Jul 17, 2024
@mansona mansona requested a review from NullVoxPopuli July 17, 2024 13:11
<% if (pnpm) { %>
- uses: pnpm/action-setup@v3
node-version: 18<% if (pnpm) { %>
- uses: pnpm/action-setup@v4<% if (!hasPackageManager) { %>
with:
Copy link
Contributor

Choose a reason for hiding this comment

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

why specify the version at all? the duplication in workflows is maddening.

This is what package.json#packageManager is for

Copy link
Contributor

Choose a reason for hiding this comment

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

or wait, I can't read ejs.
this does exactly what I expect.

Copy link
Member Author

Choose a reason for hiding this comment

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

look a bit more carefully at the change, it only adds the version if you haven't set packageManager. And version is a required parameter if you haven't got packageManager so the action will break without it

I agree that packageManager should be used by most people at this stage but we can't impose it as part of our workflow. And people who do define it will benefit from it 💪

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we crossed paths
image

Copy link
Member Author

Choose a reason for hiding this comment

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

omg Github 😭 this is why eventual consistency doesn't always work. it didn't even update the thread after I posted my response

@mansona mansona merged commit 9280da2 into main Jul 17, 2024
4 checks passed
@mansona mansona deleted the use-latest-pnpm-action branch July 17, 2024 13:26
@github-actions github-actions bot mentioned this pull request Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants