diff --git a/CHANGELOG.md b/CHANGELOG.md index 054aae9..2a26bce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,30 @@ A `Migration` section is added to any release that bumps `schema_version`. ### Fixed +- **promote:** A matrix-based promote deploy (a deploy declaring inputs) now + threads the per-promotion environment and sha to its reusable-workflow + callback, matching orchestrate. The matrix path previously emitted only the + declared manifest inputs, so the callback ran with an empty environment while + the job name referenced `${{ matrix.environment }}`, a key the matrix builder + never set. environment and sha are added to every matrix entry and passed in + the `with:` block (sha only when the callback declares it, and neither when the + manifest already wires it as an explicit input). +- **rollback:** The repository_dispatch dry-run guard now treats a JSON boolean + `true` from `client_payload` and the string `'true'` from a workflow_dispatch + input alike. A bare `!= 'true'` compared a boolean against a string, which + GitHub Actions coerces numerically, so a natural `{"dry_run": true}` payload + read as not-a-dry-run and a dry-run rollback ran real deploys and wrote + rolled-back state. Without the trigger the output is unchanged. +- **generate:** A dependent deploy is now gated on the base deploy's effective + result (the base job or any retry shim succeeded), so a base deploy rescued by + a retry no longer skips the deploys that depend on it. The condition was + reading the base job's immutable `result`, frozen at `failure` after a rescued + attempt, and a deploy-on-deploy dependency also emitted the clause twice. +- **promote:** A dry-run promote no longer creates a real GitHub Deployment, and + the Deployment's terminal status no longer counts a legitimately skipped deploy + as a failure or omits the prod deploy result. The native-deployment lifecycle + steps are gated on a non-dry-run run, and the status expression judges each + deploy (including its prod job) on success-or-skipped. - **state:** A single-component (flat) state write on a manifest that carries per-component state no longer destroys that state. The flat state map models environments only, so parsing a component-scoped manifest lifted diff --git a/e2e/scenarios/31-native-deployments.yaml b/e2e/scenarios/31-native-deployments.yaml index 3e15502..313d560 100644 --- a/e2e/scenarios/31-native-deployments.yaml +++ b/e2e/scenarios/31-native-deployments.yaml @@ -49,13 +49,15 @@ steps: contains: - " deployments: write" - " - name: Create deployment" - - " if: ${{ github.server_url == 'https://github.com' }}" + # A dry-run run skips its deploys, so it must not create a real + # Deployment: every lifecycle step is also gated on a non-dry-run run. + - " if: ${{ github.server_url == 'https://github.com' && github.event.inputs.dry_run != 'true' }}" - " deployment_id=$(gh api repos/${{ github.repository }}/deployments \\" - " --field auto_inactive=false \\" - " - name: Set deployment in_progress" - " --field state=in_progress" - " - name: Set deployment status" - - " if: ${{ github.server_url == 'https://github.com' && always() }}" + - " if: ${{ github.server_url == 'https://github.com' && github.event.inputs.dry_run != 'true' && always() }}" - " production) environment_url='https://app.example.com' ;;" - name: "Regenerate and confirm no drift" diff --git a/e2e/scenarios/32-rollback-repository-dispatch.yaml b/e2e/scenarios/32-rollback-repository-dispatch.yaml index 62d9552..dd0d829 100644 --- a/e2e/scenarios/32-rollback-repository-dispatch.yaml +++ b/e2e/scenarios/32-rollback-repository-dispatch.yaml @@ -59,6 +59,12 @@ steps: # deploy guard and finalize gate coalesce dry_run and deployable. - "github.event.inputs.dry_run || github.event.client_payload.dry_run" - "github.event.inputs.deployable || github.event.client_payload.deployable" + # The dry_run guard matches both a JSON boolean true (client_payload) + # and the string 'true' (workflow_dispatch input): a natural + # {"dry_run": true} payload sends a boolean, and a bare "!= 'true'" + # would read it as not-a-dry-run and run real deploys. + - "(github.event.inputs.dry_run || github.event.client_payload.dry_run) != true" + - "(github.event.inputs.dry_run || github.event.client_payload.dry_run) != 'true'" not_contains: # the bare, un-coalesced reads must be gone once the toggle is on. - "ENVIRONMENT: ${{ github.event.inputs.environment }}" diff --git a/e2e/scenarios/43-deploy-rollout-strategy.yaml b/e2e/scenarios/43-deploy-rollout-strategy.yaml index a49e046..7775966 100644 --- a/e2e/scenarios/43-deploy-rollout-strategy.yaml +++ b/e2e/scenarios/43-deploy-rollout-strategy.yaml @@ -72,3 +72,8 @@ steps: contains: - "fail-fast: true" - "max-parallel: 2" + # The matrix deploy must thread the per-promotion environment and sha + # to its callback (the way orchestrate does), or the deploy targets an + # empty environment while its job name references matrix.environment. + - "environment: ${{ matrix.environment }}" + - "sha: ${{ matrix.sha }}" diff --git a/internal/generate/generator.go b/internal/generate/generator.go index 2422b47..7cba61f 100644 --- a/internal/generate/generator.go +++ b/internal/generate/generator.go @@ -1264,6 +1264,9 @@ func (g *Generator) writeStrategyBlock(sb *strings.Builder, m *config.MatrixConf func (g *Generator) writeIfCondition(sb *strings.Builder, info CallbackInfo, needs []string) { var conditions []string buildLinkedDeploy := false + // Dependencies already emitted by the build-linked path below, so the + // general dependency loop does not emit a second, duplicate clause for them. + linkedDeps := make(map[string]struct{}) // For deploys with depends_on build, check if build ran successfully // instead of using setup detection @@ -1278,14 +1281,20 @@ func (g *Generator) writeIfCondition(sb *strings.Builder, info CallbackInfo, nee if err != nil { continue } - // Apply run_policy to build dependency check + // Record so the general dependency loop below does not emit + // a second, duplicate clause for the same dependency. + linkedDeps[depJobID] = struct{}{} + depRetries := g.graph.Nodes[depJobID].Retries + // Apply run_policy to the dependency check, judged on the + // dependency's effective result (base OR any retry shim + // succeeded) so a rescued deploy does not skip its dependents. switch info.RunPolicy { case config.RunPolicyAlways: - conditions = append(conditions, fmt.Sprintf("(needs.%s.result == 'success' || needs.%s.result == 'skipped')", depJobID, depJobID)) + conditions = append(conditions, fmt.Sprintf("(%s || needs.%s.result == 'skipped')", effectiveSuccessCond(depJobID, depRetries), depJobID)) case config.RunPolicyForce: // No condition needed for force default: - conditions = append(conditions, fmt.Sprintf("needs.%s.result == 'success'", depJobID)) + conditions = append(conditions, effectiveDepSuccessGate(depJobID, depRetries)) } } break @@ -1309,12 +1318,17 @@ func (g *Generator) writeIfCondition(sb *strings.Builder, info CallbackInfo, nee if buildLinkedDeploy && depInfo.Type == config.CallbackTypeBuild { continue } + // Skip any dependency the build-linked path already emitted, so a + // deploy-on-deploy dependency is not gated by a duplicated clause. + if _, done := linkedDeps[depJobID]; done { + continue + } switch info.RunPolicy { case config.RunPolicyDefault, "": - conditions = append(conditions, fmt.Sprintf("needs.%s.result == 'success'", depJobID)) + conditions = append(conditions, effectiveDepSuccessGate(depJobID, depInfo.Retries)) case config.RunPolicyAlways: - conditions = append(conditions, fmt.Sprintf("(needs.%s.result == 'success' || needs.%s.result == 'skipped')", depJobID, depJobID)) + conditions = append(conditions, fmt.Sprintf("(%s || needs.%s.result == 'skipped')", effectiveSuccessCond(depJobID, depInfo.Retries), depJobID)) case config.RunPolicyForce: // No dependency condition } @@ -1733,7 +1747,11 @@ func (g *Generator) writeNativeDeploymentSteps(sb *strings.Builder, sorted []str resultExpr = fmt.Sprintf("${{ (%s) && 'success' || 'failure' }}", strings.Join(conds, " && ")) } - writeNativeDeploymentSteps(sb, g.config, envExpr, resultExpr, " ") + // A dry-run orchestrate skips its deploy callbacks, so it must not create a + // real GitHub Deployment either. github.event.inputs.dry_run is null-safe on + // the non-dispatch triggers (push/schedule/workflow_run), where it renders + // empty and reads as not-a-dry-run. + writeNativeDeploymentSteps(sb, g.config, envExpr, resultExpr, "github.event.inputs.dry_run != 'true'", " ") } func (g *Generator) writeSummaryStep(sb *strings.Builder, sorted []string) { @@ -2118,6 +2136,22 @@ func effectiveSuccessCond(jobName string, retries int) string { return cond } +// effectiveDepSuccessGate renders the "dependency satisfied" clause for a +// downstream job's if: condition, judged on the dependency's effective result so +// a base deploy that failed but was rescued by a retry shim still lets its +// dependents run. It reuses effectiveSuccessCond (the shared retry-aware helper) +// and parenthesizes the disjunction only when a retry ladder is present, since +// callers join these clauses with " && "; with no retries it collapses to the +// bare needs..result == 'success', so a manifest without retries emits +// byte-identical output. +func effectiveDepSuccessGate(jobName string, retries int) string { + cond := effectiveSuccessCond(jobName, retries) + if retries > 0 { + return "(" + cond + ")" + } + return cond +} + // effectiveResultExpr renders a callback's effective result as a ${{ }} // expression evaluating to the string 'success' or 'failure', suitable for an // env: value that shell then compares against "success". diff --git a/internal/generate/native_deployments.go b/internal/generate/native_deployments.go index acd6b70..87f39ac 100644 --- a/internal/generate/native_deployments.go +++ b/internal/generate/native_deployments.go @@ -31,20 +31,33 @@ func deploymentAutoInactive(cfg *config.TrunkConfig) bool { // envExpr is the shell-safe expression that resolves to the target environment // name at run time (it differs between the orchestrate and promote seams). // resultExpr is the shell expression that evaluates to "success" or "failure" -// for the deploy outcome. indent is the per-step indent (matching the -// surrounding generated YAML). -func writeNativeDeploymentSteps(sb *strings.Builder, cfg *config.TrunkConfig, envExpr, resultExpr, indent string) { +// for the deploy outcome. dryRunGuard is the expression body (no ${{ }} wrapper) +// that is true when the run is NOT a dry run; a dry run must never create a real +// GitHub Deployment, so it is ANDed into every step's if:. An empty dryRunGuard +// leaves the steps ungated (no dry-run concept for that seam). indent is the +// per-step indent (matching the surrounding generated YAML). +func writeNativeDeploymentSteps(sb *strings.Builder, cfg *config.TrunkConfig, envExpr, resultExpr, dryRunGuard, indent string) { if !nativeDeploymentsEnabled(cfg) { return } body := indent + " " + // Compose the server guard with the non-dry-run guard so a dry run neither + // creates the Deployment nor posts status against a Deployment that was never + // created. serverGuard is the bare comparison (no ${{ }} wrapper) so it can + // be joined with the other clauses inside a single expression. + serverGuard := stripTokenExprWrapper(appTokenServerGuard) + createGuard := "${{ " + serverGuard + " }}" + if dryRunGuard != "" { + createGuard = "${{ " + serverGuard + " && " + dryRunGuard + " }}" + } + // Create the Deployment. The environment name is resolved at run time, so the // URL lookup is a shell case over the configured environment_config entries. sb.WriteString(indent + "- name: Create deployment\n") sb.WriteString(body + "id: cascade-deployment\n") - sb.WriteString(body + "if: " + appTokenServerGuard + "\n") + sb.WriteString(body + "if: " + createGuard + "\n") sb.WriteString(body + "env:\n") sb.WriteString(body + " GH_TOKEN: ${{ github.token }}\n") sb.WriteString(body + "run: |\n") @@ -61,7 +74,7 @@ func writeNativeDeploymentSteps(sb *strings.Builder, cfg *config.TrunkConfig, en // Mark the deployment in_progress. sb.WriteString(indent + "- name: Set deployment in_progress\n") - sb.WriteString(body + "if: " + appTokenServerGuard + "\n") + sb.WriteString(body + "if: " + createGuard + "\n") sb.WriteString(body + "env:\n") sb.WriteString(body + " GH_TOKEN: ${{ github.token }}\n") sb.WriteString(body + "run: |\n") @@ -72,8 +85,12 @@ func writeNativeDeploymentSteps(sb *strings.Builder, cfg *config.TrunkConfig, en // Report the terminal status. always() lets it run even when a deploy failed, // so the Deployment never sticks at in_progress. The URL is selected by the // runtime environment name from the configured environment_config entries. + statusGuard := serverGuard + if dryRunGuard != "" { + statusGuard += " && " + dryRunGuard + } sb.WriteString(indent + "- name: Set deployment status\n") - sb.WriteString(body + "if: ${{ " + stripTokenExprWrapper(appTokenServerGuard) + " && always() }}\n") + sb.WriteString(body + "if: ${{ " + statusGuard + " && always() }}\n") sb.WriteString(body + "env:\n") sb.WriteString(body + " GH_TOKEN: ${{ github.token }}\n") sb.WriteString(body + "run: |\n") diff --git a/internal/generate/native_deployments_test.go b/internal/generate/native_deployments_test.go index 52f75cd..52e4cb0 100644 --- a/internal/generate/native_deployments_test.go +++ b/internal/generate/native_deployments_test.go @@ -32,7 +32,7 @@ on: require.NoError(t, os.WriteFile(filepath.Join(tmpDir, ".github/workflows/deploy.yaml"), []byte(deployWorkflow), 0o644)) cfg := &config.TrunkConfig{ - TrunkBranch: "main", + TrunkBranch: "main", Environments: []config.EnvironmentEntry{ {Name: "production", EnvironmentConfig: config.EnvironmentConfig{EnvironmentURL: "https://app.example.com"}}, }, @@ -63,8 +63,12 @@ func TestNativeDeployments_Enabled(t *testing.T) { "must POST to the deployment statuses collection") assert.Contains(t, out, "https://app.example.com", "the configured environment_url must be wired into the status update") - assert.Contains(t, out, appTokenServerGuard, + assert.Contains(t, out, "github.server_url == 'https://github.com'", "deployment steps must be guarded to real GitHub via the server_url guard") + // A dry-run orchestrate skips its deploys, so it must not create a real + // Deployment: every lifecycle step is also gated on a non-dry-run run. + assert.Contains(t, out, "github.server_url == 'https://github.com' && github.event.inputs.dry_run != 'true'", + "deployment steps must not run on a dry-run orchestrate") // The terminal status reflects the deploy job result, not a hardcoded value. assert.Contains(t, out, "needs.deploy-app.result", "terminal status must derive from the deploy job result") diff --git a/internal/generate/pass10_silent_output_test.go b/internal/generate/pass10_silent_output_test.go new file mode 100644 index 0000000..1ca5617 --- /dev/null +++ b/internal/generate/pass10_silent_output_test.go @@ -0,0 +1,265 @@ +package generate + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stablekernel/cascade/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// pass10DeployWorkflow is a deploy callback that declares the standard +// environment and sha inputs plus a custom region input, so a manifest deploy +// with inputs takes the matrix path and the framework may thread environment/sha. +const pass10DeployWorkflow = `name: Deploy +on: + workflow_call: + inputs: + environment: + type: string + sha: + type: string + region: + type: string +` + +func pass10Fixture(t *testing.T, workflow string) string { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".github/workflows"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, ".github/workflows/deploy.yaml"), []byte(workflow), 0o644)) + return dir +} + +// pass10JobBlock returns the lines of the named job, from its " :" header up to +// the next top-level job header (a line starting with two spaces then a word +// then a colon at the same indent), so an assertion can scope to one job. +func pass10JobBlock(t *testing.T, content, jobID string) string { + t.Helper() + lines := strings.Split(content, "\n") + start := -1 + header := " " + jobID + ":" + for i, l := range lines { + if l == header { + start = i + break + } + } + require.GreaterOrEqual(t, start, 0, "job %q not found", jobID) + end := len(lines) + for i := start + 1; i < len(lines); i++ { + l := lines[i] + if len(l) > 2 && l[0] == ' ' && l[1] == ' ' && l[2] != ' ' && strings.HasSuffix(strings.TrimSpace(l), ":") && !strings.HasPrefix(l, " ") { + end = i + break + } + } + return strings.Join(lines[start:end], "\n") +} + +// TestGB2_PromoteMatrixDeploy_ThreadsEnvironmentAndSha proves the promote matrix +// deploy path passes the per-promotion environment and sha to its callback the +// way orchestrate does. Before the fix the matrix with: block carried only +// declared manifest inputs, so a promote deployed to an empty environment while +// the job name referenced ${{ matrix.environment }}, a key the matrix builder +// never set. +func TestGB2_PromoteMatrixDeploy_ThreadsEnvironmentAndSha(t *testing.T) { + // The manifest references the callback by a BARE filename (deploy.yaml), the + // same way the e2e scenarios do. Declared-input detection must resolve it to + // the canonical .github/workflows/deploy.yaml where the file is staged; a + // fully-qualified path here would mask the resolution bug that dropped sha. + dir := pass10Fixture(t, pass10DeployWorkflow) + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev", "prod"), + Deploys: []config.DeployConfig{{ + Name: "web", Workflow: "deploy.yaml", Triggers: []string{"src/**"}, + Inputs: map[string]interface{}{"region": "us-east-1"}, + }}, + } + out, err := NewPromoteGenerator(cfg, dir).Generate() + require.NoError(t, err) + + block := pass10JobBlock(t, out, "deploy-web") + assert.Contains(t, block, "environment: ${{ matrix.environment }}", + "matrix deploy must thread the per-promotion environment to the callback") + assert.Contains(t, block, "sha: ${{ matrix.sha }}", + "matrix deploy must thread sha when the callback declares it") + + // The matrix builder must inject environment/sha onto each entry so the job + // name and the with: inputs resolve to real values. + assert.Contains(t, out, "'. + {environment: $env, sha: $sha}'", + "matrix builder must carry environment and sha on every matrix entry") +} + +// TestGB2_PromoteMatrixDeploy_OmitsShaWhenUndeclared keeps sha gated on +// declaration, matching orchestrate: a callback that does not declare sha must +// not receive it (an undeclared reusable-workflow input is a hard error). +func TestGB2_PromoteMatrixDeploy_OmitsShaWhenUndeclared(t *testing.T) { + wf := "name: Deploy\non:\n workflow_call:\n inputs:\n environment:\n type: string\n region:\n type: string\n" + dir := pass10Fixture(t, wf) + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev", "prod"), + Deploys: []config.DeployConfig{{ + Name: "web", Workflow: "deploy.yaml", Triggers: []string{"src/**"}, + Inputs: map[string]interface{}{"region": "us-east-1"}, + }}, + } + out, err := NewPromoteGenerator(cfg, dir).Generate() + require.NoError(t, err) + block := pass10JobBlock(t, out, "deploy-web") + assert.Contains(t, block, "environment: ${{ matrix.environment }}") + assert.NotContains(t, block, "sha: ${{ matrix.sha }}", + "sha must not be threaded to a callback that does not declare it") +} + +// TestGB2_PromoteMatrixDeploy_NoDuplicateShaWhenManifestInput proves that when +// the manifest already wires sha as an explicit deploy input, the with: block +// emits it exactly once (from the matrix-input loop) and the framework does not +// add a second sha key. +func TestGB2_PromoteMatrixDeploy_NoDuplicateShaWhenManifestInput(t *testing.T) { + dir := pass10Fixture(t, pass10DeployWorkflow) + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev", "prod"), + Deploys: []config.DeployConfig{{ + Name: "web", Workflow: "deploy.yaml", Triggers: []string{"src/**"}, + Inputs: map[string]interface{}{"region": "us-east-1", "sha": "${{ matrix.sha }}"}, + }}, + } + out, err := NewPromoteGenerator(cfg, dir).Generate() + require.NoError(t, err) + block := pass10JobBlock(t, out, "deploy-web") + assert.Equal(t, 1, strings.Count(block, "sha: ${{ matrix.sha }}"), + "sha must be emitted exactly once, not duplicated, when it is a manifest input") +} + +// TestGM4_RollbackDispatch_DryRunTreatsBooleanAndString proves the rollback +// deploy guard and finalize gate treat a JSON boolean true (client_payload) and +// the string 'true' (workflow_dispatch) alike. Before the fix a bare "!= 'true'" +// read a boolean true as not-a-dry-run, so a dry-run rollback ran real deploys. +func TestGM4_RollbackDispatch_DryRunTreatsBooleanAndString(t *testing.T) { + dir := pass10Fixture(t, pass10DeployWorkflow) + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev", "prod"), + Deploys: []config.DeployConfig{{ + Name: "app", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, + }}, + Rollback: &config.RollbackConfig{RepositoryDispatch: &config.RepositoryDispatchTrigger{Types: []string{"rollback-requested"}}}, + } + out, err := NewRollbackGenerator(cfg, dir).Generate() + require.NoError(t, err) + + coalesced := "github.event.inputs.dry_run || github.event.client_payload.dry_run" + // Both the boolean and string forms of dry_run must be excluded. + assert.Contains(t, out, "("+coalesced+") != true", + "guard must treat a JSON boolean true as a dry run") + assert.Contains(t, out, "("+coalesced+") != 'true'", + "guard must treat the string 'true' as a dry run") + // The old, single-form guard must be gone. + assert.NotContains(t, out, "("+coalesced+") != 'true' && (github.event.inputs.deployable", + "the boolean-blind single-comparison guard must be replaced") +} + +// TestGM4_Rollback_NoDispatch_ByteIdenticalDryRun keeps the non-dispatch output +// unchanged: without repository_dispatch the dry_run signal is always the +// workflow_dispatch string, so the guard stays the bare string comparison. +func TestGM4_Rollback_NoDispatch_ByteIdenticalDryRun(t *testing.T) { + dir := pass10Fixture(t, pass10DeployWorkflow) + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev", "prod"), + Deploys: []config.DeployConfig{{ + Name: "app", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, + }}, + } + out, err := NewRollbackGenerator(cfg, dir).Generate() + require.NoError(t, err) + assert.Contains(t, out, "github.event.inputs.dry_run != 'true'") + assert.NotContains(t, out, "!= true &&", "non-dispatch guard must not gain the boolean comparison") +} + +// TestGM5_DependentDeploy_JudgesEffectiveResult proves a dependent deploy runs +// when the base deploy's ladder succeeded via a retry shim, not only when the +// immutable base result is success. It also proves the previously duplicated +// clause is gone. +func TestGM5_DependentDeploy_JudgesEffectiveResult(t *testing.T) { + dir := pass10Fixture(t, pass10DeployWorkflow) + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev"), + Deploys: []config.DeployConfig{ + {Name: "web", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, Retries: 2}, + {Name: "api", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, DependsOn: []string{"web"}}, + }, + } + out, err := NewGenerator(cfg, dir).Generate() + require.NoError(t, err) + + block := pass10JobBlock(t, out, "deploy-api") + assert.Contains(t, block, "needs.deploy-web-retry-1.result == 'success'", + "dependent deploy must consult the base deploy's retry shims") + assert.Contains(t, block, "needs.deploy-web-retry-2.result == 'success'") + // The duplicated immutable-result clause must be gone. + assert.NotContains(t, block, "needs.deploy-web.result == 'success' &&\n needs.deploy-web.result == 'success'", + "the duplicated dependency clause must be removed") +} + +// TestGM5_DependentDeploy_NoRetries_SingleClause proves the N=0 path: no retries +// means the effective helper collapses to the bare result read, and the dedup +// leaves exactly one clause (not the previously duplicated pair). +func TestGM5_DependentDeploy_NoRetries_SingleClause(t *testing.T) { + dir := pass10Fixture(t, pass10DeployWorkflow) + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: config.EnvNames("dev"), + Deploys: []config.DeployConfig{ + {Name: "web", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}}, + {Name: "api", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, DependsOn: []string{"web"}}, + }, + } + out, err := NewGenerator(cfg, dir).Generate() + require.NoError(t, err) + block := pass10JobBlock(t, out, "deploy-api") + assert.Equal(t, 1, strings.Count(block, "needs.deploy-web.result == 'success'"), + "a no-retry dependency must gate on exactly one, non-duplicated clause") + assert.NotContains(t, block, "retry", "no retry shims exist for a zero-retry dependency") +} + +// TestGM6_PromoteNativeDeployment_GuardsDryRunAndCountsSkips proves a dry-run +// promote does not create a real GitHub Deployment, and that the terminal status +// counts a legitimately skipped deploy as success and includes the prod deploy. +func TestGM6_PromoteNativeDeployment_GuardsDryRunAndCountsSkips(t *testing.T) { + dir := pass10Fixture(t, pass10DeployWorkflow) + tru := true + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: []config.EnvironmentEntry{ + {Name: "staging", EnvironmentConfig: config.EnvironmentConfig{EnvironmentURL: "https://staging.example.com"}}, + {Name: "production", EnvironmentConfig: config.EnvironmentConfig{EnvironmentURL: "https://app.example.com"}}, + }, + Deployments: &config.DeploymentsConfig{Enabled: &tru}, + Deploys: []config.DeployConfig{{ + Name: "app", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, + }}, + } + out, err := NewPromoteGenerator(cfg, dir).Generate() + require.NoError(t, err) + + // A dry run must not create a real Deployment. + assert.Contains(t, out, "if: ${{ github.server_url == 'https://github.com' && github.event.inputs.dry_run != 'true' }}", + "the Create deployment step must be gated on a non-dry-run promote") + assert.Contains(t, out, "github.event.inputs.dry_run != 'true' && always()", + "the status step must also be gated on a non-dry-run promote") + + // A skipped deploy is not a failure, and the prod deploy is not omitted. + assert.Contains(t, out, "needs.deploy-app.result == 'success' || needs.deploy-app.result == 'skipped'", + "a legitimately skipped deploy must not be counted as a Deployment failure") + assert.Contains(t, out, "needs.deploy-app-prod.result == 'success' || needs.deploy-app-prod.result == 'skipped'", + "the prod deploy result must be included in the Deployment status") +} diff --git a/internal/generate/promote.go b/internal/generate/promote.go index fe67b95..2db1913 100644 --- a/internal/generate/promote.go +++ b/internal/generate/promote.go @@ -152,7 +152,19 @@ func (g *PromoteGenerator) Generate() (string, error) { // discoverDeployInputs parses deploy workflow files to discover their inputs func (g *PromoteGenerator) discoverDeployInputs() error { for _, d := range g.config.Deploys { - workflowPath := filepath.Join(g.baseDir, d.Workflow) + // Cross-repo callbacks reference a reusable workflow in another + // repository (org/repo/.github/workflows/file.yaml@ref); there is no + // local file to parse, and external deploys thread environment/sha + // directly rather than through declared-input detection. + if config.IsExternalWorkflow(d.Workflow) { + continue + } + // Resolve to the normalized on-disk location so a bare filename + // (deploy.yaml) resolves to .github/workflows/deploy.yaml, matching the + // emitted uses: reference and the orchestrate generator. Reading the raw + // d.Workflow silently missed a canonically-located file, which dropped + // declared-input detection (for example the sha a matrix deploy threads). + workflowPath := filepath.Join(g.baseDir, normalizeWorkflowPath(d.Workflow)) data, err := os.ReadFile(workflowPath) if err != nil { // Skip if workflow doesn't exist yet @@ -448,6 +460,13 @@ func (g *PromoteGenerator) writeMatrixBuildingLogic(sb *strings.Builder, outputN sb.WriteString(" gsub(\"\\\\$\\\\{\\\\{ matrix.version \\\\}\\\\}\"; $version)\n") sb.WriteString(" else . end)')\n") sb.WriteString(" \n") + sb.WriteString(" # Carry the per-promotion environment and sha on the matrix entry so\n") + sb.WriteString(" # the deploy job name and its environment/sha with: inputs resolve.\n") + sb.WriteString(" RESOLVED=$(echo \"$RESOLVED\" | jq -c \\\n") + sb.WriteString(" --arg env \"$ENV\" \\\n") + sb.WriteString(" --arg sha \"$SHA\" \\\n") + sb.WriteString(" '. + {environment: $env, sha: $sha}')\n") + sb.WriteString(" \n") sb.WriteString(" # Add to matrix (with comma separator)\n") sb.WriteString(" if [ \"$FIRST\" = \"true\" ]; then\n") fmt.Fprintf(sb, " MATRIX_%s=\"${MATRIX_%s}${RESOLVED}\"\n", strings.ToUpper(outputName), strings.ToUpper(outputName)) @@ -774,6 +793,22 @@ func (g *PromoteGenerator) writeDeployJobs(sb *strings.Builder) { fmt.Fprintf(sb, " uses: %s\n", normalizeWorkflowPath(d.Workflow)) sb.WriteString(" with:\n") + // Thread the per-promotion environment and (when the callback + // declares it) sha from the matrix entry. Orchestrate auto-passes + // both to the same callback; without them here a promote deploys to + // an empty environment while the job name (Deploy X (${{ + // matrix.environment }})) references a key the matrix would not carry. + // environment mirrors orchestrate's contract that every deploy + // callback accepts it; sha is gated on declaration. Skip either when + // the manifest already wires it as an explicit input, so the with: + // block never emits a duplicate mapping key. + if _, ok := d.Inputs["environment"]; !ok { + sb.WriteString(" environment: ${{ matrix.environment }}\n") + } + if _, ok := d.Inputs["sha"]; !ok && g.deployHasInput(d.Name, "sha") { + sb.WriteString(" sha: ${{ matrix.sha }}\n") + } + // When the callback opts in to dry-run passthrough, forward the // dispatch input so it can emulate internally. if d.SupportsDryRun { @@ -1350,12 +1385,22 @@ func (g *PromoteGenerator) writeNativeDeploymentSteps(sb *strings.Builder) { if len(g.config.Environments) > 0 && len(g.config.Deploys) > 0 { var conds []string for _, d := range g.config.Deploys { - conds = append(conds, fmt.Sprintf("needs.deploy-%s.result == 'success'", d.Name)) + // A promotion runs a subset of deploys (only those whose scope + // includes the target env), and the prod deploy runs only in cascade + // mode, so a deploy job legitimately SKIPS. A skip is not a deploy + // failure, so success-or-skipped is the per-job success signal. The + // prod job is included so a failed prod deploy is not omitted from + // the Deployment's terminal status. + for _, job := range []string{fmt.Sprintf("deploy-%s", d.Name), fmt.Sprintf("deploy-%s-prod", d.Name)} { + conds = append(conds, fmt.Sprintf("(needs.%s.result == 'success' || needs.%s.result == 'skipped')", job, job)) + } } resultExpr = fmt.Sprintf("${{ (%s) && 'success' || 'failure' }}", strings.Join(conds, " && ")) } - writeNativeDeploymentSteps(sb, g.config, envExpr, resultExpr, " ") + // A dry-run promote skips every deploy and must not create a real GitHub + // Deployment; guard the Deployment lifecycle on the dispatch dry_run input. + writeNativeDeploymentSteps(sb, g.config, envExpr, resultExpr, "github.event.inputs.dry_run != 'true'", " ") } // writeConcurrency emits a top-level concurrency: block on the promote workflow. diff --git a/internal/generate/rollback.go b/internal/generate/rollback.go index d5f53d7..62739f4 100644 --- a/internal/generate/rollback.go +++ b/internal/generate/rollback.go @@ -372,9 +372,28 @@ func (g *RollbackGenerator) paramReadExpr(name string) string { // repository_dispatch trigger is enabled, so an external signal honors the same // dry-run and deployable scoping the manual path does. func (g *RollbackGenerator) rollbackDeployGuard(deployName string) string { - dryRun := g.paramReadExpr("dry_run") deployable := g.paramReadExpr("deployable") - return fmt.Sprintf("${{ %s != 'true' && (%s == '' || %s == '%s') }}", dryRun, deployable, deployable, deployName) + return fmt.Sprintf("${{ %s && (%s == '' || %s == '%s') }}", g.notDryRunExpr(), deployable, deployable, deployName) +} + +// notDryRunExpr returns the expression body (no ${{ }} wrapper) that is true +// when the rollback is NOT a dry run, so a deploy or finalize job may proceed. +// +// Without the repository_dispatch trigger the dry_run signal is only ever the +// workflow_dispatch input, which is always the string 'true' or 'false', so the +// output collapses to the bare string comparison and stays byte-identical to the +// baseline. With the trigger enabled the coalesced read can also carry a JSON +// boolean true from client_payload (a natural {"dry_run": true} payload), and +// GitHub Actions compares a boolean against the string 'true' by numeric +// coercion (the string casts to NaN), so a bare "!= 'true'" reads a boolean true +// as NOT a dry run and lets a dry-run rollback perform real deploys. Matching +// both the boolean literal and the string closes that gap for either source. +func (g *RollbackGenerator) notDryRunExpr() string { + if g.dispatchTrigger() == nil { + return "github.event.inputs.dry_run != 'true'" + } + v := g.paramRead("dry_run") // github.event.inputs.dry_run || github.event.client_payload.dry_run + return fmt.Sprintf("(%s) != true && (%s) != 'true'", v, v) } // writeDeployJobs emits one deploy job per configured deploy, re-running the same @@ -432,7 +451,7 @@ func (g *RollbackGenerator) writeFinalizeJob(sb *strings.Builder) { sb.WriteString(" finalize:\n") sb.WriteString(" name: Finalize\n") fmt.Fprintf(sb, " needs: %s\n", needsStr) - fmt.Fprintf(sb, " if: always() && needs.preflight.result == 'success' && %s != 'true'\n", g.paramReadExpr("dry_run")) + fmt.Fprintf(sb, " if: always() && needs.preflight.result == 'success' && %s\n", g.notDryRunExpr()) sb.WriteString(" runs-on: ubuntu-latest\n") // The finalize job commits the rolled-back state, so it needs contents: write. writeJobPermissions(sb, " ", [][2]string{{"contents", "write"}})