Accept a pinned version with or without the leading v - #29
Merged
Merged
Conversation
This was referenced Sep 16, 2026
zmofei
previously approved these changes
Sep 17, 2026
zmofei
previously approved these changes
Sep 18, 2026
Reported on #27. The channel's directories are named `v0.2.1`, and `MAPBOX_CLI_VERSION` went into the URL untouched — so the spelling a person would actually copy failed: $ MAPBOX_CLI_VERSION=0.2.1 … curl: (56) The requested URL returned error: 403 mapbox-cli: could not read https://cli.mapbox.com/0.2.1/manifest.json Every place a version is read from shows it without the `v`: `mapbox --version`, CHANGELOG.md, Cargo.toml. So the one spelling that worked was the one nobody sees, and the failure was a bare S3 `403`, which reads as "you are not allowed" rather than "no such version". The review asked for a note about it. A note documents a trap; this removes it. Both installers now prepend the `v` when the value starts with a digit, which leaves `latest` — and any other channel name — alone, since only a leading digit means a version number is being named. Verified against the live channel: `0.2.1`, `v0.2.1` and `latest` all install 0.2.1. Two cases added to each installer suite, including one pinning `latest` because getting that wrong would break the default install rather than an edge case. Both confirmed to fail with the fix reverted. **And the variable is now documented at all**, which is the other half of this. `MAPBOX_CLI_VERSION` appeared in no `.md` in the repository — the reviewer on #26 suggested using it, which is how I found that a reader had no way to know it exists. It goes in the install section with `MAPBOX_INSTALL_DIR`, which was also undocumented. 601 tests, both installer suites green, fmt clean, no broken anchors.
CI went red on the previous commit for a reason worth recording rather than just fixing: `PSUseBOMForUnicodeEncodedFile` fails a non-ASCII `.ps1` that carries no BOM, and the em-dash in my new comment was the first non-ASCII byte the file had ever contained. Both `.ps1` files were ASCII-clean before this branch and are again. The comment says so, so the next person writing prose in there knows the constraint exists before CI tells them. Also ran PSScriptAnalyzer locally with ci.yml's exact rule set — the same exclusions and the same `PSUseCompatibleSyntax` 5.1/7.0 pair — rather than pushing and hoping. Clean on both files.
zmofei
force-pushed
the
version-prefix
branch
from
September 18, 2026 12:21
0a5da8c to
4c6f14e
Compare
zmofei
approved these changes
Sep 18, 2026
ctufts
approved these changes
Sep 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
From @zmofei's review on #27, plus the gap his #26 comment exposed.
The bug
The channel's directories are named
v0.2.1, andMAPBOX_CLI_VERSIONwent into the URL untouched:Every place a person reads a version from shows it without the
v—mapbox --version,CHANGELOG.md,Cargo.toml. So the only spelling that worked was the one nobody sees, and the failure was a bare S3403, which reads as "you are not allowed" rather than "no such version".The fix, rather than a note about it
The review asked for a note near the version section. A note documents a trap; this removes it. Both installers prepend the
vwhen the value starts with a digit:A leading digit is the signal, so
latest— and any future channel name — is left alone.Verified against the live channel:
Two cases added to each installer suite, including one that pins
latest, because getting that wrong would break the default install rather than an edge case. Both confirmed to fail with the fix reverted —could not read …/0.1.0-dev.abc1234/manifest.jsonin bash, the same in PowerShell.And the variable is documented now
This is the other half, and it came from the #26 comment.
MAPBOX_CLI_VERSIONappeared in no.mdanywhere in the repo — so suggesting people use it was suggesting something they had no way to discover. It now sits in the install section alongsideMAPBOX_INSTALL_DIR, which was also undocumented:curl -fsSL https://cli.mapbox.com/install.sh | MAPBOX_CLI_VERSION=0.2.1 sh601 Rust tests, both installer suites green, fmt clean, no broken anchors.