diff --git a/CHANGELOG.md b/CHANGELOG.md index 496775d..a595db7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,12 @@ that may never merge. They are not releases and are not listed here. ### Added +- `mapbox config` — `get`/`set` for settings that persist across shells and + sessions, written to `~/.mapbox/config.json` (or `$MAPBOX_CONFIG_DIR`) + rather than an environment variable that only lasts for the session it was + set in. One setting today: `update-check`, which `mapbox config set + update-check off` turns off for good, mirroring `MAPBOX_NO_UPDATE_CHECK`. + - The README now documents installing without the install script: the archives are plain HTTP downloads, `manifest.json` lists every target with its checksum, and the commands to verify and extract one are written out. diff --git a/README.md b/README.md index 2e3e67c..a70c611 100644 --- a/README.md +++ b/README.md @@ -418,12 +418,16 @@ kept narrow: | What it sends | A `GET` for the channel's `latest/manifest.json`, with no token, no account, no command, and nothing about you or your machine beyond `User-Agent: mapbox-cli/` | | When | At most once a day, and only when stderr is a terminal, so CI and piped runs never check and never print | | Where | A detached background process. Your command never waits on it: offline, the timing is unchanged and nothing is printed | -| Off | `MAPBOX_NO_UPDATE_CHECK=1`, or `MAPBOX_CLI_NO_TELEMETRY=1`, which silences this too | +| Off | `MAPBOX_NO_UPDATE_CHECK=1`, or `MAPBOX_CLI_NO_TELEMETRY=1`, which silences this too, for the shell session it's set in | `~/.mapbox/update-check.json` (or `$MAPBOX_CONFIG_DIR`) holds the answer between runs. A build that names no release channel never checks at all, and `cargo build` produces one. +`mapbox config set update-check off` turns it off for good, in every shell — +see [Config](docs/commands.md#config) — rather than just the session an +environment variable happens to be set in. + ### Privacy **YOUR PRIVACY - COLLECTION OF TELEMETRY** diff --git a/docs/commands.md b/docs/commands.md index 03758e3..d20a63d 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -60,6 +60,9 @@ nests, and is typed `mapbox styles draft get`. **[Uninstall](#uninstall)** — [uninstall](#mapbox-uninstall) +**[Config](#config)** — [config.get](#mapbox-config-get) · +[config.set](#mapbox-config-set) + **[Usage](#usage)** — [usage](#mapbox-usage) **[Accounts](#accounts)** — @@ -3103,6 +3106,98 @@ Removed /home/user/.local/bin/mapbox. --- +## Config + +Settings that persist across shells and sessions — `~/.mapbox/config.json` +(or `$MAPBOX_CONFIG_DIR`), written the same way credentials are. One setting +today, `update-check`, which mirrors `MAPBOX_NO_UPDATE_CHECK` (see [Update +notices](../README.md#update-notices)) but stays off in every future shell +rather than only the one the environment variable was set in. + +### `mapbox config get` + +Prints a setting's current value: `on` in `text` mode, `true`/`false` in +`json`. Reading an unset `update-check` reports `on` — its default — rather +than failing, the same forgiving read the update-check cache itself uses. + +#### Parameters + +| Parameter | Effect | +| --- | --- | +| `` | Which setting to read. Only `update-check` exists today. | + +#### Examples + +```sh +mapbox config get update-check +``` + +#### Outputs + + + + +
textjson
+ +``` +on +``` + + + +```json +{ + "key": "update-check", + "value": true +} +``` + +
+ +### `mapbox config set` + +Persists a setting to `~/.mapbox/config.json`, so it survives across shells +without an environment variable. + +#### Parameters + +| Parameter | Effect | +| --- | --- | +| `` | Which setting to change. Only `update-check` exists today. | +| `` | `on` or `off`. | + +#### Examples + +```sh +mapbox config set update-check off + +mapbox config set update-check on +``` + +#### Outputs + + + + +
textjson
+ +``` +update-check set to off. +``` + + + +```json +{ + "key": "update-check", + "value": false +} +``` + +
+ +--- + ## Usage ### `mapbox usage` diff --git a/src/config.rs b/src/config.rs new file mode 100644 index 0000000..0a4a5f6 --- /dev/null +++ b/src/config.rs @@ -0,0 +1,184 @@ +//! `mapbox config` — settings that persist across shells and sessions. +//! +//! `MAPBOX_NO_UPDATE_CHECK=1` silences the update notice, but only for the +//! shell session that set it — there is no way to turn the check off once +//! and have it stay off. This is the persisted alternative: a small JSON +//! file beside the credentials, written through the same +//! [`crate::auth::write_private`] so it gets the same `0600` treatment. +//! +//! One setting today — `update-check` — with room for more: `get`/`set` take +//! a `key`, restricted by clap to [`KEYS`], so adding a second setting is a +//! new key and a new match arm rather than a new pair of subcommands. + +use std::path::PathBuf; + +use anyhow::{Context, Result}; +use clap::builder::PossibleValuesParser; +use clap::{Arg, ArgMatches, Command}; +use serde::{Deserialize, Serialize}; +use serde_json::json; + +use crate::auth; +use crate::output::{self, Mode}; + +pub const COMMAND: &str = "config"; + +const CONFIG_FILE: &str = "config.json"; + +const UPDATE_CHECK_KEY: &str = "update-check"; +const KEYS: &[&str] = &[UPDATE_CHECK_KEY]; + +const ON: &str = "on"; +const OFF: &str = "off"; + +/// What's persisted. `None` means "never set" for a setting whose CLI-visible +/// default is `on` — distinct from `Some(true)`, which is someone turning it +/// back on after having turned it off, but read identically by +/// [`update_check_setting`]. +#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize)] +struct Config { + #[serde(default, skip_serializing_if = "Option::is_none")] + update_check: Option, +} + +fn config_path() -> Option { + Some(auth::config_dir_path()?.join(CONFIG_FILE)) +} + +/// The persisted config, or the all-default one when there is nothing on +/// disk yet or what's there doesn't parse — the same forgiving read +/// [`crate::update_check`]'s cache uses, and for the same reason: a +/// malformed file here should cost nothing more than falling back to +/// defaults, never a failing command. +fn read_config() -> Config { + config_path() + .and_then(|path| std::fs::read_to_string(path).ok()) + .and_then(|text| serde_json::from_str(&text).ok()) + .unwrap_or_default() +} + +/// Best-effort in the read, deliberate in the write: `config set` is the one +/// command whose entire job is writing this file, so unlike the cache, a +/// failure here is reported rather than swallowed. +fn write_config(config: &Config) -> Result<()> { + let dir = auth::config_dir()?; + let text = serde_json::to_string(config).context("could not serialize the config")?; + auth::write_private(&dir.join(CONFIG_FILE), &text) +} + +/// Pure half of [`update_check_enabled`], so the default can be pinned +/// without going through the filesystem. +fn update_check_setting(config: &Config) -> bool { + config.update_check.unwrap_or(true) +} + +/// Whether the update check may run at all, per the persisted setting. +/// [`crate::update_check`] checks this alongside `MAPBOX_NO_UPDATE_CHECK` — +/// either one saying no is enough to stop it. +pub fn update_check_enabled() -> bool { + update_check_setting(&read_config()) +} + +fn on_off(enabled: bool) -> &'static str { + if enabled { + ON + } else { + OFF + } +} + +pub fn command() -> Command { + let key_arg = || { + Arg::new("key") + .required(true) + .value_parser(PossibleValuesParser::new(KEYS)) + }; + + Command::new(COMMAND) + .about("Get or set a persisted mapbox setting") + .long_about( + "Get or set a mapbox setting that persists across shells and sessions, \ + written to a file beside the stored credentials rather than an \ + environment variable that only lasts for the session it was set in.", + ) + .subcommand_required(true) + .subcommand( + Command::new("get") + .about("Print a setting's current value") + .arg(key_arg()), + ) + .subcommand( + Command::new("set") + .about("Persist a setting") + .arg(key_arg()) + .arg(Arg::new("value").required(true).value_parser([ON, OFF])), + ) +} + +pub fn get(matches: &ArgMatches, mode: Mode) -> Result<()> { + let key = matches.get_one::("key").expect("required"); + let config = read_config(); + + match key.as_str() { + UPDATE_CHECK_KEY => { + let enabled = update_check_setting(&config); + output::emit( + mode, + on_off(enabled), + json!({ "key": key, "value": enabled }), + ) + } + _ => unreachable!("clap's value_parser restricts `key` to {KEYS:?}"), + } +} + +pub fn set(matches: &ArgMatches, mode: Mode) -> Result<()> { + let key = matches.get_one::("key").expect("required"); + let value = matches.get_one::("value").expect("required"); + let enabled = value == ON; + + let mut config = read_config(); + match key.as_str() { + UPDATE_CHECK_KEY => config.update_check = Some(enabled), + _ => unreachable!("clap's value_parser restricts `key` to {KEYS:?}"), + } + write_config(&config)?; + + output::emit( + mode, + &format!("{key} set to {}.", on_off(enabled)), + json!({ "key": key, "value": enabled }), + ) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn an_unset_config_reads_update_check_as_on() { + assert!(update_check_setting(&Config::default())); + } + + #[test] + fn the_config_round_trips_and_tolerates_an_empty_one() { + let off = Config { + update_check: Some(false), + }; + let text = serde_json::to_string(&off).expect("serialize"); + assert_eq!(text, r#"{"update_check":false}"#); + let read: Config = serde_json::from_str(&text).expect("deserialize"); + assert_eq!(read, off); + + // A file from before this key existed, or one with nothing set yet. + let empty: Config = serde_json::from_str("{}").expect("an empty object"); + assert_eq!(empty.update_check, None); + assert!(update_check_setting(&empty)); + } + + #[test] + fn on_and_off_round_trip_through_on_off() { + assert_eq!(on_off(true), ON); + assert_eq!(on_off(false), OFF); + } +} diff --git a/src/main.rs b/src/main.rs index cf56b23..035dcea 100644 --- a/src/main.rs +++ b/src/main.rs @@ -17,6 +17,7 @@ mod agent_skills; mod api_command_surface; mod auth; mod completion; +mod config; mod confirm; mod deprecation; mod executor; @@ -591,6 +592,10 @@ fn build_app(specs: &[ServiceSpec]) -> Command { app = app.subcommand(uninstall::command()); + // Beside `uninstall`: the other command that only ever touches this + // machine, never the network. + app = app.subcommand(config::command()); + app = app.subcommand(account_usage::command()); app.subcommand(tilesets_cli::command()) @@ -1041,6 +1046,14 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - uninstall::run(assume_yes, mode)? } } + // Also ahead of the generic service arm, and for the same reason as + // `uninstall`: this reads and writes a file on this machine and + // makes no request. + Some((config::COMMAND, config_matches)) => match config_matches.subcommand() { + Some(("get", get_matches)) => config::get(get_matches, mode)?, + Some(("set", set_matches)) => config::set(set_matches, mode)?, + _ => unreachable!("`config` sets subcommand_required(true)"), + }, // Token resolution mirrors the service arm below, minus path // placeholders, a request body, and `--dry-run` — this GET always refreshes. Some((account_usage::COMMAND, usage_matches)) => { diff --git a/src/schema.rs b/src/schema.rs index e3e3003..d49d655 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -371,6 +371,14 @@ fn commands(app: &Command, specs: &[ServiceSpec], path: &[String]) -> Vec Option { /// /// Pure, and takes every input as an argument, because the interesting cases /// are exactly the ones a test process cannot be: a production build, a -/// terminal on stderr. See the module docs for why each of the four is here. +/// terminal on stderr. See the module docs for why each of the five is here. +/// The fifth, `config_allows`, is [`crate::config::update_check_enabled`] — +/// `MAPBOX_NO_UPDATE_CHECK` and `mapbox config set update-check off` are two +/// ways to say the same thing, and either saying it is enough. fn enabled( manifest_url: Option<&str>, stderr_is_terminal: bool, no_update_check: Option<&str>, telemetry_allowed: bool, + config_allows: bool, ) -> bool { - manifest_url.is_some() && stderr_is_terminal && no_update_check.is_none() && telemetry_allowed + manifest_url.is_some() + && stderr_is_terminal + && no_update_check.is_none() + && telemetry_allowed + && config_allows } /// The manifest this build would ask, or `None` for a build with no public @@ -353,7 +364,10 @@ pub fn is_refresh_child() -> bool { /// it. Belt and braces: this is the process that makes the request, and the /// switch that says "make no request" should be read by it. pub fn run_refresh_child() -> ExitCode { - if from_environment(NO_UPDATE_CHECK_ENV).is_some() || !crate::telemetry::telemetry_allowed() { + if from_environment(NO_UPDATE_CHECK_ENV).is_some() + || !crate::telemetry::telemetry_allowed() + || !crate::config::update_check_enabled() + { return ExitCode::SUCCESS; } if let Some(url) = manifest_url() { @@ -405,6 +419,7 @@ pub fn notify() { std::io::stderr().is_terminal(), from_environment(NO_UPDATE_CHECK_ENV).as_deref(), crate::telemetry::telemetry_allowed(), + crate::config::update_check_enabled(), ) { return; } @@ -520,24 +535,31 @@ mod tests { assert!(!is_newer("v0.1.5", "v0.1.5")); } - /// Every one of the four has to hold, and each one alone has to be able + /// Every one of the five has to hold, and each one alone has to be able /// to stop it. Written as a loop over which condition is broken so a - /// fifth condition added later cannot be silently untested. + /// sixth condition added later cannot be silently untested. #[test] fn every_gate_alone_is_enough_to_stop_it() { let url = Some("https://cli.mapbox.com/latest/manifest.json"); - assert!(enabled(url, true, None, true), "all four hold"); + assert!(enabled(url, true, None, true, true), "all five hold"); - assert!(!enabled(None, true, None, true), "no public channel"); - assert!(!enabled(url, false, None, true), "stderr is not a terminal"); + assert!(!enabled(None, true, None, true, true), "no public channel"); + assert!( + !enabled(url, false, None, true, true), + "stderr is not a terminal" + ); assert!( - !enabled(url, true, Some("1"), true), + !enabled(url, true, Some("1"), true, true), "MAPBOX_NO_UPDATE_CHECK is set" ); assert!( - !enabled(url, true, None, false), + !enabled(url, true, None, false, true), "MAPBOX_CLI_NO_TELEMETRY is set" ); + assert!( + !enabled(url, true, None, true, false), + "mapbox config set update-check off" + ); } /// The switch is a switch, not a value: `MAPBOX_NO_UPDATE_CHECK=0` is @@ -549,7 +571,7 @@ mod tests { let url = Some("https://cli.mapbox.com/latest/manifest.json"); for value in ["1", "true", "0", "no", "please-stop"] { assert!( - !enabled(url, true, Some(value), true), + !enabled(url, true, Some(value), true, true), "{value:?} did not opt out" ); } diff --git a/tests/config.rs b/tests/config.rs new file mode 100644 index 0000000..36dc19f --- /dev/null +++ b/tests/config.rs @@ -0,0 +1,147 @@ +//! End-to-end tests for `mapbox config get`/`set`. +//! +//! The unit tests in `src/config.rs` cover the pure decision — what an unset +//! key reads as, how the file round-trips. What they cannot show is that the +//! real binary's `get` sees what its own `set` wrote, through the config +//! directory rather than a value handed to a function in the same process. + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +fn scratch(name: &str) -> PathBuf { + let home = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join(format!("config-{name}")); + let _ = std::fs::remove_dir_all(&home); + std::fs::create_dir_all(home.join("config")).expect("create the scratch config dir"); + home +} + +fn config_dir(home: &Path) -> PathBuf { + home.join("config") +} + +/// The real binary, isolated from the developer's own environment and +/// credential store the same way `tests/update_check.rs` isolates it. +fn command(home: &Path) -> Command { + let mut cmd = Command::new(env!("CARGO_BIN_EXE_mapbox")); + cmd.env_remove("MAPBOX_ACCESS_TOKEN") + .env_remove("MapboxAccessToken") + .env_remove("MAPBOX_USERNAME") + .env_remove("MAPBOX_OUTPUT") + .env("HOME", home) + .env("XDG_CONFIG_HOME", home.join(".config")) + .env("MAPBOX_CONFIG_DIR", config_dir(home)); + cmd +} + +fn stdout(output: &Output) -> String { + String::from_utf8_lossy(&output.stdout) + .trim_end() + .to_string() +} + +#[test] +fn an_unset_update_check_reads_on() { + let home = scratch("get-default"); + + // `-o text` explicitly: `auto`, the default, reads a piped stdout as a + // request for JSON, which is exactly the other half of this test. + let text = command(&home) + .args(["-o", "text", "config", "get", "update-check"]) + .output() + .expect("run mapbox config get"); + assert!(text.status.success()); + assert_eq!(stdout(&text), "on"); + + let json = command(&home) + .args(["-o", "json", "config", "get", "update-check"]) + .output() + .expect("run mapbox config get -o json"); + assert!(json.status.success()); + assert_eq!( + stdout(&json), + r#"{"key":"update-check","value":true}"#, + "no config.json exists yet, so this is the default reading itself back" + ); + assert!( + !config_dir(&home).join("config.json").exists(), + "a bare `get` must not create the file a `set` would" + ); +} + +#[test] +fn set_persists_across_separate_invocations() { + let home = scratch("set-then-get"); + + let off = command(&home) + .args(["-o", "json", "config", "set", "update-check", "off"]) + .output() + .expect("run mapbox config set"); + assert!(off.status.success()); + assert_eq!(stdout(&off), r#"{"key":"update-check","value":false}"#); + + // A fresh process, not the one that wrote it — the whole point being + // tested is that the setting outlives a single invocation. + let read_back = command(&home) + .args(["-o", "text", "config", "get", "update-check"]) + .output() + .expect("run mapbox config get"); + assert!(read_back.status.success()); + assert_eq!(stdout(&read_back), "off"); + + let on = command(&home) + .args(["-o", "text", "config", "set", "update-check", "on"]) + .output() + .expect("run mapbox config set"); + assert!(on.status.success()); + assert_eq!(stdout(&on), "update-check set to on."); + + let read_back_again = command(&home) + .args(["-o", "text", "config", "get", "update-check"]) + .output() + .expect("run mapbox config get"); + assert_eq!(stdout(&read_back_again), "on"); +} + +#[test] +fn an_unknown_key_or_value_is_a_usage_error_not_a_panic() { + let home = scratch("bad-input"); + + let bad_key = command(&home) + .args(["config", "get", "not-a-real-setting"]) + .output() + .expect("run mapbox config get"); + assert!(!bad_key.status.success()); + + let bad_value = command(&home) + .args(["config", "set", "update-check", "sideways"]) + .output() + .expect("run mapbox config set"); + assert!(!bad_value.status.success()); + assert!( + !config_dir(&home).join("config.json").exists(), + "a rejected value must not reach the file" + ); +} + +#[cfg(unix)] +#[test] +fn the_config_file_is_written_private() { + use std::os::unix::fs::PermissionsExt; + + let home = scratch("perms"); + let out = command(&home) + .args(["config", "set", "update-check", "off"]) + .output() + .expect("run mapbox config set"); + assert!(out.status.success()); + + let mode = std::fs::metadata(config_dir(&home).join("config.json")) + .expect("config.json exists") + .permissions() + .mode() + & 0o777; + assert_eq!( + mode, 0o600, + "config.json should be as private as credentials.json" + ); +} diff --git a/tests/update_check.rs b/tests/update_check.rs index 6ac4b88..1f49b6c 100644 --- a/tests/update_check.rs +++ b/tests/update_check.rs @@ -96,6 +96,22 @@ fn stderr(output: &Output) -> String { String::from_utf8_lossy(&output.stderr).to_string() } +/// Writes the persisted opt-out through the real `mapbox config set`, rather +/// than hand-writing `config.json` the way [`seed_cache`] hand-writes the +/// cache — proving the two commands agree on the file, not just that this +/// test's idea of its shape does. +fn set_update_check(home: &Path, value: &str) { + let out = command(home) + .args(["config", "set", "update-check", value]) + .output() + .expect("run mapbox config set"); + assert!( + out.status.success(), + "mapbox config set update-check {value} failed: {}", + stderr(&out) + ); +} + /// A loopback stand-in for `/latest/manifest.json`. /// /// Serves one request and reports the request head it saw, so a test can @@ -298,6 +314,30 @@ fn the_switches_stop_the_child_too() { } } +/// The same proof as [`the_switches_stop_the_child_too`], for the persisted +/// setting rather than an environment variable — and written through the +/// CLI's own `config set` instead of a hand-seeded file, so this is really +/// two commands agreeing rather than one test's assumption about both. +#[test] +fn the_persisted_opt_out_stops_the_child_too() { + let home = scratch("child-config"); + set_update_check(&home, "off"); + let (server, url) = manifest_server(manifest(NEWER)); + + let out = command(&home) + .env("MAPBOX_INTERNAL_UPDATE_REFRESH", "1") + .env("MAPBOX_INTERNAL_UPDATE_URL", &url) + .output() + .expect("run mapbox"); + + assert!(out.status.success()); + assert!( + read_cache(&home).is_none(), + "a persisted `update-check off` did not stop the child from fetching" + ); + drop(server); +} + // --------------------------------------------------------------- no terminal /// Piped, scripted or in CI, the whole thing is off: no notice, no request, @@ -522,6 +562,31 @@ fn either_switch_silences_the_notice() { } } +/// The persisted opt-out silences the notice at a terminal too, the same as +/// either environment switch does above. +#[cfg(unix)] +#[test] +fn the_persisted_opt_out_silences_the_notice() { + let home = scratch("pty-config"); + let out_path = home.join("stdout"); + seed_cache(&home, NEWER, now(), 0); + set_update_check(&home, "off"); + + let (session, _) = under_a_pty( + &home, + &out_path, + "--version", + &[("MAPBOX_INTERNAL_UPDATE_URL", &closed_port_url())], + "mapbox ", + ) + .expect("neither `script` form ran the command"); + + assert!( + !session.contains(NOTICE_MARK), + "a persisted `update-check off` did not silence the notice: {session}" + ); +} + /// The whole loop, as a person would meet it: a run with a cold cache /// refreshes in the background and says nothing, and the run after it is the /// one that tells them.