diff --git a/commands/play_executor_test.go b/commands/play_executor_test.go index 91c0349..f2106ed 100644 --- a/commands/play_executor_test.go +++ b/commands/play_executor_test.go @@ -133,18 +133,15 @@ func TestMultiPlayPlayFilterComposesWithTags(t *testing.T) { tasks: - name: deploy-api tags: [deploy] - dokku_app: - app: docket-test-deploy-api + dokku_stub: { key: deploy-api } - name: configure-api tags: [configure] - dokku_app: - app: docket-test-configure-api + dokku_stub: { key: configure-api } - name: worker tasks: - name: deploy-worker tags: [deploy] - dokku_app: - app: docket-test-deploy-worker + dokku_stub: { key: deploy-worker } `) stdout, _, _ := runPlan(t, path, "--play", "api", "--tags", "deploy") if !strings.Contains(stdout, "deploy-api") { @@ -168,14 +165,12 @@ func TestMultiPlayPlayLevelTagsPropagateToTasks(t *testing.T) { tags: [api] tasks: - name: deploy-api - dokku_app: - app: docket-test-deploy-api + dokku_stub: { key: deploy-api } - name: worker tags: [worker] tasks: - name: deploy-worker - dokku_app: - app: docket-test-deploy-worker + dokku_stub: { key: deploy-worker } `) stdout, _, _ := runPlan(t, path, "--tags", "api") if !strings.Contains(stdout, "deploy-api") { @@ -238,6 +233,41 @@ func TestPlanFailedWhenClearsError(t *testing.T) { } } +// TestPlanProbeErrorRendersMarkerAndExits pins the documented contract +// that an uncleared probe error renders [!] on stderr and makes plan +// exit 1, and that errors win over drift under --detailed-exitcode (exit +// 1, not 2). This is the end-to-end guard for #328: a probe that could +// not run must not be reported as absent with an optimistic [+] create. +func TestPlanProbeErrorRendersMarkerAndExits(t *testing.T) { + defer stubReset() + stubSet("a", StubFixture{ + PlanError: errors.New(`exec: "dokku": executable file not found in $PATH`), + }) + + path := writeMultiPlayTasks(t, `--- +- tasks: + - name: needs-dokku + dokku_stub: { key: a } +`) + + stdout, stderr, exit := runPlan(t, path) + if exit != 1 { + t.Fatalf("exit = %d, want 1; stdout=%s stderr=%s", exit, stdout, stderr) + } + if !strings.Contains(stderr, "[!]") { + t.Errorf("expected [!] marker on stderr; got stdout=%s stderr=%s", stdout, stderr) + } + if strings.Contains(stdout, "[+]") { + t.Errorf("plan must not predict [+] create for state it never read; got:\n%s", stdout) + } + + // --detailed-exitcode: errors win over drift, so exit stays 1, not 2. + _, _, exit = runPlan(t, path, "--detailed-exitcode") + if exit != 1 { + t.Errorf("detailed-exitcode exit = %d, want 1 (errors win over drift)", exit) + } +} + // TestPlanChangedWhenTrueRecomputesStatus pins #313: changed_when: 'true' // on an in-sync task must flip the marker and the JSON status to a // would-change verdict, not leave the stale [ok] / "status":"ok" the diff --git a/docs/command-reference.md b/docs/command-reference.md index 156e98c..fb7482a 100644 --- a/docs/command-reference.md +++ b/docs/command-reference.md @@ -149,6 +149,10 @@ decide whether to mutate. A few tasks (notably `dokku_git_auth`, `dokku_registry `dokku_storage_ensure`) cannot read their state without running the underlying command, so they always report drift with a `(... not probed)` reason. +When the probe command cannot run at all - for example the local `dokku` CLI is not installed, or +the configured SSH host is unreachable - the task renders `[!]` and `plan` exits `1`, rather than +optimistically predicting `[+] create` for state it never actually read. + | Flag | Effect | |------|--------| | `--tasks ` | Use a specific recipe. Accepts a local path or an `http(s)://` URL (fetched over HTTP). | diff --git a/subprocess/exec.go b/subprocess/exec.go index 20f40b4..be9185c 100644 --- a/subprocess/exec.go +++ b/subprocess/exec.go @@ -79,6 +79,15 @@ type ExecCommandInput struct { type ExecError struct { Response ExecCommandResponse Err error + + // Ran is true only when the command executed to completion and + // Response.ExitCode is its real exit status. It is false when the + // command could not be started (binary not found, permission denied) + // or was cancelled, in which cases Response.ExitCode is not + // meaningful. Probe() uses this to tell a dokku-level "state absent" + // (Ran, non-zero exit) apart from a real execution failure that must + // be propagated. + Ran bool } // Error returns the wrapped error's message so existing string-based @@ -312,6 +321,10 @@ func defaultExecRunner(ctx context.Context, input ExecCommandInput) (ExecCommand res, err := cmd.Execute(ctx) if err != nil { + // The command could not be run to completion: the binary was not + // found, was not executable, or the context was cancelled. The + // exit code is not meaningful, so Ran stays false and callers such + // as Probe surface the failure instead of reading it as absence. response := ExecCommandResponse{ Command: resolved, Stdout: res.Stdout, @@ -330,7 +343,7 @@ func defaultExecRunner(ctx context.Context, input ExecCommandInput) (ExecCommand ExitCode: res.ExitCode, Cancelled: res.Cancelled, } - return response, &ExecError{Response: response, Err: errors.New(res.Stderr)} + return response, &ExecError{Response: response, Err: errors.New(res.Stderr), Ran: true} } return ExecCommandResponse{ diff --git a/subprocess/exec_test.go b/subprocess/exec_test.go index 3e8adad..3570e75 100644 --- a/subprocess/exec_test.go +++ b/subprocess/exec_test.go @@ -3,6 +3,7 @@ package subprocess import ( "bytes" "context" + "errors" "log" "strings" "testing" @@ -170,6 +171,15 @@ func TestCallExecCommandFailure(t *testing.T) { if resp.ExitCode == 0 { t.Error("expected non-zero exit code") } + // The command ran and exited non-zero, so the exit code is real: + // ExecError.Ran must be true so Probe reads it as "state absent". + var execErr *ExecError + if !errors.As(err, &execErr) { + t.Fatalf("expected *ExecError, got %T", err) + } + if !execErr.Ran { + t.Error("ExecError.Ran should be true when the command ran and exited non-zero") + } } func TestCallExecCommandNotFound(t *testing.T) { @@ -179,6 +189,16 @@ func TestCallExecCommandNotFound(t *testing.T) { if err == nil { t.Fatal("expected error for nonexistent command") } + // The command could not be started, so there is no real exit code: + // ExecError.Ran must be false so Probe propagates the failure instead + // of reporting the probed state as absent. + var execErr *ExecError + if !errors.As(err, &execErr) { + t.Fatalf("expected *ExecError, got %T", err) + } + if execErr.Ran { + t.Error("ExecError.Ran should be false when the binary is not found") + } } func TestCallExecCommandWithEnv(t *testing.T) { diff --git a/subprocess/ssh.go b/subprocess/ssh.go index 3ccb00b..36155c0 100644 --- a/subprocess/ssh.go +++ b/subprocess/ssh.go @@ -217,12 +217,18 @@ var ( ) // Probe runs input as a state probe and reports whether it matched -// (exit 0). A dokku-level non-zero exit is reported as `(false, nil)`, -// i.e. "the probed state is absent," so callers can write idempotent -// probes without unwrapping errors themselves. A transport-level -// failure (`*SSHError`) is propagated as `(false, err)` so the caller -// can short-circuit `Plan()` with `PlanResult{Error: err}` and let the -// formatter render `! ssh: ...`. +// (exit 0). A non-zero exit from a command that actually ran is reported +// as `(false, nil)`, i.e. "the probed state is absent," so callers can +// write idempotent probes without unwrapping errors themselves. +// +// Any failure that means the probe never produced a real answer is +// propagated as `(false, err)` so the caller can short-circuit `Plan()` +// with `PlanResult{Error: err}` and let the formatter render `[!]`. That +// covers a transport-level failure (`*SSHError`), a command that could +// not be executed at all (the dokku binary is missing or not +// executable), and a cancelled probe. Distinguishing "ran and said no" +// from "could not run" relies on `ExecError.Ran`, since binary-not-found +// reports `ExitCode 0` and so cannot be told apart by exit code. // // Use this for any plan-time probe that today reads exit code only // (`apps:exists`, `network:exists`, `:linked`, etc.). Probes @@ -231,11 +237,22 @@ var ( func Probe(input ExecCommandInput) (bool, error) { result, err := CallExecCommand(input) if err != nil { + // Transport-level failure (ssh connect/auth/host-key): propagate + // so the caller can render `! ssh: ...`. var sshErr *SSHError if errors.As(err, &sshErr) { return false, err } - return false, nil + // The command executed and exited non-zero: the probed state is + // absent. Report (false, nil) so idempotent probes need not unwrap. + var execErr *ExecError + if errors.As(err, &execErr) && execErr.Ran { + return false, nil + } + // Anything else - the command could not be executed (binary not + // found, permission denied) or was cancelled - is a real failure + // the caller must surface, not "state absent". + return false, err } return result.ExitCode == 0, nil } @@ -390,7 +407,10 @@ func classifySshResult(target sshTarget, remote []string, resp ExecCommandRespon } } if resp.ExitCode != 0 { - return resp, &ExecError{Response: resp, Err: errors.New(resp.Stderr)} + // The remote dokku command ran and exited non-zero (not a + // transport failure). Ran marks the exit code as authoritative so + // Probe treats it as "state absent" rather than an execution error. + return resp, &ExecError{Response: resp, Err: errors.New(resp.Stderr), Ran: true} } return resp, nil } diff --git a/subprocess/ssh_test.go b/subprocess/ssh_test.go index 9a58565..781aee2 100644 --- a/subprocess/ssh_test.go +++ b/subprocess/ssh_test.go @@ -1,6 +1,7 @@ package subprocess import ( + "context" "errors" "strings" "testing" @@ -430,6 +431,62 @@ func TestProbeSshTransportErrorPropagates(t *testing.T) { } } +func TestProbeLocalExecErrorPropagates(t *testing.T) { + // A local probe whose binary is not on PATH must surface the failure + // rather than reporting the probed state as absent. This is the + // no-dokku-installed scenario: without propagation, plan would print a + // confident [+] create for every resource instead of [!]. + matched, err := Probe(ExecCommandInput{ + Command: "nonexistent-binary-docket-test-12345", + Args: []string{"--quiet", "apps:exists", "anything"}, + }) + if matched { + t.Error("Probe should report matched=false when the binary is missing") + } + if err == nil { + t.Fatal("Probe should propagate a binary-not-found error, not swallow it as absent") + } +} + +func TestProbeExecRanFlagControlsAbsence(t *testing.T) { + // The Ran flag is the sole signal that separates "the command ran and + // exited non-zero" (state absent) from "the command could not run" + // (real failure). Inject each case via the swappable runner so the + // behaviour is pinned without spawning a process. + t.Run("ran non-zero reports absent", func(t *testing.T) { + defer SetExecRunner(func(_ context.Context, _ ExecCommandInput) (ExecCommandResponse, error) { + resp := ExecCommandResponse{ExitCode: 1} + return resp, &ExecError{Response: resp, Err: errors.New("absent"), Ran: true} + })() + + matched, err := Probe(ExecCommandInput{Command: "dokku"}) + if err != nil { + t.Fatalf("Probe should treat a ran non-zero exit as absent, got err %v", err) + } + if matched { + t.Error("Probe should report matched=false for a ran non-zero exit") + } + }) + + t.Run("cancelled probe propagates", func(t *testing.T) { + defer SetExecRunner(func(_ context.Context, _ ExecCommandInput) (ExecCommandResponse, error) { + resp := ExecCommandResponse{ExitCode: -1, Cancelled: true} + return resp, &ExecError{Response: resp, Err: context.Canceled} + })() + + matched, err := Probe(ExecCommandInput{Command: "dokku"}) + if matched { + t.Error("Probe should report matched=false on a cancelled probe") + } + if err == nil { + t.Fatal("Probe should propagate a cancelled probe, not swallow it as absent") + } + if !errors.Is(err, context.Canceled) { + t.Errorf("propagated error should wrap context.Canceled, got %v", err) + } + }) +} + func TestSetAndGetDefaultHost(t *testing.T) { t.Cleanup(func() { SetDefaultHost("") }) SetDefaultHost("alice@host:2222") diff --git a/tasks/app_lock_task.go b/tasks/app_lock_task.go index ceebe23..b14f73c 100644 --- a/tasks/app_lock_task.go +++ b/tasks/app_lock_task.go @@ -131,9 +131,10 @@ func (t AppLockTask) ExportApp(app string) ([]interface{}, error) { return []interface{}{AppLockTask{App: app, State: StatePresent}}, nil } -// appLocked checks if a dokku app is locked. Returns (false, -// *subprocess.SSHError) on transport failure; (false, nil) when dokku -// reports unlocked; (true, nil) when locked. +// appLocked checks if a dokku app is locked. Returns (false, err) when +// the probe could not run - a transport failure, a missing dokku binary, +// or a cancellation; (false, nil) when dokku reports unlocked; (true, +// nil) when locked. func appLocked(app string) (bool, error) { return subprocess.Probe(subprocess.ExecCommandInput{ Command: "dokku", diff --git a/tasks/app_task.go b/tasks/app_task.go index c179d37..3f39a1a 100644 --- a/tasks/app_task.go +++ b/tasks/app_task.go @@ -115,8 +115,9 @@ func (t AppTask) ExportApp(app string) ([]interface{}, error) { // appExists checks if an app exists. Returns (true, nil) when the app // is present, (false, nil) when dokku reports it absent, and -// (false, *subprocess.SSHError) when the probe could not reach the -// server. Plan() callers must short-circuit on the error. +// (false, err) when the probe could not run - a transport failure, a +// missing dokku binary, or a cancellation. Plan() callers must +// short-circuit on the error. func appExists(appName string) (bool, error) { return subprocess.Probe(subprocess.ExecCommandInput{ Command: "dokku", diff --git a/tasks/network_task.go b/tasks/network_task.go index 1705236..7516afc 100644 --- a/tasks/network_task.go +++ b/tasks/network_task.go @@ -164,9 +164,10 @@ func (t NetworkTask) ExportGlobal() ([]interface{}, error) { return out, nil } -// networkExists checks if a Docker network exists. Returns (false, -// *subprocess.SSHError) on transport failure; (false, nil) when dokku -// reports the network absent; (true, nil) when present. +// networkExists checks if a Docker network exists. Returns (false, err) +// when the probe could not run - a transport failure, a missing dokku +// binary, or a cancellation; (false, nil) when dokku reports the network +// absent; (true, nil) when present. func networkExists(name string) (bool, error) { return subprocess.Probe(subprocess.ExecCommandInput{ Command: "dokku", diff --git a/tasks/service_create_task.go b/tasks/service_create_task.go index 3e703ec..4864ac0 100644 --- a/tasks/service_create_task.go +++ b/tasks/service_create_task.go @@ -147,9 +147,10 @@ func (t ServiceCreateTask) Plan() PlanResult { }) } -// serviceExists checks if a dokku service exists. Returns (false, -// *subprocess.SSHError) on transport failure, (false, nil) when dokku -// reports the service absent, (true, nil) when present. +// serviceExists checks if a dokku service exists. Returns (false, err) +// when the probe could not run - a transport failure, a missing dokku +// binary, or a cancellation, (false, nil) when dokku reports the service +// absent, (true, nil) when present. func serviceExists(service, name string) (bool, error) { return subprocess.Probe(subprocess.ExecCommandInput{ Command: "dokku", diff --git a/tasks/service_link_task.go b/tasks/service_link_task.go index 8fb2eae..4642ee3 100644 --- a/tasks/service_link_task.go +++ b/tasks/service_link_task.go @@ -190,8 +190,9 @@ func (t ServiceLinkTask) Plan() PlanResult { } // serviceLinked checks if a dokku service is linked to an app. Returns -// (false, *subprocess.SSHError) on transport failure; (false, nil) when -// dokku reports no link; (true, nil) when linked. +// (false, err) when the probe could not run - a transport failure, a +// missing dokku binary, or a cancellation; (false, nil) when dokku +// reports no link; (true, nil) when linked. func serviceLinked(service, name, app string) (bool, error) { return subprocess.Probe(subprocess.ExecCommandInput{ Command: "dokku", diff --git a/tests/bats/plays.bats b/tests/bats/plays.bats index 7bd91c9..c983506 100644 --- a/tests/bats/plays.bats +++ b/tests/bats/plays.bats @@ -20,6 +20,11 @@ setup() { dokku_clean_app docket-test-play-kept dokku_clean_app docket-test-play-bail-ok dokku_clean_app docket-test-play-failfast + dokku_clean_app docket-test-playtags-deploy-api + dokku_clean_app docket-test-playtags-configure-api + dokku_clean_app docket-test-playtags-deploy-worker + dokku_clean_app docket-test-playlvltags-api + dokku_clean_app docket-test-playlvltags-worker } teardown() { @@ -31,6 +36,11 @@ teardown() { dokku_clean_app docket-test-play-kept dokku_clean_app docket-test-play-bail-ok dokku_clean_app docket-test-play-failfast + dokku_clean_app docket-test-playtags-deploy-api + dokku_clean_app docket-test-playtags-configure-api + dokku_clean_app docket-test-playtags-deploy-worker + dokku_clean_app docket-test-playlvltags-api + dokku_clean_app docket-test-playlvltags-worker } @test "plays: multi-play tasks.yml runs all plays in order" { @@ -98,6 +108,57 @@ EOF assert_output --partial '"second"' } +@test "plays: --play composes with --tags to filter tasks within the play" { + require_dokku + write_tasks_file <