-
-
Notifications
You must be signed in to change notification settings - Fork 103
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
feat: Support volta
, asdf
, and package.json engines
for declaring version
#151
Conversation
Using the same logic as `actions/setup-node`, we can support `asdf`, `volta` or other version management techniques.
volta
, asdf
, and package.json engines
volta
, asdf
, and package.json engines
for declaring version
version_file_path: | ||
description: "Path to a version file. Eg: '.tool-versions' for asdf or 'package.json' for volta" | ||
required: false |
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 problematic. .tool-versions
and package.json
are completely different formats and should be treated as different features.
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.
Honestly, the only reason I did it this way was to match up with how they do it for node versioning in the referenced lib. I figure if js devs are familiar with any GitHub actions at all, it’s likely that one.
I’ll rethink the inputs throwing out that goal and come back with something else.
Or not. Heh. If I have the time I will.
@zkochan Normally, I only want @happycollision Shouldn't |
I doubt it? I assume the best actions to use for setup are the incredibly common one provided by the GH Actions team for node and then for pnpm, the one maintained by the actual developers of pnpm. 😇 I use a node action to setup node and this pnpm action to set up pnpm in CI (just as described in your docs). But to version control in development I use asdf, sometimes Volta. All I really want is a single, canonical place to declare a version and have it respected by all those tools, rather than assume everyone on my team is familiar enough with all of them to know and remember to update the version in multiple places. So that’s my particular use case. Now, the .tool-versions format is easy enough to dig through with shell scripting, but if you give people an easy way to do it, you’d save people some time and effort. This, I assume, is why the node action offers to read these common version declaration files. I gave my other answers above before reading this comment, so until I hear back from you guys, I’ll sit on this so you can decide what is best for the project. Feel free to close this out if you like. Cheers. Side note: We are in the middle of transitioning from npm to pnpm at my workplace. I am the one doing the work for it, and I am loving the benefits I am seeing. Thanks for the incredible work on this package manager. |
@happycollision Both pnpm initially also didn't have a GH action, so I made one, and proposed with zkochan to move it from my account to the @pnpm org, making it official. |
I can see where Not that my doubting it makes it less "their problem". What would you say to just adding some docs that show the very workaround I am using for
update Yeah, volta has their own: https://github.com/volta-cli/action So I would bet that when |
Creating a GH action for Corepack has |
I think it is OK to read the setting from those files. Those are just tools used during local development. |
36bc808
to
f1b72a0
Compare
Added more logging
f1b72a0
to
05cbfb6
Compare
Considering @ksgithub's concerns versus @zkochan's general approval, I added two commits to fixup into the main one, which throw errors and log a touch more info so that if something is going wrong, the user has a shot at seeing what it is. So yes, we are still treating |
|
||
// Try parsing the file as a `package.json` file. | ||
try { | ||
const manifest = JSON.parse(contents) |
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.
The behavior would be more predictable if you match the extension of versionFilePath
against .json
and .tool-versions
instead of try
. We can even be stricter: match the basename of versionFilePath
against package.json
and .tool-versions
.
if (manifest.volta?.pnpm) { | ||
return manifest.volta.pnpm as string | ||
} | ||
|
||
if (manifest.engines?.pnpm) { | ||
return manifest.engines.pnpm as string | ||
} |
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 (manifest.volta?.pnpm) { | |
return manifest.volta.pnpm as string | |
} | |
if (manifest.engines?.pnpm) { | |
return manifest.engines.pnpm as string | |
} | |
if (typeof manifest.volta?.pnpm === 'string') { | |
return manifest.volta.pnpm | |
} | |
if (typeof manifest.engines?.pnpm === 'string') { | |
return manifest.engines.pnpm | |
} |
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.
Also, what if both manifest.engines.volta.pnpm
and manifest.engines.pnpm
are specified but different from each other? Shouldn't it throw an error?
const matched = contents.match(/^(?:pnpm\s+)?v?(?<version>[^\s]+)$/m) | ||
const found = matched?.groups?.version ?? contents.trim() |
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.
Regex is overkill here.
The format is simple enough. Just use .split('\n')
, .find()
, .startsWith('pnpm')
, and .slice('pnpm'.length).trim()
.
The main reason I opened this PR was just to get a quick win by using almost the exact same implementation that the nodejs action uses for node version management. It seems to me that the kind of solution they use is not to your liking, which is understandable. I don’t really have the time to continue this back and forth at this time, so I’ll close the PR and open a new one if I feel I have the time and desire again to tackle this in a way you prefer in the future. |
Guys,
Although would love to review your PRs, you've got the wrong guy in your
email chain. Please remove me from this chain.
Thanks
…On Tue, Jan 21, 2025 at 8:29 AM Don Denton ***@***.***> wrote:
The main reason I opened this PR was just to get a quick win by using
almost the exact same implementation that the nodejs action uses for node
version management.
It seems to me that the kind of solution they use is not to your liking,
which is understandable.
I don’t really have the time to continue this back and forth at this time,
so I’ll close the PR and open a new one if I feel I have the time and
desire again to tackle this in a way you prefer in the future.
—
Reply to this email directly, view it on GitHub
<#151 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APJCKIWUFQCPP5HEDB3GVED2LZDSDAVCNFSM6AAAAABSUIT6PCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMBUG4ZTSNRSHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Khaled Saad
|
build
script. Not sure if there is anything else needed.This addresses #109 and #84 and could be extended later to also address #150.
I thought it'd be nice to allow something like this: