amadan
carlos deploy: cover the clean-tree case, name the dirty tree, guard-order comment
Fix round 1 review findings on the dirty-tree preflight: - Add TestDeployAcceptsADerivedVersionFromACleanTree: the branch whose failure mode is refusing every ordinary deploy was untested, and invisible everywhere else in the suite (every runDeploy call site passes -version explicitly; CI checks out a clean tree). initDirtyGitRepo is now initGitRepo(t, dirty bool), with initDirtyGitRepo kept as a thin wrapper for readability at existing call sites. - deployPreflight now names the absolute path of the tree it inspected in its refusal, so an operator shipping a prebuilt artifact from an unrelated dirty checkout knows which tree tripped it. - Comment the explicitVersion capture in runDeploy: it MUST precede the gitShortHead() default-fill, and moving it below would silently defeat the guard (explicitVersion never empty, refusal never fires) with every existing test still green, since none of them rely on the derived-version path through runDeploy itself.
2 files changed,
+50
−8
deploy.go+16 −2deploy_test.go+34 −6
diff --git a/deploy.go b/deploy.go| index a57693017..c8538b9d7 100644 |
| --- a/deploy.go |
| +++ b/deploy.go |
| @@ -50,6 +50,13 @@ func runDeploy(args []string) { |
| "[-account <sqid-or-name>] [-account-id <sqid>] [-console <origin>] [-capabilities <file.json>] " + |
| "[-channel <ch>] [-host <host>] [-timeout 120s] <files-or-site-dir>\n" + |
| "or, in a project with saved defaults (or at a terminal, to create them): carlos deploy" |
| + // MUST be captured before the gitShortHead() default-fill below: this is |
| + // the only signal that distinguishes "operator typed -version" from |
| + // "flag defaulted to the derived sha", and deployPreflight uses it to |
| + // decide whether a dirty tree is refused or trusted. Move this line below |
| + // the default-fill and explicitVersion is never empty, the guard never |
| + // fires — and silently: every runDeploy test in this package passes |
| + // -version explicitly, so that regression would still show green. |
| explicitVersion := *ver |
| if *ver == "" { |
| *ver = gitShortHead() |
| @@ -950,6 +957,13 @@ func deployPreflight(dir, explicitVersion string) error { |
| if explicitVersion != "" { |
| return nil |
| } |
| + // Named in the refusal below so an operator shipping a prebuilt artifact |
| + // from an unrelated dirty checkout knows which tree tripped it, rather |
| + // than guessing from a bare "-C .". |
| + named := dir |
| + if abs, err := filepath.Abs(dir); err == nil { |
| + named = abs |
| + } |
| out, err := exec.Command("git", "-C", dir, "status", "--porcelain").Output() |
| if err != nil { |
| return nil // not a git checkout, or no git: nothing to be dirty |
| @@ -957,8 +971,8 @@ func deployPreflight(dir, explicitVersion string) error { |
| if len(bytes.TrimSpace(out)) == 0 { |
| return nil |
| } |
| - return errors.New("this tree has uncommitted changes and -version defaults to the git short sha, " + |
| - "which would name a commit this build does not match — commit, or pass -version explicitly") |
| + return fmt.Errorf("%s has uncommitted changes and -version defaults to the git short sha, "+ |
| + "which would name a commit this build does not match — commit, or pass -version explicitly", named) |
| } |
| // gitShortHead resolves -version's default: the working directory's short |
diff --git a/deploy_test.go b/deploy_test.go| index bfa6da928..53da1b438 100644 |
| --- a/deploy_test.go |
| +++ b/deploy_test.go |
| @@ -1700,10 +1700,14 @@ func TestDeployRefusesAMalformedProbeAndFallsBackToTheHeader(t *testing.T) { |
| } |
| } |
| -// initDirtyGitRepo builds a git repo in a fresh temp dir with one committed |
| -// file, then modifies that file so the working tree is dirty. Skips the test |
| -// if git is unavailable in this environment. |
| -func initDirtyGitRepo(t *testing.T) string { |
| +// initGitRepo builds a git repo in a fresh temp dir with one committed file. |
| +// With dirty=true, the file is then modified so the working tree is dirty; |
| +// with dirty=false, the tree is left exactly as committed — the branch whose |
| +// failure mode is "refuses every ordinary deploy" and which nothing else in |
| +// this suite exercises, since every runDeploy call site passes -version |
| +// explicitly and CI itself checks out a clean tree. Skips the test if git is |
| +// unavailable in this environment. |
| +func initGitRepo(t *testing.T, dirty bool) string { |
| t.Helper() |
| if _, err := exec.LookPath("git"); err != nil { |
| t.Skip("git not available") |
| @@ -1729,12 +1733,21 @@ func initDirtyGitRepo(t *testing.T) string { |
| } |
| run("add", "file.txt") |
| run("commit", "-m", "initial") |
| - if err := os.WriteFile(f, []byte("dirty\n"), 0o644); err != nil { |
| - t.Fatal(err) |
| + if dirty { |
| + if err := os.WriteFile(f, []byte("dirty\n"), 0o644); err != nil { |
| + t.Fatal(err) |
| + } |
| } |
| return dir |
| } |
| +// initDirtyGitRepo is initGitRepo(t, true) — kept as its own name since it |
| +// reads better at call sites that only ever want a dirty tree. |
| +func initDirtyGitRepo(t *testing.T) string { |
| + t.Helper() |
| + return initGitRepo(t, true) |
| +} |
| + |
| // TestDeployRefusesADerivedVersionFromADirtyTree: the default -version is the |
| // git short sha, so a dirty tree ships a stamp naming a commit the binary was |
| // not built from — which is precisely how a false "verified" happens, and why |
| @@ -1749,7 +1762,22 @@ func TestDeployRefusesADerivedVersionFromADirtyTree(t *testing.T) { |
| if !strings.Contains(err.Error(), "-version") { |
| t.Fatalf("refusal %q should name the flag that overrides it", err) |
| } |
| + if !strings.Contains(err.Error(), dir) { |
| + t.Fatalf("refusal %q should name the tree it inspected (%s)", err, dir) |
| + } |
| if err := deployPreflight(dir, "abc1234"); err != nil { |
| t.Fatalf("an explicit -version from a dirty tree = %v, want accepted", err) |
| } |
| } |
| + |
| +// TestDeployAcceptsADerivedVersionFromACleanTree: the branch every other |
| +// test in this file skips over. All 13 runDeploy call sites pass -version |
| +// explicitly, and CI itself checks out a clean tree, so a preflight that |
| +// wrongly refused every derived-version deploy from a clean checkout would |
| +// ship green with nothing else in the suite to catch it. |
| +func TestDeployAcceptsADerivedVersionFromACleanTree(t *testing.T) { |
| + dir := initGitRepo(t, false) |
| + if err := deployPreflight(dir, "" /* no explicit -version */); err != nil { |
| + t.Fatalf("a clean tree with a derived version = %v, want accepted", err) |
| + } |
| +} |