diff --git a/e2e/cli_exit_codes_test.go b/e2e/cli_exit_codes_test.go new file mode 100644 index 0000000000..1ffb8b6cf2 --- /dev/null +++ b/e2e/cli_exit_codes_test.go @@ -0,0 +1,123 @@ +//go:build e2e && unix +// +build e2e,unix + +// Copyright (c) 2025-2026 Netresearch DTT GmbH +// SPDX-License-Identifier: MIT + +package e2e + +import ( + "path/filepath" + "strings" + "testing" +) + +// Everything a user reaches for before ofelia ever schedules anything — the +// version banner, the help listing, `validate` in a deploy gate — is judged by +// its exit status long before anyone reads its output. These tests run the +// real binary and assert that status, because the process boundary is where +// the contract lives: a unit test can call a function and inspect its error, +// but only the binary can be wrong about what it hands back to the shell. +// +// This surface previously exited 0 for every failure, which made +// `ofelia validate --config=… || exit 1` a no-op in any pipeline that used it. + +// TestE2E_ExitCode_UnknownCommand pins that a typo is a failure. A shell +// wrapper that dispatches to ofelia has nothing else to go on. +func TestE2E_ExitCode_UnknownCommand(t *testing.T) { + t.Parallel() + + stdout, stderr, err := runCommand(t, "definitely-not-a-command") + assertExitCode(t, err, 1, stdout, stderr) + + // The listing is what tells the user what they should have typed. + if !strings.Contains(stdout+stderr, "daemon") { + t.Errorf("an unknown command should print the available commands, got:\nstdout=%s\nstderr=%s", + stdout, stderr) + } +} + +// TestE2E_ExitCode_SuccessPaths covers the other direction: asking for +// information succeeded, so these must not report failure. Getting this +// backwards would break every CI step that runs `ofelia version` as a probe. +func TestE2E_ExitCode_SuccessPaths(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + args []string + }{ + {name: "version subcommand", args: []string{"version"}}, + {name: "version flag", args: []string{"--version"}}, + {name: "help flag", args: []string{"--help"}}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + stdout, stderr, err := runCommand(t, tc.args...) + assertExitCode(t, err, 0, stdout, stderr) + + if strings.TrimSpace(stdout+stderr) == "" { + t.Errorf("%v printed nothing", tc.args) + } + }) + } +} + +// TestE2E_ExitCode_StrictValidationFails complements the INI-syntax case in +// config_validation_test.go with a file that parses but does not satisfy +// strict validation. Both have to fail, and for a deploy gate the distinction +// does not matter — what matters is that neither is silently accepted. +// +// Strict validation is opt-in (`enable-strict-validation`, default false), so +// the config turns it on. Without it ofelia accepts semantically broken jobs +// here — including an unparsable schedule, which the daemon then logs as a +// warning while starting anyway, leaving a job that never fires. That is a +// separate question from exit codes and is not pinned here. +func TestE2E_ExitCode_StrictValidationFails(t *testing.T) { + t.Parallel() + + configPath := writeConfig(t, `[global] + enable-strict-validation = true + +[job-local "broken"] + schedule = not-a-schedule + command = echo hi +`) + + stdout, stderr, err := runCommand(t, "validate", "--config="+configPath) + assertExitCode(t, err, 1, stdout, stderr) + + if !strings.Contains(stdout+stderr, "validation failed") { + t.Errorf("expected a validation failure message, got:\nstdout=%s\nstderr=%s", stdout, stderr) + } +} + +// TestE2E_ExitCode_ValidateIsUsableAsAGate is the test that would have caught +// the original defect on its own: it uses validate exactly as a deployment +// pipeline does — run it, branch on the status — over a good and a bad config, +// and requires the two to be distinguishable. +func TestE2E_ExitCode_ValidateIsUsableAsAGate(t *testing.T) { + t.Parallel() + + good := writeConfig(t, `[global] + log-level = info + +[job-local "hello"] + schedule = @every 30s + command = echo hello +`) + bad := filepath.Join(t.TempDir(), "missing.ini") + + _, _, goodErr := runCommand(t, "validate", "--config="+good) + _, _, badErr := runCommand(t, "validate", "--config="+bad) + + if goodErr != nil { + t.Errorf("a valid config was rejected: %v", goodErr) + } + if badErr == nil { + t.Error("a missing config was accepted; validate cannot be used as a gate") + } +} diff --git a/e2e/config_validation_test.go b/e2e/config_validation_test.go index 39f4a3df12..dec1bb123e 100644 --- a/e2e/config_validation_test.go +++ b/e2e/config_validation_test.go @@ -24,11 +24,11 @@ func TestE2E_Validate_MalformedINI(t *testing.T) { stdout, stderr, err := runCommand(t, "validate", "--config="+configPath) - // Note on exit code: ofelia.go intentionally calls `return` instead of - // os.Exit(1) after go-flags reports the error (see ofelia.go ~L132), - // so the process exits 0. We therefore assert on the human-readable - // error text — that's what the user actually sees in CI logs. - _ = err + // The exit status is what a pipeline reads. This used to be discarded here + // with a note calling the exit-0 behavior intentional, which meant the + // test documented the defect instead of catching it: `ofelia validate … + // || exit 1` could never fire on a broken config. + assertExitCode(t, err, 1, stdout, stderr) combined := stdout + stderr for _, needle := range []string{ @@ -49,7 +49,9 @@ func TestE2E_Validate_MissingConfigFile(t *testing.T) { t.Parallel() missingPath := filepath.Join(t.TempDir(), "does-not-exist.ini") - stdout, stderr, _ := runCommand(t, "validate", "--config="+missingPath) + stdout, stderr, err := runCommand(t, "validate", "--config="+missingPath) + + assertExitCode(t, err, 1, stdout, stderr) combined := stdout + stderr for _, needle := range []string{ @@ -84,7 +86,11 @@ func TestE2E_Validate_AcceptsValidConfig(t *testing.T) { ` configPath := writeConfig(t, configBody) - stdout, stderr, _ := runCommand(t, "validate", "--config="+configPath) + stdout, stderr, err := runCommand(t, "validate", "--config="+configPath) + + // The counterpart to the failure cases: a good config must exit 0, or a + // pipeline that gates on validate would reject every deployment. + assertExitCode(t, err, 0, stdout, stderr) // JSON dump should mention both jobs we defined. for _, needle := range []string{`"hello"`, `"world"`, `"Image": "alpine:3.20"`} { diff --git a/e2e/helpers_test.go b/e2e/helpers_test.go index dbbe203d73..6e2cee476f 100644 --- a/e2e/helpers_test.go +++ b/e2e/helpers_test.go @@ -345,3 +345,26 @@ func dockerRemove(t *testing.T, name string) { cmd.Stderr = io.Discard _ = cmd.Run() } + +// assertExitCode fails the test unless the command exited with want. +// +// The exit status is the only thing a shell, a Makefile or a CI step can act +// on, so it is asserted separately from the human-readable output: a command +// that prints a clear error and still exits 0 is indistinguishable from +// success to everything except a person reading the log. +func assertExitCode(t *testing.T, runErr error, want int, stdout, stderr string) { + t.Helper() + + got := 0 + if runErr != nil { + var exitErr *exec.ExitError + if !errors.As(runErr, &exitErr) { + t.Fatalf("command failed to run at all: %v", runErr) + } + got = exitErr.ExitCode() + } + + if got != want { + t.Errorf("exit code = %d, want %d\nstdout=%s\nstderr=%s", got, want, stdout, stderr) + } +} diff --git a/ofelia.go b/ofelia.go index 1d08de20e8..81dae7e5c2 100644 --- a/ofelia.go +++ b/ofelia.go @@ -43,14 +43,28 @@ func buildLogger(level string) (*slog.Logger, *slog.LevelVar) { return slog.New(handler), levelVar } +// Exit codes. A command that failed has to say so in the only channel a shell +// reads, otherwise `ofelia validate … || exit 1` in a pipeline never fires and +// a broken config sails through the gate that exists to stop it. +const ( + exitOK = 0 + exitFailure = 1 +) + func main() { + os.Exit(run(os.Args[1:])) +} + +// run holds what main used to do and returns the process exit code instead of +// ending the process, so the exit status is a value tests can assert on. +func run(args []string) int { cli.Version = version cli.Build = build // Handle --version flag before parser setup - if slices.Contains(os.Args[1:], "--version") { + if slices.Contains(args, "--version") { _, _ = fmt.Fprintln(os.Stdout, cli.VersionString()) - return + return exitOK } // Pre-parse log-level flag to configure logger early @@ -58,7 +72,6 @@ func main() { LogLevel string `long:"log-level"` ConfigFile string `long:"config" default:"/etc/ofelia/config.ini"` } - args := os.Args[1:] preParser := flags.NewParser(&pre, flags.IgnoreUnknown) _, _ = preParser.ParseArgs(args) @@ -118,8 +131,10 @@ func main() { ) if _, err := parser.ParseArgs(args); err != nil { + // Help was asked for and printed. That is the command doing its job, + // not a failure. if flags.WroteHelp(err) { - return + return exitOK } var flagErr *flags.Error @@ -128,7 +143,14 @@ func main() { _, _ = fmt.Fprintf(os.Stdout, "\n%s\n", cli.VersionString()) } - logger.Error("Command failed to execute") - return // Exit gracefully instead of os.Exit(1) + // Every other error — an unusable config, a subcommand that returned + // an error, an unknown command — is a failure and has to leave a + // non-zero status behind. This used to return 0 with a logged message, + // which meant `ofelia validate … || exit 1` could not fire and a + // broken config passed the gate meant to catch it. + logger.Error("Command failed to execute", "error", err) + return exitFailure } + + return exitOK } diff --git a/ofelia_main_test.go b/ofelia_main_test.go index 3e9a944f24..76f1789cb2 100644 --- a/ofelia_main_test.go +++ b/ofelia_main_test.go @@ -5,31 +5,29 @@ package main import ( "os" + "path/filepath" "strings" "testing" ) -// main() is reachable from a test because every one of its exits is a plain -// return: the flag-error path was deliberately changed to return instead of -// calling os.Exit(1) (see the comment at its final return). Exercising it here -// covers the command wiring — a command dropped from the parser, or a -// constructor that starts panicking, fails these tests instead of only showing -// up when a user runs the binary. +// run() is what main() does, minus the call to os.Exit, so a test can assert +// on the exit code the process would have produced. Exercising it here covers +// the command wiring — a command dropped from the parser, or a constructor +// that starts panicking, fails these tests instead of only showing up when a +// user runs the binary — and, since the exit code is the only thing a shell +// reads, that each outcome maps to the right status. // // These tests never pass a real command such as `daemon`, which would start a // scheduler. Only paths that parse and return are used. -// runMain invokes main() with the given argv while redirecting stdout, so the -// parser's help output does not drown the test log. It restores both os.Args -// and os.Stdout before returning and reports what main() printed. -func runMain(t *testing.T, argv ...string) string { +// runMain invokes run() with the given argv while redirecting stdout, so the +// parser's help output does not drown the test log. It restores os.Stdout +// before returning and reports both what was printed and the exit code. +func runMain(t *testing.T, argv ...string) (string, int) { t.Helper() - origArgs, origStdout := os.Args, os.Stdout - t.Cleanup(func() { - os.Args = origArgs - os.Stdout = origStdout - }) + origStdout := os.Stdout + t.Cleanup(func() { os.Stdout = origStdout }) r, w, err := os.Pipe() if err != nil { @@ -38,7 +36,7 @@ func runMain(t *testing.T, argv ...string) string { // A pipe's buffer is finite and the help text is long, so drain it // concurrently; writing more than the buffer holds would otherwise block - // main() forever. + // run() forever. captured := make(chan string, 1) go func() { var sb strings.Builder @@ -55,39 +53,46 @@ func runMain(t *testing.T, argv ...string) string { captured <- sb.String() }() - os.Args = append([]string{"ofelia"}, argv...) os.Stdout = w - main() + code := run(argv) _ = w.Close() out := <-captured _ = r.Close() - return out + return out, code } // TestMain_VersionFlag covers the short-circuit before the parser is built: // --version must print and return without constructing any command. // -//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global +//nolint:paralleltest // replaces os.Stdout, which is process-global func TestMain_VersionFlag(t *testing.T) { - // No t.Parallel(): os.Args and os.Stdout are process-global. - out := runMain(t, "--version") + // No t.Parallel(): os.Stdout is process-global. + out, code := runMain(t, "--version") if strings.TrimSpace(out) == "" { t.Error("--version printed nothing") } + if code != exitOK { + t.Errorf("--version exit code = %d, want %d", code, exitOK) + } } // TestMain_Help covers the full command-registration path and the // flags.WroteHelp branch: every AddCommand call runs before the parser reports // that it wrote help. // -//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global +//nolint:paralleltest // replaces os.Stdout, which is process-global func TestMain_Help(t *testing.T) { - out := runMain(t, "--help") + out, code := runMain(t, "--help") + + // Asking for help and getting it is the command succeeding, not failing. + if code != exitOK { + t.Errorf("--help exit code = %d, want %d", code, exitOK) + } // Each registered command should appear in the help output. This is what - // turns the test from "main did not panic" into a check that the command + // turns the test from "run did not panic" into a check that the command // set is intact. for _, cmd := range []string{"daemon", "validate", "config", "init", "doctor", "hash-password", "version"} { if !strings.Contains(out, cmd) { @@ -99,47 +104,66 @@ func TestMain_Help(t *testing.T) { // TestMain_UnknownCommand covers the flags.Error branch, which prints help plus // the version string and returns rather than exiting the process. // -//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global +//nolint:paralleltest // replaces os.Stdout, which is process-global func TestMain_UnknownCommand(t *testing.T) { - out := runMain(t, "definitely-not-a-command") + out, code := runMain(t, "definitely-not-a-command") if !strings.Contains(out, "daemon") { t.Errorf("an unknown command should print the help listing; got %q", out) } + // The status is the only part a shell can act on. + if code != exitFailure { + t.Errorf("unknown command exit code = %d, want %d", code, exitFailure) + } } // TestMain_NoArguments covers the same error branch reached with no command at // all, which is what a bare `ofelia` invocation does. // -//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global +//nolint:paralleltest // replaces os.Stdout, which is process-global func TestMain_NoArguments(t *testing.T) { - out := runMain(t) + out, code := runMain(t) if strings.TrimSpace(out) == "" { t.Error("a bare invocation printed nothing; expected the help listing") } + if code != exitFailure { + t.Errorf("bare invocation exit code = %d, want %d (no command was given)", code, exitFailure) + } } -// TestMain_LogLevelFlagIsPreParsed pins that --log-level is consumed by the -// pre-parser before the real parser runs, so an early log level applies to -// everything the commands log. It reaches the same help path, but with the -// pre-parse branch populated. +// TestMain_LogLevelFromConfigFile covers the pre-parse branch that reads the +// log level out of the INI when no --log-level flag was given, which is how a +// containerised deployment configures it. // -//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global -func TestMain_LogLevelFlagIsPreParsed(t *testing.T) { - out := runMain(t, "--log-level", "debug", "--help") - if !strings.Contains(out, "daemon") { - t.Errorf("expected help output with a log level set; got %q", out) +// The flag is passed after the subcommand on purpose: --config and --log-level +// are pre-parsed from anywhere in argv, but the top-level parser does not +// declare them, so `ofelia --config=x validate` is rejected as an unknown flag +// while `ofelia validate --config=x` works. That placement asymmetry is a +// separate defect, not something this test should bake in. +// +//nolint:paralleltest // replaces os.Stdout, which is process-global +func TestMain_LogLevelFromConfigFile(t *testing.T) { + iniPath := filepath.Join(t.TempDir(), "ofelia.ini") + body := "[global]\n log-level = debug\n\n[job-local \"noop\"]\n schedule = @every 1h\n command = true\n" + if err := os.WriteFile(iniPath, []byte(body), 0o600); err != nil { + t.Fatalf("writing config: %v", err) + } + + out, code := runMain(t, "validate", "--config="+iniPath) + if code != exitOK { + t.Errorf("validating a good config exited %d, want %d; output:\n%s", code, exitOK, out) } } -// TestMain_ConfigFlagMissingFileIsTolerated pins that a --config path which -// does not exist does not stop startup: the INI load is best-effort and only -// supplies a log level. +// TestMain_ValidateMissingConfigFails pins the status a scripted caller reads: +// a config file that is not there is a failure, and `ofelia validate … || …` +// has to be able to see it. // -//nolint:paralleltest // mutates os.Args and os.Stdout, which are process-global -func TestMain_ConfigFlagMissingFileIsTolerated(t *testing.T) { - missing := t.TempDir() + "/no-such-config.ini" - out := runMain(t, "--config", missing, "--help") - if !strings.Contains(out, "daemon") { - t.Errorf("a missing --config file should be tolerated; got %q", out) +//nolint:paralleltest // replaces os.Stdout, which is process-global +func TestMain_ValidateMissingConfigFails(t *testing.T) { + missing := filepath.Join(t.TempDir(), "no-such-config.ini") + + _, code := runMain(t, "validate", "--config="+missing) + if code != exitFailure { + t.Errorf("validating a missing config exited %d, want %d", code, exitFailure) } }