Teach both installers the telemetry opt-out's new name - #7
Merged
Merged
Conversation
This was referenced Sep 14, 2026
mattpodwysocki
force-pushed
the
installer-telemetry-name
branch
2 times, most recently
from
September 14, 2026 17:55
b56a3df to
831fff5
Compare
zmofei
approved these changes
Sep 14, 2026
zmofei
left a comment
Member
There was a problem hiding this comment.
LGTM — precedence logic and installer parity verified, tests cover both directions, CI green. Note: PR has a merge conflict with base, needs rebase before merging.
The binary's opt-out was renamed from `DISABLE_TELEMETRY` to `MAPBOX_CLI_NO_TELEMETRY` and the break was documented honestly. Both installers were left on the old name, so after that release neither variable silenced both halves: the documented one stopped the CLI's markers but not the installer's, and the old one did the reverse. `docs/distribution.md` in the private repo already claimed the fix — "`MAPBOX_CLI_NO_TELEMETRY` switches all of that off … Both installers carry their own copy of this logic, and both test suites pin it in both directions." The first half was false and the second pinned the wrong name. This makes that documentation true rather than changing it. **The old name keeps working here, and only here.** The binary's break was announced in a changelog someone upgrading can read. An installer is fetched and executed in one line — `curl … | sh`, `irm … | iex` — so its reader has no release notes in front of them, and breaking an opt-out is the one change that must not happen quietly. Asymmetric on purpose, and the comment in each file says so. When both are set the new name wins, including when it declines: an explicit `MAPBOX_CLI_NO_TELEMETRY=0` beats a stale `DISABLE_TELEMETRY=1` left in an image from before the rename. Without that the old variable could never be retired. Verified rather than reasoned about: each script's decision block was run over the cases directly — nine in `sh`, eight in `pwsh` 7.6.6 — covering both names, both precedence directions, the six false spellings, an unknown spelling, whitespace, and a set-but-empty new name as a deliberate clear. Both real harnesses then got cases asserting the new name opts out, wins when the two disagree, and wins in the opt-out direction too. `test-install.sh` reports all cases passed; `test-install.ps1` reports all good. Ported from mapbox/mapbox-cli-private#141, which cannot merge there now that `oss/` is a submodule (mapbox/mapbox-cli-private#132). That PR also edited `docs/distribution.md`, which lives in the private repo and stays there.
mattpodwysocki
force-pushed
the
installer-telemetry-name
branch
from
September 14, 2026 18:05
831fff5 to
eb5e140
Compare
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
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.
Ported from mapbox-cli-private#141, which can no longer merge there now that
oss/is a submodule (#132).The gap
The binary's telemetry opt-out was renamed from
DISABLE_TELEMETRYtoMAPBOX_CLI_NO_TELEMETRY, and that break was documented honestly — the changelog says aDISABLE_TELEMETRY=1"silently stops opting out". Both installers were left on the old name, so after that release neither variable silenced both halves:mapboxbinaryinstall.sh/install.ps1MAPBOX_CLI_NO_TELEMETRY=1DISABLE_TELEMETRY=1The private repo's
docs/distribution.mdalready claimed the fix:The first half was false, and the second pinned the wrong name. So this makes the documentation true rather than changing it.
The old name keeps working, in the installers only
Asymmetric on purpose. The binary's break was announced in a changelog someone upgrading can read. An installer is fetched and executed in one line —
curl … | sh,irm … | iex— so its reader has no release notes in front of them, and breaking an opt-out is the one change that must not happen quietly. The comment in each file says exactly that, so the asymmetry reads as chosen rather than as an oversight in the other direction.When both are set the new name wins, including when it declines: an explicit
MAPBOX_CLI_NO_TELEMETRY=0beats a staleDISABLE_TELEMETRY=1left in an image from before the rename. Without that the old variable could never be retired.Verified rather than reasoned about
I pulled each script's decision block out and ran it over the cases directly — nine in
sh, eight inpwsh7.6.6 — covering both names, both precedence directions, the six false spellings, an unknown spelling (which must send less), whitespace, and a set-but-empty new name as a deliberate clear. All pass.Then added cases to both real harnesses, which pass here too:
Both suites green end to end in this repo:
scripts/test-install.shreportsall cases passed,scripts/test-install.ps1reportsall good(2 pre-existing Windows-only skips, unrelated).new_case_env/New-CaseEnvnow clear the new variable too, for the same reason they already cleared the old one — a developer with it set in their own shell would otherwise turn every marker case into a failure that looks like the marker broke.The original PR also edited
docs/distribution.md, which lives in the private repo and stays there.