diff --git a/e2e/scenarios/06-callback-timeout.yaml b/e2e/scenarios/06-callback-timeout.yaml index cdb1d63b..e985559d 100644 --- a/e2e/scenarios/06-callback-timeout.yaml +++ b/e2e/scenarios/06-callback-timeout.yaml @@ -1,20 +1,22 @@ name: "Callback Timeout" description: | - Verifies the timeout_minutes field on validate/builds/deploys propagates - into the generated orchestrate.yaml as a job-level timeout-minutes (#97). + Verifies the timeout_minutes field on an inline run: callback propagates into + the generated orchestrate.yaml as a job-level timeout-minutes (#97). - This is a generator-output verification scenario. Assertion runs on the - staged repo after StageRepoFromConfig generates workflows but before any - orchestrate runs. We don't run orchestrate because act doesn't honor - timeout-minutes on `uses:` reusable workflow callers and would fail the - workflow execution. Real timeout-firing belongs to real-GHA PoC validation. + timeout-minutes is only valid on a steps job, so the callback uses an inline + run:. GitHub rejects timeout-minutes on a reusable-workflow (uses:) caller job, + so for those callbacks the timeout must live inside the called workflow. + + This is a generator-output verification scenario. Assertion runs on the staged + repo after StageRepoFromConfig generates workflows but before any orchestrate + runs. Real timeout-firing belongs to real-GHA PoC validation. config: trunk_branch: main environments: [] builds: - name: app - workflow: build.yaml + run: "make build" triggers: ["src/**"] timeout_minutes: 30 deploys: [] diff --git a/e2e/scenarios/11-job-timeouts-and-optional-deps.yaml b/e2e/scenarios/11-job-timeouts-and-optional-deps.yaml index b7e6b799..c9caebe7 100644 --- a/e2e/scenarios/11-job-timeouts-and-optional-deps.yaml +++ b/e2e/scenarios/11-job-timeouts-and-optional-deps.yaml @@ -56,3 +56,10 @@ steps: not_contains: # #18: optional dep must NOT contribute a skip-gate condition. - "needs.build-migrations.result == 'success'" + # With environments: [] no deploy jobs are emitted in promote.yaml, so + # rollback jobs and finalize must not reference nonexistent deploy jobs. + - path: ".github/workflows/promote.yaml" + not_contains: + - "rollback-deploy-app:" + - "needs.deploy-deploy-app" + - "deploy-deploy-app-prod" diff --git a/internal/config/parse.go b/internal/config/parse.go index 5a821cce..acd1b8bc 100644 --- a/internal/config/parse.go +++ b/internal/config/parse.go @@ -159,8 +159,11 @@ func Validate(cfg *TrunkConfig) []string { // Empty environments is valid - means pre-release -> release only (no deployments) envSet := make(map[string]bool) - for _, env := range cfg.Environments { + for i, env := range cfg.Environments { envSet[env] = true + // Environment names key job IDs and ${{ }} expression references, so they + // must be job-ID-safe. + errors = append(errors, validateJobIDSafeName(fmt.Sprintf("environments[%d]", i), env)...) } // Build name sets for each section (builds and deploys can share names) @@ -176,6 +179,9 @@ func Validate(cfg *TrunkConfig) []string { } else { buildNames[b.Name] = true } + // The name becomes part of the job ID (build-); enforce the job-ID + // grammar so generation cannot emit invalid YAML. + errors = append(errors, validateJobIDSafeName(fmt.Sprintf("builds[%d].name", i), b.Name)...) // workflow XOR run: exactly one must be set. errors = append(errors, validateWorkflowRunXOR(fmt.Sprintf("builds[%d]", i), b.Workflow, b.Run, b.Shell)...) errors = append(errors, validateLocalCallbackWorkflowPath(fmt.Sprintf("builds[%d]", i), b.Workflow)...) @@ -233,6 +239,9 @@ func Validate(cfg *TrunkConfig) []string { } else { deployNames[d.Name] = true } + // The name becomes part of the job ID (deploy-); enforce the job-ID + // grammar so generation cannot emit invalid YAML. + errors = append(errors, validateJobIDSafeName(fmt.Sprintf("deploys[%d].name", i), d.Name)...) // workflow XOR run: exactly one must be set. errors = append(errors, validateWorkflowRunXOR(fmt.Sprintf("deploys[%d]", i), d.Workflow, d.Run, d.Shell)...) errors = append(errors, validateLocalCallbackWorkflowPath(fmt.Sprintf("deploys[%d]", i), d.Workflow)...) @@ -352,6 +361,8 @@ func Validate(cfg *TrunkConfig) []string { errors = append(errors, fmt.Sprintf("external deploy name '%s' conflicts with local deploy name", d.Name)) } } + // External deploys also key job IDs (deploy-); enforce the grammar. + errors = append(errors, validateJobIDSafeName(fmt.Sprintf("external[%d].deploys[%d].name", i, j), d.Name)...) prefix := fmt.Sprintf("external[%d].deploys[%d]", i, j) errors = append(errors, validateExternalDeployWorkflowOnly(prefix, d.Workflow, d.Run, d.Shell)...) // External deploys are always reusable-workflow callbacks. diff --git a/internal/config/parse_test.go b/internal/config/parse_test.go index 768a9ec3..5a090403 100644 --- a/internal/config/parse_test.go +++ b/internal/config/parse_test.go @@ -853,3 +853,111 @@ func TestValidate_ReleaseTag(t *testing.T) { }) } } + +// TestValidate_JobIDSafeNames asserts that build, deploy, external-deploy, and +// environment names which would become part of a GitHub Actions job ID +// (build-, deploy-, and env-keyed identifiers) are rejected at +// config validation when they contain characters outside the job-ID grammar. +// +// A GitHub job ID must start with a letter or _ and contain only [A-Za-z0-9_-]. +// Because the name is used as a suffix after build-/deploy-, a leading digit, +// uppercase letters, and hyphens are all fine; only characters outside the +// allowed set (such as ., spaces, and /) are rejected. Sanitizing is avoided +// deliberately: two distinct names could collapse to one job ID. +func TestValidate_JobIDSafeNames(t *testing.T) { + tests := []struct { + name string + config TrunkConfig + wantErr string // substring that must appear; "" means expect no errors + wantNone bool + }{ + { + name: "build name with dot rejected", + config: TrunkConfig{ + TrunkBranch: "main", + Environments: []string{"dev"}, + Builds: []BuildConfig{{Name: "app.web", Workflow: ".github/workflows/build.yaml"}}, + }, + wantErr: `builds[0].name "app.web"`, + }, + { + name: "build name with space rejected", + config: TrunkConfig{ + TrunkBranch: "main", + Environments: []string{"dev"}, + Builds: []BuildConfig{{Name: "my app", Workflow: ".github/workflows/build.yaml"}}, + }, + wantErr: `builds[0].name "my app"`, + }, + { + name: "deploy name with slash rejected", + config: TrunkConfig{ + TrunkBranch: "main", + Environments: []string{"dev"}, + Deploys: []DeployConfig{{Name: "svc/api", Workflow: ".github/workflows/deploy.yaml"}}, + }, + wantErr: `deploys[0].name "svc/api"`, + }, + { + name: "external deploy name with dot rejected", + config: TrunkConfig{ + TrunkBranch: "main", + Environments: []string{"dev"}, + External: []ExternalRepoConfig{{ + Repo: "owner/repo", + Deploys: []ExternalDeployConfig{{Name: "svc.api", Workflow: ".github/workflows/deploy.yaml"}}, + }}, + }, + wantErr: `external[0].deploys[0].name "svc.api"`, + }, + { + name: "environment name with dot rejected", + config: TrunkConfig{ + TrunkBranch: "main", + Environments: []string{"us.east"}, + }, + wantErr: `environments[0] "us.east"`, + }, + { + name: "valid names: hyphen, uppercase, leading digit, underscore", + config: TrunkConfig{ + TrunkBranch: "main", + Environments: []string{"dev-1", "Prod", "2nd", "us_west"}, + Builds: []BuildConfig{ + {Name: "my-app", Workflow: ".github/workflows/build.yaml"}, + {Name: "MyApp", Workflow: ".github/workflows/build.yaml"}, + {Name: "_internal", Workflow: ".github/workflows/build.yaml"}, + }, + Deploys: []DeployConfig{ + {Name: "1svc", Workflow: ".github/workflows/deploy.yaml"}, + }, + }, + wantNone: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := tt.config + errs := Validate(&cfg) + if tt.wantNone { + for _, e := range errs { + if strings.Contains(e, "must contain only") { + t.Errorf("Validate() unexpectedly rejected a valid name: %q", e) + } + } + return + } + found := false + for _, e := range errs { + if strings.Contains(e, tt.wantErr) && strings.Contains(e, "must contain only") { + found = true + break + } + } + if !found { + t.Errorf("Validate() missing job-ID-safe error containing %q, got: %v", tt.wantErr, errs) + } + }) + } +} diff --git a/internal/config/validate_v1.go b/internal/config/validate_v1.go index 721727c1..fbaeba23 100644 --- a/internal/config/validate_v1.go +++ b/internal/config/validate_v1.go @@ -2,6 +2,7 @@ package config import ( "fmt" + "regexp" "sort" "strings" ) @@ -44,6 +45,32 @@ var validRolloutTypes = map[string]bool{ RolloutTypeBlueGreen: true, } +// jobIDSafeNameRe matches a name that is safe to use as a component of a GitHub +// Actions job ID. cascade derives job IDs as build-/deploy- and +// keys several identifiers and expression references off environment names. A +// GitHub job ID must start with a letter or _ and contain only [A-Za-z0-9_-]. +// Because the name is a suffix after a build-/deploy- prefix, a leading digit, +// uppercase letters, and hyphens are all acceptable in the name itself; only +// characters outside [A-Za-z0-9_-] (such as ".", spaces, and "/") break the job +// ID and the ${{ }} dereferences that read its outputs. +var jobIDSafeNameRe = regexp.MustCompile(`^[A-Za-z0-9_-]+$`) + +// validateJobIDSafeName rejects a name that would produce an invalid GitHub +// Actions job ID or break the expression references derived from it. Names are +// rejected (not sanitized) on purpose: sanitizing distinct names could collapse +// them to a single job ID and silently merge two callbacks. +func validateJobIDSafeName(prefix, name string) []string { + if name == "" { + // Empty names are reported separately by the caller (".name is required"). + return nil + } + if jobIDSafeNameRe.MatchString(name) { + return nil + } + return []string{fmt.Sprintf( + "%s %q must contain only letters, digits, hyphens, and underscores", prefix, name)} +} + // validateWorkflowRunXOR enforces that exactly one of workflow:/run: is set, and // that shell: is only present alongside run:. func validateWorkflowRunXOR(prefix, workflow, run, shell string) []string { diff --git a/internal/generate/generator.go b/internal/generate/generator.go index 74845a82..95ed1fa7 100644 --- a/internal/generate/generator.go +++ b/internal/generate/generator.go @@ -832,8 +832,12 @@ func (g *Generator) writeCallbackJob(sb *strings.Builder, info CallbackInfo, wor g.writeIfCondition(sb, info, needs) switch { - case info.TimeoutMinutes > 0: - // Explicit per-callback timeout always wins. + case info.TimeoutMinutes > 0 && info.Run != "": + // Explicit per-callback timeout wins, but only on an inline run: callback. + // timeout-minutes is forbidden on a reusable-workflow caller job + // (jobs..uses): GitHub rejects the workflow at parse time. For uses: + // callbacks the timeout must live inside the called workflow. This mirrors + // the info.Run gate on environment: below. fmt.Fprintf(sb, " timeout-minutes: %d\n", info.TimeoutMinutes) case info.Run != "": // Inline run: callbacks are cascade-owned jobs, so they inherit the diff --git a/internal/generate/generator_test.go b/internal/generate/generator_test.go index cc9b18b2..2300aff3 100644 --- a/internal/generate/generator_test.go +++ b/internal/generate/generator_test.go @@ -158,28 +158,30 @@ on: // TestGenerator_CallbackTimeoutMinutes asserts the per-callback // timeout_minutes field renders into the generated workflow as a job-level -// `timeout-minutes:` setting (#97). Without this, callbacks default to GHA's -// 360min timeout, which is too lenient for tight feedback loops and too -// strict for longer integration jobs. +// `timeout-minutes:` setting (#97) for inline run: callbacks. Without this, +// inline callbacks default to GHA's 360min timeout, which is too lenient for +// tight feedback loops and too strict for longer integration jobs. +// +// timeout-minutes is only valid on a steps job, so the callbacks here use inline +// run:. Reusable-workflow (uses:) callbacks must NOT receive it (GitHub rejects +// the workflow); that gate is covered by +// TestGenerator_ExplicitTimeoutNotOnReusableCallback. func TestGenerator_CallbackTimeoutMinutes(t *testing.T) { tmpDir := t.TempDir() require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, ".github/workflows"), 0755)) - require.NoError(t, os.WriteFile(filepath.Join(tmpDir, ".github/workflows/build.yaml"), []byte("on:\n workflow_call:\n"), 0644)) - require.NoError(t, os.WriteFile(filepath.Join(tmpDir, ".github/workflows/deploy.yaml"), []byte("on:\n workflow_call:\n"), 0644)) - require.NoError(t, os.WriteFile(filepath.Join(tmpDir, ".github/workflows/validate.yaml"), []byte("on:\n workflow_call:\n"), 0644)) cfg := &config.TrunkConfig{ TrunkBranch: "main", Environments: []string{"dev"}, Validate: &config.ValidateConfig{ - Workflow: ".github/workflows/validate.yaml", + Run: "make validate", TimeoutMinutes: 5, }, Builds: []config.BuildConfig{ - {Name: "app", Workflow: ".github/workflows/build.yaml", Triggers: []string{"src/**"}, TimeoutMinutes: 30}, + {Name: "app", Run: "make build", Triggers: []string{"src/**"}, TimeoutMinutes: 30}, }, Deploys: []config.DeployConfig{ - {Name: "svc", Workflow: ".github/workflows/deploy.yaml", DependsOn: []string{"app"}, TimeoutMinutes: 15}, + {Name: "svc", Run: "make deploy", DependsOn: []string{"app"}, TimeoutMinutes: 15}, }, } diff --git a/internal/generate/job_control_test.go b/internal/generate/job_control_test.go index 3837c69e..2e2ebb89 100644 --- a/internal/generate/job_control_test.go +++ b/internal/generate/job_control_test.go @@ -248,3 +248,48 @@ func TestGenerator_BothFieldsUnsetNonBreaking(t *testing.T) { assert.NotContains(t, jobBlock(t, result, "build-app"), "timeout-minutes:") assert.Contains(t, jobBlock(t, result, "setup"), "timeout-minutes: 30") } + +// TestGenerator_ExplicitTimeoutNotOnReusableCallback asserts that an explicit +// per-callback timeout_minutes is NOT emitted as a job-level timeout-minutes on +// a reusable-workflow (uses:) callback. GitHub forbids timeout-minutes on a job +// that calls a reusable workflow (allowed caller keys: name, uses, with, +// secrets, needs, if, permissions, strategy, concurrency); the timeout must live +// inside the called workflow. An inline run: callback with the same setting DOES +// carry it, since inline jobs are cascade-owned steps jobs. +func TestGenerator_ExplicitTimeoutNotOnReusableCallback(t *testing.T) { + tmpDir := t.TempDir() + writeStubWorkflow(t, tmpDir, "build.yaml") + writeStubWorkflow(t, tmpDir, "deploy.yaml") + + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: []string{"dev"}, + Builds: []config.BuildConfig{ + // Reusable-workflow callback with an explicit timeout: must NOT emit it. + {Name: "reusable", Workflow: ".github/workflows/build.yaml", Triggers: []string{"src/**"}, TimeoutMinutes: 15}, + // Inline run: callback with an explicit timeout: DOES emit it. + {Name: "inline", Run: "go test ./...", Triggers: []string{"src/**"}, TimeoutMinutes: 15}, + }, + Deploys: []config.DeployConfig{ + // Reusable-workflow deploy callback with an explicit timeout: no emit. + {Name: "svc", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}, TimeoutMinutes: 15}, + }, + } + + result, err := NewGenerator(cfg, tmpDir).Generate() + require.NoError(t, err) + + reusableBuild := jobBlock(t, result, "build-reusable") + assert.Contains(t, reusableBuild, "uses:", "sanity: reusable callback is a uses: caller") + assert.NotContains(t, reusableBuild, "timeout-minutes:", + "explicit timeout_minutes must not be emitted on a reusable-workflow caller job") + + reusableDeploy := jobBlock(t, result, "deploy-svc") + assert.Contains(t, reusableDeploy, "uses:", "sanity: reusable deploy is a uses: caller") + assert.NotContains(t, reusableDeploy, "timeout-minutes:", + "explicit timeout_minutes must not be emitted on a reusable-workflow deploy caller job") + + inline := jobBlock(t, result, "build-inline") + assert.Contains(t, inline, "timeout-minutes: 15", + "explicit timeout_minutes on an inline run: callback is honored") +} diff --git a/internal/generate/promote.go b/internal/generate/promote.go index 5f6dd593..b71de80f 100644 --- a/internal/generate/promote.go +++ b/internal/generate/promote.go @@ -1014,6 +1014,14 @@ func (g *PromoteGenerator) writeRollbackJobs(sb *strings.Builder) { return } + // Deploy jobs are only emitted when there is at least one environment (see + // writeDeployJobs). With no environments, the deploy jobs a rollback would + // depend on do not exist, so emitting rollback jobs would produce a + // needs: reference to a nonexistent job that GitHub rejects at parse time. + if len(g.config.Environments) == 0 { + return + } + // Collect all deploy job names for failure detection var allDeployJobs []string for _, d := range g.config.Deploys { @@ -1100,20 +1108,27 @@ func (g *PromoteGenerator) writeRollbackJobs(sb *strings.Builder) { func (g *PromoteGenerator) writeFinalizeJob(sb *strings.Builder) { // Build needs list: [preflight, promote, deploy-, deploy-, deploy--prod, ...] + // + // Deploy jobs are only emitted when there is at least one environment (see + // writeDeployJobs). With no environments there are no deploy jobs, so + // referencing them in needs: would produce a "needs job X which does not + // exist" parse error on GitHub. needs := []string{"preflight", "promote"} - for _, d := range g.config.Deploys { - needs = append(needs, fmt.Sprintf("deploy-%s", d.Name)) - } - // Add prod deploy jobs - for _, d := range g.config.Deploys { - needs = append(needs, fmt.Sprintf("deploy-%s-prod", d.Name)) - } - // Add external deploy jobs - for _, ext := range g.config.External { - for _, d := range ext.Deploys { + if len(g.config.Environments) > 0 { + for _, d := range g.config.Deploys { needs = append(needs, fmt.Sprintf("deploy-%s", d.Name)) + } + // Add prod deploy jobs + for _, d := range g.config.Deploys { needs = append(needs, fmt.Sprintf("deploy-%s-prod", d.Name)) } + // Add external deploy jobs + for _, ext := range g.config.External { + for _, d := range ext.Deploys { + needs = append(needs, fmt.Sprintf("deploy-%s", d.Name)) + needs = append(needs, fmt.Sprintf("deploy-%s-prod", d.Name)) + } + } } sb.WriteString(" finalize:\n") @@ -1338,9 +1353,14 @@ func (g *PromoteGenerator) writeFinalizeJob(sb *strings.Builder) { fmt.Fprintf(sb, " GH_TOKEN: %s\n", g.getStateTokenRef()) fmt.Fprintf(sb, " GITHUB_TOKEN: %s\n", g.getReleaseTokenRef()) sb.WriteString(" PROMOTION_RESULT: ${{ needs.preflight.outputs.promotion_result }}\n") - for _, d := range g.config.Deploys { - envKey := "DEPLOY_RESULT_" + strings.ToUpper(strings.ReplaceAll(d.Name, "-", "_")) - fmt.Fprintf(sb, " %s: ${{ needs.deploy-%s.result }}\n", envKey, d.Name) + // Deploy result env vars reference deploy jobs, which only exist when there + // is at least one environment. Skip them otherwise so finalize does not + // dereference a job that was never emitted. + if len(g.config.Environments) > 0 { + for _, d := range g.config.Deploys { + envKey := "DEPLOY_RESULT_" + strings.ToUpper(strings.ReplaceAll(d.Name, "-", "_")) + fmt.Fprintf(sb, " %s: ${{ needs.deploy-%s.result }}\n", envKey, d.Name) + } } sb.WriteString(" run: |\n") fmt.Fprintf(sb, " cascade promote finalize \\\n") diff --git a/internal/generate/promote_test.go b/internal/generate/promote_test.go index 8ac08c8d..0a93c8f3 100644 --- a/internal/generate/promote_test.go +++ b/internal/generate/promote_test.go @@ -1248,6 +1248,43 @@ func TestPromoteGenerator_RollbackJobs(t *testing.T) { assert.Contains(t, content, "rollback_on_failure: ${{ steps.preflight.outputs.rollback_on_failure }}") } +// TestPromoteGenerator_NoRollbackWhenNoEnvironments asserts that with +// environments: [] no rollback job is emitted, even when deploys: is non-empty. +// Deploy jobs are only written when len(Environments) > 0, so a rollback job +// would reference deploy jobs that do not exist (needs: deploy-), which +// GitHub rejects at parse ("needs job X which does not exist"). +func TestPromoteGenerator_NoRollbackWhenNoEnvironments(t *testing.T) { + cfg := &config.TrunkConfig{ + TrunkBranch: "main", + Environments: []string{}, + Deploys: []config.DeployConfig{ + {Name: "app", Workflow: ".github/workflows/deploy-app.yaml"}, + }, + } + + gen := NewPromoteGenerator(cfg, "") + content, err := gen.Generate() + require.NoError(t, err) + + // No deploy jobs exist, so no rollback job may reference them. + assert.NotContains(t, content, "rollback-app:", + "no rollback job when there are no environments (no deploy jobs exist)") + assert.NotContains(t, content, "deploy-app:", + "sanity: no deploy job is emitted when environments is empty") + assert.NotContains(t, content, "needs.deploy-app", + "no needs: reference to a nonexistent deploy job") + // The finalize job's needs: list must not reference deploy jobs either. + assert.NotContains(t, content, "deploy-app-prod", + "no needs: reference to a nonexistent prod deploy job") + assert.NotContains(t, content, "DEPLOY_RESULT_APP", + "no deploy-result env var dereferencing a nonexistent deploy job") + + // The emitted workflow must remain structurally valid YAML. + var parsed map[string]any + require.NoError(t, yaml.Unmarshal([]byte(content), &parsed), + "emitted promote workflow must be valid YAML") +} + func TestPromoteGenerator_RollbackJobs_InlineRunDeploy(t *testing.T) { tests := []struct { name string