From 5a435a3cba99c282bbb09df02a1b95ca044144ce Mon Sep 17 00:00:00 2001 From: "Mars.P" Date: Wed, 10 Jun 2026 15:31:48 +0800 Subject: [PATCH] Skip PostDeploy convention when the main script fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A non-zero main-script exit did not throw, so the pipeline carried on and ran the PostDeploy convention against a failed deploy. PostDeploy is where smoke tests, cache warm-up, and service-mesh registration live — running them on a deploy that already failed is worse than not running them at all. Gate PostDeploy on a single-source DeployHasFailed predicate (an upstream step threw OR the main script exited non-zero) via an opt-in skipWhenDeployFailed flag on ConventionScriptStep. PreDeploy keeps the default (false): it runs before the main script, so the failure state is never set when it is evaluated. DeployFailedConventionStep now consults the same predicate, so the skip-PostDeploy gate and the run-DeployFailed gate can never disagree about whether the deploy failed. The flag is an optional constructor parameter, so every existing call site is unchanged; only PostDeploy's behaviour on a failed main script changes. --- .../Conventions/ConventionScriptStep.cs | 14 ++- .../Conventions/DeployFailedConventionStep.cs | 10 +- .../Commands/RunScriptCommand.cs | 10 +- .../Commands/RunScriptCommandPipeline.cs | 11 ++ .../Conventions/ConventionScriptStepTests.cs | 72 +++++++++++ .../Commands/RunScriptCommandTests.cs | 118 ++++++++++++++++++ 6 files changed, 226 insertions(+), 9 deletions(-) diff --git a/src/Squid.Calamari/Commands/Conventions/ConventionScriptStep.cs b/src/Squid.Calamari/Commands/Conventions/ConventionScriptStep.cs index 31c948b1..e9627366 100644 --- a/src/Squid.Calamari/Commands/Conventions/ConventionScriptStep.cs +++ b/src/Squid.Calamari/Commands/Conventions/ConventionScriptStep.cs @@ -40,6 +40,7 @@ internal sealed class ConventionScriptStep : ExecutionStep /// Construct a convention hook bound to a script filename (without @@ -49,12 +50,18 @@ internal sealed class ConventionScriptStep : ExecutionStep - public ConventionScriptStep(string conventionName, IScriptEngine scriptEngine) + /// When true, the hook is skipped + /// once the deploy has failed () — + /// used for PostDeploy so a smoke test / traffic switch never runs against a + /// failed deploy. PreDeploy passes false (the default): it runs before + /// the main script, so the failure state is never set when it is evaluated. + public ConventionScriptStep(string conventionName, IScriptEngine scriptEngine, bool skipWhenDeployFailed = false) { if (string.IsNullOrWhiteSpace(conventionName)) throw new ArgumentException("Convention name MUST be non-empty.", nameof(conventionName)); _conventionName = conventionName; _scriptEngine = scriptEngine ?? throw new ArgumentNullException(nameof(scriptEngine)); + _skipWhenDeployFailed = skipWhenDeployFailed; } /// The convention name — exposed for test pinning + log output. @@ -68,6 +75,11 @@ public override bool IsEnabled(RunScriptCommandContext context) if (string.IsNullOrEmpty(context.WorkingDirectory)) return false; if (context.Variables is null) return false; + // PostDeploy must not run once the deploy has failed (non-zero main-script + // exit or a prior step exception). PreDeploy passes skipWhenDeployFailed=false + // and runs before the main script, so this gate never trips for it. + if (_skipWhenDeployFailed && context.DeployHasFailed) return false; + return ConventionScriptResolver.Resolve( context.WorkingDirectory, _conventionName, PreferredSyntax(context)) is not null; } diff --git a/src/Squid.Calamari/Commands/Conventions/DeployFailedConventionStep.cs b/src/Squid.Calamari/Commands/Conventions/DeployFailedConventionStep.cs index 28772453..a9efe64e 100644 --- a/src/Squid.Calamari/Commands/Conventions/DeployFailedConventionStep.cs +++ b/src/Squid.Calamari/Commands/Conventions/DeployFailedConventionStep.cs @@ -52,11 +52,11 @@ public bool IsEnabled(RunScriptCommandContext context) if (string.IsNullOrEmpty(context.WorkingDirectory)) return false; if (context.Variables is null) return false; - // Predicate: at least one of (a) something threw before cleanup - // phase, or (b) main script ran but exit code is non-zero. - var hadFailure = context.ExecutionFailed - || (context.ScriptResult is not null && context.ScriptResult.ExitCode != 0); - if (!hadFailure) return false; + // Predicate: the deploy failed — something threw before the cleanup + // phase, or the main script ran but exited non-zero. Same single source + // of truth the PostDeploy gate consults (inverted), so the two can never + // disagree about whether the deploy failed. + if (!context.DeployHasFailed) return false; return ConventionScriptResolver.Resolve( context.WorkingDirectory, ConventionName, PreferredSyntax(context)) is not null; diff --git a/src/Squid.Calamari/Commands/RunScriptCommand.cs b/src/Squid.Calamari/Commands/RunScriptCommand.cs index d028fd7b..872cf140 100644 --- a/src/Squid.Calamari/Commands/RunScriptCommand.cs +++ b/src/Squid.Calamari/Commands/RunScriptCommand.cs @@ -30,7 +30,9 @@ namespace Squid.Calamari.Commands; /// WriteBootstrappedBashScript — prepend `export VAR=` for the main script. /// ExecuteScriptWithEngine — run the operator's main script. /// PostDeploy convention (G1.5) — runs PostDeploy.sh. -/// Smoke tests, cache warm-up, service-mesh registration, etc. +/// Smoke tests, cache warm-up, service-mesh registration, etc. +/// Skipped if the deploy already failed (non-zero main-script exit +/// or a prior step exception). /// BuildRunScriptCommandResult — collect exit code + outputs. /// CleanupTemporaryFiles — best-effort cleanup (always-runs). /// @@ -79,8 +81,10 @@ public RunScriptCommand(IScriptEngine scriptEngine) new ExecuteScriptWithEngineStep(scriptEngine), // G1.5 — PostDeploy convention hook. Runs only when the package // ships a `PostDeploy.sh` file. Smoke tests, cache warm-up, - // registration with a service mesh, etc. - new ConventionScriptStep(ConventionScriptNames.PostDeploy, scriptEngine), + // registration with a service mesh, etc. skipWhenDeployFailed=true: + // a non-zero main-script exit is a failed deploy, so PostDeploy is + // skipped (a smoke test against a failed deploy is worse than none). + new ConventionScriptStep(ConventionScriptNames.PostDeploy, scriptEngine, skipWhenDeployFailed: true), new BuildRunScriptCommandResultStep(), // DeployFailed convention — IAlwaysRun (cleanup phase). Fires only // when an upstream step threw OR the main script returned non-zero. diff --git a/src/Squid.Calamari/Commands/RunScriptCommandPipeline.cs b/src/Squid.Calamari/Commands/RunScriptCommandPipeline.cs index ba86d58c..9d6a4f34 100644 --- a/src/Squid.Calamari/Commands/RunScriptCommandPipeline.cs +++ b/src/Squid.Calamari/Commands/RunScriptCommandPipeline.cs @@ -43,6 +43,17 @@ internal sealed class RunScriptCommandContext : IPathBasedExecutionContext, IVar /// public bool ExecutionFailed { get; set; } + /// + /// True once the deploy has failed: either an upstream step threw + /// () or the main script ran but returned a + /// non-zero exit code. Single source of truth for the two consumers that + /// must agree on "did the deploy fail?": the DeployFailed cleanup hook + /// (fires when true) and the PostDeploy convention (skipped when true, so a + /// smoke test / traffic switch never runs against a failed deploy). + /// + public bool DeployHasFailed + => ExecutionFailed || (ScriptResult is not null && ScriptResult.ExitCode != 0); + /// /// PR-5 — structured per-step outcomes. Each rewriter / extract / /// convention / main-script step appends one entry as it finishes. diff --git a/tests/Squid.Calamari.Tests/Calamari/Commands/Conventions/ConventionScriptStepTests.cs b/tests/Squid.Calamari.Tests/Calamari/Commands/Conventions/ConventionScriptStepTests.cs index acfc5588..28bd2bde 100644 --- a/tests/Squid.Calamari.Tests/Calamari/Commands/Conventions/ConventionScriptStepTests.cs +++ b/tests/Squid.Calamari.Tests/Calamari/Commands/Conventions/ConventionScriptStepTests.cs @@ -90,6 +90,78 @@ public void IsEnabled_WorkingDirNull_SkipsStep_NoCrash() step.IsEnabled(ctx).ShouldBeFalse(); } + // ── skipWhenDeployFailed gating (PostDeploy) ───────────────────────────── + + [Fact] + public void IsEnabled_SkipWhenDeployFailed_MainExitedNonZero_SkipsEvenWithScriptPresent() + { + // PostDeploy is wired with skipWhenDeployFailed=true. A non-zero main- + // script exit is a failed deploy → PostDeploy MUST be skipped so a smoke + // test / traffic switch never runs against a failed deploy. + File.WriteAllText(Path.Combine(_workDir, "PostDeploy.sh"), "echo hi"); + var step = new ConventionScriptStep(ConventionScriptNames.PostDeploy, new StubScriptEngine(), skipWhenDeployFailed: true); + var ctx = BuildContext(); + ctx.ScriptResult = new ScriptExecutionResult(2); + + step.IsEnabled(ctx).ShouldBeFalse(); + } + + [Fact] + public void IsEnabled_SkipWhenDeployFailed_ExecutionFailed_SkipsEvenWithScriptPresent() + { + // The other failure signal: an upstream step threw before PostDeploy. + File.WriteAllText(Path.Combine(_workDir, "PostDeploy.sh"), "echo hi"); + var step = new ConventionScriptStep(ConventionScriptNames.PostDeploy, new StubScriptEngine(), skipWhenDeployFailed: true); + var ctx = BuildContext(); + ctx.ExecutionFailed = true; + + step.IsEnabled(ctx).ShouldBeFalse(); + } + + [Fact] + public void IsEnabled_SkipWhenDeployFailed_MainSucceeded_RunsWhenScriptPresent() + { + // Happy path: main script exit 0 → PostDeploy still runs. + File.WriteAllText(Path.Combine(_workDir, "PostDeploy.sh"), "echo hi"); + var step = new ConventionScriptStep(ConventionScriptNames.PostDeploy, new StubScriptEngine(), skipWhenDeployFailed: true); + var ctx = BuildContext(); + ctx.ScriptResult = new ScriptExecutionResult(0); + + step.IsEnabled(ctx).ShouldBeTrue(); + } + + [Fact] + public void IsEnabled_DefaultFlag_IgnoresFailureState() + { + // PreDeploy passes skipWhenDeployFailed=false (the default). Even with a + // failure state on the context it stays gated only on file presence — + // proves the flag is opt-in and PreDeploy semantics are unchanged. + File.WriteAllText(Path.Combine(_workDir, "PreDeploy.sh"), "echo hi"); + var step = new ConventionScriptStep(ConventionScriptNames.PreDeploy, new StubScriptEngine()); + var ctx = BuildContext(); + ctx.ScriptResult = new ScriptExecutionResult(9); + ctx.ExecutionFailed = true; + + step.IsEnabled(ctx).ShouldBeTrue(); + } + + // ── DeployHasFailed predicate (single source of truth) ─────────────────── + + [Theory] + [InlineData(false, null, false)] // no throw, main not run → not failed + [InlineData(false, 0, false)] // main exit 0 → not failed + [InlineData(false, 1, true)] // main exit non-zero → failed + [InlineData(true, null, true)] // upstream threw → failed + [InlineData(true, 0, true)] // threw wins even if a result exists + public void DeployHasFailed_Predicate(bool executionFailed, int? exitCode, bool expected) + { + var ctx = BuildContext(); + ctx.ExecutionFailed = executionFailed; + ctx.ScriptResult = exitCode is null ? null : new ScriptExecutionResult(exitCode.Value); + + ctx.DeployHasFailed.ShouldBe(expected); + } + // ── Execute happy path ────────────────────────────────────────────────── [Fact] diff --git a/tests/Squid.Calamari.Tests/Calamari/Commands/RunScriptCommandTests.cs b/tests/Squid.Calamari.Tests/Calamari/Commands/RunScriptCommandTests.cs index 7a43061f..c4031526 100644 --- a/tests/Squid.Calamari.Tests/Calamari/Commands/RunScriptCommandTests.cs +++ b/tests/Squid.Calamari.Tests/Calamari/Commands/RunScriptCommandTests.cs @@ -42,4 +42,122 @@ public async Task ExecuteWithResultAsync_ReturnsExitCode_And_OutputVariables() Directory.Delete(tempDir, recursive: true); } } + + [Fact] + public async Task MainScriptExitsNonZero_SkipsPostDeploy_RunsDeployFailed() + { + // The core fix: a non-zero main-script exit is a failed deploy. PostDeploy + // (smoke tests / traffic switch) MUST NOT run against a failed deploy, and + // the DeployFailed hook MUST run. Real bash, real convention scripts. + if (OperatingSystem.IsWindows()) + return; + + var tempDir = Path.Combine(Path.GetTempPath(), "squid-calamari-cmd-" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(tempDir); + + var postDeploySentinel = Path.Combine(tempDir, "postdeploy-ran.txt"); + var deployFailedSentinel = Path.Combine(tempDir, "deployfailed-ran.txt"); + + try + { + File.WriteAllText(Path.Combine(tempDir, "command.sh"), "echo main-running\nexit 3\n"); + File.WriteAllText(Path.Combine(tempDir, "PostDeploy.sh"), $"echo ran > '{postDeploySentinel}'\n"); + File.WriteAllText(Path.Combine(tempDir, "DeployFailed.sh"), $"echo ran > '{deployFailedSentinel}'\n"); + + var result = await new RunScriptCommand().ExecuteWithResultAsync( + Path.Combine(tempDir, "command.sh"), Path.Combine(tempDir, "variables.json"), null, null, CancellationToken.None); + + result.ExitCode.ShouldBe(3); + result.Succeeded.ShouldBeFalse(); + + File.Exists(postDeploySentinel).ShouldBeFalse( + customMessage: "PostDeploy ran against a FAILED main script (exit 3). A smoke test / traffic " + + "switch must not run on a failed deploy — inspect ConventionScriptStep gating."); + File.Exists(deployFailedSentinel).ShouldBeTrue( + customMessage: "DeployFailed did NOT run after a non-zero main-script exit. The failure hook " + + "must fire so operators can alert / capture forensics."); + } + finally + { + if (Directory.Exists(tempDir)) + Directory.Delete(tempDir, recursive: true); + } + } + + [Fact] + public async Task MainScriptSucceeds_RunsPostDeploy_SkipsDeployFailed() + { + // Regression guard for the happy path: a successful main script still runs + // PostDeploy and must NOT trigger DeployFailed. + if (OperatingSystem.IsWindows()) + return; + + var tempDir = Path.Combine(Path.GetTempPath(), "squid-calamari-cmd-" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(tempDir); + + var postDeploySentinel = Path.Combine(tempDir, "postdeploy-ran.txt"); + var deployFailedSentinel = Path.Combine(tempDir, "deployfailed-ran.txt"); + + try + { + File.WriteAllText(Path.Combine(tempDir, "command.sh"), "echo main-running\nexit 0\n"); + File.WriteAllText(Path.Combine(tempDir, "PostDeploy.sh"), $"echo ran > '{postDeploySentinel}'\n"); + File.WriteAllText(Path.Combine(tempDir, "DeployFailed.sh"), $"echo ran > '{deployFailedSentinel}'\n"); + + var result = await new RunScriptCommand().ExecuteWithResultAsync( + Path.Combine(tempDir, "command.sh"), Path.Combine(tempDir, "variables.json"), null, null, CancellationToken.None); + + result.ExitCode.ShouldBe(0); + result.Succeeded.ShouldBeTrue(); + + File.Exists(postDeploySentinel).ShouldBeTrue( + customMessage: "PostDeploy did NOT run after a successful main script — the gate over-skipped."); + File.Exists(deployFailedSentinel).ShouldBeFalse( + customMessage: "DeployFailed ran on a SUCCESSFUL deploy — the failure predicate is wrong."); + } + finally + { + if (Directory.Exists(tempDir)) + Directory.Delete(tempDir, recursive: true); + } + } + + [Fact] + public async Task PreDeployThrows_SkipsPostDeploy_RunsDeployFailed() + { + // The other failure signal (ExecutionFailed via a thrown convention step): a + // non-zero PreDeploy aborts the deploy before the main script. PostDeploy must not + // run; DeployFailed must run. The command re-throws the original PreDeploy failure. + if (OperatingSystem.IsWindows()) + return; + + var tempDir = Path.Combine(Path.GetTempPath(), "squid-calamari-cmd-" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(tempDir); + + var postDeploySentinel = Path.Combine(tempDir, "postdeploy-ran.txt"); + var deployFailedSentinel = Path.Combine(tempDir, "deployfailed-ran.txt"); + + try + { + File.WriteAllText(Path.Combine(tempDir, "command.sh"), "echo main-running\nexit 0\n"); + File.WriteAllText(Path.Combine(tempDir, "PreDeploy.sh"), "echo pre-failing\nexit 4\n"); + File.WriteAllText(Path.Combine(tempDir, "PostDeploy.sh"), $"echo ran > '{postDeploySentinel}'\n"); + File.WriteAllText(Path.Combine(tempDir, "DeployFailed.sh"), $"echo ran > '{deployFailedSentinel}'\n"); + + await Should.ThrowAsync(() => new RunScriptCommand().ExecuteWithResultAsync( + Path.Combine(tempDir, "command.sh"), Path.Combine(tempDir, "variables.json"), null, null, CancellationToken.None)); + + File.Exists(postDeploySentinel).ShouldBeFalse( + customMessage: "PostDeploy ran after PreDeploy threw — a smoke test must not run when the deploy " + + "aborted before the main script."); + File.Exists(deployFailedSentinel).ShouldBeTrue( + customMessage: "DeployFailed did NOT run after a PreDeploy exception — the cleanup-phase failure " + + "hook must fire on the throw path too, not only on a non-zero main-script exit."); + } + finally + { + if (Directory.Exists(tempDir)) + Directory.Delete(tempDir, recursive: true); + } + } }