Fix rollback hint to reference a capture route that exists - #352
Conversation
The run_rollback missing-target error told operators to use `deploy`'s `previous-version` output, but the CLI's production deploy emits only `version=<N>` -- `previous-version` exists only as the deploy-fastly GitHub action's output, captured by running `active-version` before the deploy. An operator following the hint mid-incident found nothing to use. Reword the hint (and the matching --rollback-to doc comment) to name the real capture routes: run `active-version` before deploying, or wire the deploy-fastly action's `previous-version` output. Emitting `previous-version` from the CLI deploy itself was considered and rejected: it would make a pre-deploy Fastly API call mandatory, breaking the documented output-parsing path that works under manifest command overrides with dummy credentials, and a warn-and-continue capture would produce a line scripts cannot rely on. The action layer already captures the target fail-closed at the right place. Closes #350
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Correct diagnosis and a minimal two-string fix: the hint now names capture routes that actually exist (active-version, and the deploy-fastly action's previous-version output), matching what capture-previous.sh really does. No behavior change, CI green. All findings below are non-blocking wording/consistency polish.
😃 Praise
- Rejecting option 1 was the right call, and the reasoning is recorded. The production deploy resolves its version from the deploy command's own output first, with the Fastly API only as a fallback, precisely so manifest
deployoverrides work without API access — making a pre-deploy capture mandatory would have broken that path and added a failure mode to every production deploy. Writing that trade-off into the PR body (and commit message) is what stops it being re-litigated in six months. - Verification was done properly:
git grep previous-version crates/confirms no CLI surface emits it, and no test pinned the old string, so the change is genuinely two strings.
Findings
Blocking
None.
Non-blocking
-
🤔 Hint gives the operator no forward path —
crates/edgezero-cli/src/lib.rs:519-521(inline). The reader is past the point where "capture it before deploying" can help. -
⛏ Output key not named —
crates/edgezero-cli/src/lib.rs:519(inline).active-versionemitsversion=<N>, notprevious-version=<N>. -
⛏ Parenthetical mixes a command with an output —
crates/edgezero-cli/src/args.rs:384-385(inline). Renders in--help. -
🏕 CLI reference still names only the Actions route —
docs/guide/cli-reference.md:257. The bullet reads "capture it before the superseding deploy (deploy-fastly'sprevious-version)". Not wrong, but this is the CLI reference and it omits the CLI-native route —edgezero active-version, documented 60 lines above it atdocs/guide/cli-reference.md:194. A CLI-only operator with no GitHub Actions gets pointed at an action. This is the same asymmetry the PR fixes in--help, and a one-line edit keeps the error text,--help, and the docs all saying the same thing:- `--rollback-to <n>` — **production only:** ... capture it before the superseding deploy (run [`active-version`](#edgezero-active-version), or use `deploy-fastly`'s `previous-version` output) and pass it here. ...
(Verified: heading
### edgezero active-versionexists and is unique, so the anchor resolves and the VitePress build stays clean.) -
📝 Fastly-specific text in an adapter-agnostic function —
crates/edgezero-cli/src/lib.rs:517-521.run_rollbackdispatches to any adapter, but the hint names Fastly and a Fastly-specific GitHub action. Pre-existing (the "Fastly exposes no metadata" clause already did this) and Fastly is currently the only adapter implementing rollback. Informational, no action.
📌 Out of Scope
- 🌱 Nothing guards against the same drift recurring. The bug class here is an operator-facing string that quietly stopped matching reality; a stale hint costs the most at exactly the moment it is read. Nothing in the tree catches the next one. Not this PR's job — worth a tracking issue if hint accuracy is considered load-bearing.
CI Status
Run locally on 296f247 (PR head):
- fmt: PASS
- clippy (
--workspace --all-targets --all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS — 1392 tests, 0 failures - GitHub checks: 21/21 SUCCESS
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed 296f2476b77accb2424a2c09e4b050fdc8679073 against 10263543d352b1293c325245ab94dd1a5e275314.
This is a low-risk, two-string correction. The revised error and --help text now point to capture routes that exist: the CLI's active-version command and the deploy-fastly action's previous-version output. No deployment or rollback logic changed.
Safety proof
-
The action captures and later consumes the same rollback target
- Highest evidence: Runtime
- Evidence: The
composite-smokejob capturedversion=40asprevious-version=40before deployment, then passed it through rollback, which emittedrolled-back-to=40. - Status: proven
-
active-versionhandles active, absent, and malformed provider state- Highest evidence: Executed
- Evidence: Focused Fastly adapter tests passed for active-version policy and malformed provider responses. A compiled-CLI check also mapped
version=40throughcapture-previous.shtoprevious-version=40. - Status: proven
Findings
No P0, P1, P2, or P3 issues found.
Validation and review context
cargo test -p edgezero-cli: passed.- Focused Fastly active-version tests: passed.
- Repository-pinned Rust 1.95 clippy for
edgezero-cli: passed. - Action contract tests: 242 passed, 0 failed.
- CI: 21 of 21 checks passed.
- Existing feedback: Three unresolved non-blocking wording threads and one documentation consistency note already cover the remaining polish suggestions. No duplicate findings added.
- Residual risk: No live Fastly API call was made. The unchanged provider path is covered by focused tests, action-contract tests, and end-to-end CI smoke tests.
Address prk-Jr's non-blocking review findings on #352: - Name the output key: `active-version` prints `version=<N>`, so an operator scripting the capture knows which line feeds --rollback-to. - Add a forward path for the operator reading the error mid-incident with no captured target: choose the target from the service's version history, since Fastly cannot identify it. - Rework the --rollback-to doc comment parenthetical so it no longer joins a command and an output name with one comma (renders in --help). - Add the CLI-native capture route to the CLI reference bullet, which named only the deploy-fastly Actions route.
|
Addressed the review polish in 389362b: output key named in the hint, forward-path clause added, the |
Problem
Addresses #350. The
run_rollbackmissing-target error told operators to usedeploy'sprevious-versionoutput, but no such output exists: the CLI's production deploy emits onlyversion=<N>.previous-versionis produced one layer up, by thedeploy-fastlyGitHub action'scapture-previous.sh, which runsactive-versionbefore deploying. An operator following the hint mid-incident found nothing to use.Change
Reword the hint in
run_rollbackand the matching--rollback-todoc comment inRollbackArgsto name the capture routes that actually exist: runactive-versionbefore deploying, or wire thedeploy-fastlyaction'sprevious-versionoutput. Two strings, no behavior change. The docs site already attributesprevious-versionto the action correctly, so it needs no edit.Why not option 1 (emit
previous-versionfrom the CLI deploy)The production deploy resolves its version from the deploy command's own output first, with the Fastly API only as a fallback, deliberately so manifest
deploycommand overrides (including test fixtures with dummy credentials) work without API access. Capturing the previous version can only happen via a pre-deploy API call, so option 1 either makes that call mandatory (breaking the override path and adding a new failure mode to every production deploy) or tolerates capture failure (emitting a line scripts cannot rely on, which is worse than no line). The action layer already captures the target fail-closed at the right place; the CLI hint just needs to tell the truth.Verification
cargo fmt -p edgezero-cli -- --checkpassescargo clippy -p edgezero-cli --all-targets --all-features -- -D warningspassescargo test -p edgezero-clipassesgit grep previous-version crates/shows only the corrected references; no test pins the old hint textCloses #350