cli(upgrade): fix 404 — drop version from tarball filename (#352) - #377
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d963ea69d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| std::string ext = (platform == "windows") ? "zip" : "tar.gz"; | ||
| std::string tarball = "pulp-" + version + "-" + platform + "-" + arch + "." + ext; | ||
| std::string url = "https://github.com/danielraffel/pulp/releases/download/v" + version + "/" + tarball; | ||
| auto [tarball, url] = pulp::cli::pulp_upgrade_url_for(version, platform, arch); |
There was a problem hiding this comment.
Handle Windows zip upgrade payloads end-to-end
After this change, Windows now targets pulp-windows-*.zip assets, but the rest of cmd_upgrade still follows the Unix tarball path (tar -xzf) and later expects ${tmp_dir}/pulp. The release workflow packages Windows as pulp.exe inside a zip, so on Windows the download can succeed and then fail during extraction or binary lookup, leaving pulp upgrade broken for that platform.
Useful? React with 👍 / 👎.
`pulp upgrade` constructed asset names as "pulp-<version>-<platform>-<arch>.<ext>" and used "x86_64" for the Linux x64 arch. Both diverge from what .github/workflows/release-cli.yml actually uploads, so every upgrade 404'd on the download step. Release workflow naming (authoritative): Asset = "pulp-<platform>-<arch>.<ext>" (no version in the filename) URL = ".../releases/download/v<version>/<asset>" arch = "arm64" | "x64" (NOT "x86_64") ext = "zip" for windows, "tar.gz" otherwise Changes - tools/cli/upgrade_url.hpp: header-only URL/asset builder. Kept as a header so tests pin the naming convention without linking cmd_misc's transitive CLI deps. - tools/cli/cmd_misc.cpp: replace the local helper + version-in-filename bug with a call into upgrade_url.hpp. Drops x86_64 → x64 and the version segment. - test/test_cli_upgrade_url.cpp: 6 Catch2 cases that pin asset names and URL shape against real v0.14.0 releases — including an explicit guard that the version never reappears in the filename. Tests-ship-with-fixes: this is a correctness fix, and the 6 cases here are the regression guard that keeps the convention wired to the release workflow. Closes #352.
d963ea6 to
6a2bb8c
Compare
Unblocks the skill-sync gate on PR #377: modifying cmd_misc.cpp maps to the cli-maintenance skill, and the asset-name/URL convention the upgrade helper relies on is exactly the kind of brittle-invariant gotcha this skill is supposed to capture.
Summary
pulp upgrade404'd on every run because the CLI built download URLsusing
pulp-<version>-<platform>-<arch>.<ext>andx86_64, while therelease workflow uploads assets as
pulp-<platform>-<arch>.<ext>withx64. This PR pins the URL shape to the release workflow and adds aCatch2 regression guard so the two can't drift silently again.
This is the second half of #352; the first half (
pulp pr→shipyard prshim) is in-flight as #376. Fixing both gets
pulp upgradeandpulp prback to parity with their documented behavior.
Changes
the regression test can pin the naming convention without linking
cmd_misc's transitive CLI deps.
the version into the filename and used
x86_64) with a call intopulp::cli::pulp_upgrade_url_for().v0.14.0 release naming, including one that explicitly fails if a
future refactor re-introduces the version in the filename.
Convention (pinned by the header + tests)
Mirrors
.github/workflows/release-cli.yml.Test plan
pulp-test-cli-upgrade-urlruns green locally (10 assertions,6 cases).
cmake --build build --target pulp-clisucceeds (refactorcompiles end-to-end).
pulp upgradeagainst a realrelease downloads successfully.
Closes #352.