feat(setup-pack): add pack-version-file input and verify download checksum#428
feat(setup-pack): add pack-version-file input and verify download checksum#428somaz94 wants to merge 2 commits into
Conversation
…cksum Signed-off-by: somaz <genius5711@gmail.com>
|
This sounds great! A couple of notes/questions:
This sounds backwards to me. As I read this, my understanding is that
https://github.com/actions/setup-go/blob/main/docs/advanced-usage.md#using-the-go-version-file-input
|
|
Thanks for the review! On the checksum (2): it already covers both paths. The download and sha256 verification are in the shared "Install pack CLI" step that runs after version resolution, so the same archive is fetched and checked the same way whether the version came from pack-version or pack-version-file. On precedence (1): you're right that inline-wins is the usual convention. I had the file win because pack-version carries a default (0.40.6, kept current by the update-pack-version workflow's sed on the default: line). Unlike setup-go's go-version, pack-version is never empty, so a composite action can't distinguish "user set 0.40.6" from "defaulted", and inline-always-wins would make pack-version-file unreachable. If you're open to it I can match setup-go: drop the input default, resolve pack-version then pack-version-file then a fallback baked into the resolve step, and point the update-pack-version sed at that fallback line so the auto-bump still works. Or I keep file-as-override and just document it. Which do you prefer? |
jjbustamante
left a comment
jjbustamante
left a comment
There was a problem hiding this comment.
@somaz94 , @dmikusa . @edmorley
The checksum validation in this PR is valuable and backward-compatible on its own, so I'd suggest we land it and handle the pack-version vs pack-version-file precedence question as a follow-up? For reference, here's the trade-off we'd revisit:
| Scenario | PR as-is (file wins) | setup-go semantics (inline wins) |
|---|---|---|
| User sets nothing | default 0.40.6 |
needs a fallback baked into the resolve step |
User sets pack-version only |
that version | that version |
User sets pack-version-file only |
file version | file version |
| User sets both | file wins (non-standard) | inline wins (standard) |
update-pack-version auto-bump |
keeps working (default line untouched) | breaks unless the sed is re-pointed |
| Convention match | surprising | familiar |
Based on that, I agree with @somaz94 approach to drop the default and add the fallback but we need to be careful with the automation for update-pack-version we have in place right now.
…sion-file Signed-off-by: somaz <genius5711@gmail.com>
|
Thanks both. I went with the inline-wins redesign you endorsed rather than splitting, since it's contained to this PR:
Behavior stays backward compatible: with nothing set, the fallback equals the previous default. Validated locally — precedence (both / file-only / neither / leading- |
edmorley
left a comment
edmorley
left a comment
There was a problem hiding this comment.
Thank you for the PR! Couple of comments but other than that looks good to go :-)
| file like .pack-version (the first non-comment line is read). Used only | ||
| when pack-version is not set. | ||
| required: false | ||
| default: '0.40.6' |
There was a problem hiding this comment.
Could you update the PR description to reflect the decision to now remove the default input?
It currently say things like:
The existing pack-version default line is left untouched so the update-pack-version automation keeps working.
Both changes are backward compatible: with neither input changed
(it's now a breaking change, which is fine, but will mean a major version bump so best to state that in the PR description :-) )
| # Fallback pack version, used only when neither pack-version nor | ||
| # pack-version-file is set. Kept current by the update-pack-version | ||
| # workflow (which rewrites the line below). | ||
| FALLBACK_PACK_VERSION='0.40.6' |
There was a problem hiding this comment.
(optional) I wonder if the fallback pack version would be best set as an env var under the env: block next to PACK_VERSION etc, so they are all in one place (and this constant isn't hidden within the inline script)? If so, then the sed in .github/workflows/update-pack-version.yml will also need updating.
| ;; | ||
| *) | ||
| # Plain version file: first non-empty, non-comment line | ||
| version="$(grep -vE '^[[:space:]]*(#|$)' "${PACK_VERSION_FILE}" | head -n1 | tr -d '[:space:]')" |
There was a problem hiding this comment.
Claude says:
- Plain-file parser doesn't strip inline trailing comments (action.yml:57):
0.40.6 # pinned→ tr -d collapses to0.40.6#pinned→ broken URL. The comment says "first non-empty, non-comment line", which a reader would expect to cover this. The .tool-versions path is safe (awk '{print $2}'). Minor; worth a comment caveat or a fix.
edmorley
added
semver:major
|
Could you also rebase on |
4 participants
The
setup-packaction only accepted an explicitpack-version, and it piped the downloaded archive straight intotarwith no integrity check. This adds two things:pack-version-fileinput that reads the version from a.tool-versionsfile (thepackentry) or a plain version file like.pack-version. When set it takes precedence overpack-version. The existingpack-versiondefault line is left untouched so theupdate-pack-versionautomation keeps working.<asset>.sha256, so the action now downloads the archive to a temp file, verifies it against that checksum, and only then extracts. If the checksum cannot be fetched it warns and proceeds, so mirror / air-gapped setups are not broken.Both changes are backward compatible: with neither input changed, behavior is identical except the download is now checksum verified.
Validation
Done locally (the repo has no composite-action test harness, only the Go unit tests):
.tool-versionswith apackentry (including tab separated + trailing comment), plain version file (including a leadingvand comment lines), missing-file error, and.tool-versionswithout apackentry error.v0.40.6linux asset: a matching sum passes, a corrupted archive is rejected, andtarmember extraction still yields thepackbinary.shellcheckclean on both inline scripts.