diff --git a/docs/pipeline-design.md b/docs/pipeline-design.md index 4afd6ff..0b7c40c 100644 --- a/docs/pipeline-design.md +++ b/docs/pipeline-design.md @@ -458,6 +458,40 @@ answer for almost every stage, a widened stage names the one scope it needs, and the rendered orchestrator says which stage holds what — so the grant is reviewable in the diff rather than implied by a template. +### Independent stages + +Ordering is fixed, but *dependency* is a claim about the repository, and a +repository can be right that it does not have one. + +```yaml +pipeline: + ci: + stages: [preflight, build, test, end2end] + # end2end rebuilds the daemon from source inside Docker and consumes + # nothing build produces. + independent_stages: [end2end] +``` + +A stage named here drops its sibling dependencies and depends on preflight +alone. That is the only thing it can do: it cannot add a dependency, invent an +edge, or change the order stages render in. The fixed graph survives, because +removing an edge you do not have is not rearranging the pipeline. + +The dependency on preflight is never removed. preflight produces no artefact — +it is the gate deciding whether a stage runs at all, and the rendered `if:` +reads its outputs, so dropping it from `needs:` would leave a condition that is +never true. `preflight` itself cannot be declared independent; it has no +sibling dependency to drop. + +Detaching a stage does **not** remove it from `ci-gate`. A failing end2end +still blocks the merge; only its start time moves. + +wardnet is the case this was added for. Its end2end suite rebuilds the daemon +from source, so it consumes nothing the build stage produces, and its previous +hand-rolled pipeline ran the two concurrently. Serialising them added about +eleven minutes to every daemon pull request — 24 minutes to 35 — to prove an +edge that does not exist. + ## Migration Per repo: move build and test jobs out of the existing `ci.yml` into diff --git a/internal/repogov/pipeline.go b/internal/repogov/pipeline.go index 50041a3..024ad3c 100644 --- a/internal/repogov/pipeline.go +++ b/internal/repogov/pipeline.go @@ -120,12 +120,16 @@ var cdWiring = map[string]stageWiring{ // decided whether it needs to run at all. func buildStages( enabled []string, order []string, wiring map[string]stageWiring, root, guard string, - baseline map[string]string, grants repospec.StagePermissions, + baseline map[string]string, grants repospec.StagePermissions, independent []string, ) ([]stageJob, error) { present := map[string]bool{} for _, s := range enabled { present[s] = true } + detached := map[string]bool{} + for _, s := range independent { + detached[s] = true + } hasPreflight := present["preflight"] jobs := make([]stageJob, 0, len(enabled)) @@ -143,9 +147,17 @@ func buildStages( needs := []string{root} for _, dep := range w.after { - if present[dep] { - needs = append(needs, dep) + if !present[dep] { + continue + } + // A stage declared independent consumes no sibling's output, so + // only preflight survives — it produces no artefact, and the + // gating `if:` below reads its outputs, which requires it in + // `needs:`. + if detached[name] && dep != repospec.StagePreflight { + continue } + needs = append(needs, dep) } var conds []string @@ -343,7 +355,8 @@ const checkoutRef = "actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # func buildCIData(in Input, shared templateData) (ciData, error) { stages, err := buildStages( in.Spec.Pipeline.CI.Stages, repospec.CIStages, ciWiring, "attest", attestGuard, - ciBaselinePermissions, in.Spec.Pipeline.CI.StagePermissions) + ciBaselinePermissions, in.Spec.Pipeline.CI.StagePermissions, + in.Spec.Pipeline.CI.IndependentStages) if err != nil { return ciData{}, err } @@ -388,7 +401,8 @@ func buildCDData(in Input, shared templateData) (cdData, error) { const root = "verify-attestation" stages, err := buildStages( in.Spec.Pipeline.CD.Stages, repospec.CDStages, cdWiring, root, "", - cdBaselinePermissions, in.Spec.Pipeline.CD.StagePermissions) + cdBaselinePermissions, in.Spec.Pipeline.CD.StagePermissions, + in.Spec.Pipeline.CD.IndependentStages) if err != nil { return cdData{}, err } diff --git a/internal/repospec/spec.go b/internal/repospec/spec.go index eb4daa0..21ab796 100644 --- a/internal/repospec/spec.go +++ b/internal/repospec/spec.go @@ -160,6 +160,9 @@ type PipelineCI struct { // StagePermissions grants a named stage scopes beyond the CI baseline. // See StagePermissions for why this exists and what it cannot do. StagePermissions StagePermissions `yaml:"stage_permissions,omitempty" json:"stage_permissions,omitempty"` + // IndependentStages names stages that consume no sibling stage's output. + // See the type for what it can and cannot express. + IndependentStages []string `yaml:"independent_stages,omitempty" json:"independent_stages,omitempty"` } type PipelineCD struct { @@ -171,6 +174,8 @@ type PipelineCD struct { Tags []string `yaml:"tags" json:"tags"` // StagePermissions grants a named stage scopes beyond the CD baseline. StagePermissions StagePermissions `yaml:"stage_permissions,omitempty" json:"stage_permissions,omitempty"` + // IndependentStages names stages that consume no sibling stage's output. + IndependentStages []string `yaml:"independent_stages,omitempty" json:"independent_stages,omitempty"` } // StagePermissions maps a stage name to the scopes its orchestrator job is @@ -188,6 +193,26 @@ type PipelineCD struct { // so the grant is reviewable in the diff rather than implied by a template. type StagePermissions map[string]map[string]string +// IndependentStages, in a pipeline spec, is the set of stages that consume no +// sibling stage's output and so need not wait for one. +// +// This is a statement about the repository, not a reordering knob. The stage +// graph stays fixed: nothing here can add a dependency, invent an edge, or +// change the order stages render in. The only thing it can do is remove a +// dependency the repository does not actually have, dropping that stage back +// to depending on preflight alone. +// +// The dependency on preflight is never removed, because preflight produces no +// artefact — it is the gate deciding whether a stage runs at all, and the +// rendered `if:` reads its outputs. +// +// wardnet is the case this was added for. Its end2end suite rebuilds the +// daemon from source inside Docker, so it consumes nothing the build stage +// produces, and under its previous hand-rolled pipeline the two ran +// concurrently. Making it wait for build added about eleven minutes to every +// daemon pull request while proving nothing — a real cost paid for an edge +// that does not exist. + type ConventionalCommits struct { Enabled bool `yaml:"enabled" json:"enabled"` Scope string `yaml:"scope" json:"scope"` @@ -275,10 +300,15 @@ const ( // CIStages and CDStages are the stage vocabularies, in the order the // orchestrators wire them. var ( - CIStages = []string{"preflight", "build", "test", "end2end"} - CDStages = []string{"preflight", "publish", "deploy", "verify"} + CIStages = []string{StagePreflight, "build", "test", "end2end"} + CDStages = []string{StagePreflight, "publish", "deploy", "verify"} ) +// StagePreflight is the gate every other stage reads its run- output +// from. It is the one stage that never depends on a sibling, and the one that +// can never be declared independent. +const StagePreflight = "preflight" + // PermissionScopes is the GITHUB_TOKEN scope vocabulary, and PermissionLevels // the values each may take. // @@ -583,6 +613,16 @@ func validatePipeline(p Pipeline) error { ); err != nil { return err } + if err := validateIndependentStages( + "pipeline.ci.independent_stages", p.CI.Enabled, p.CI.IndependentStages, p.CI.Stages, + ); err != nil { + return err + } + if err := validateIndependentStages( + "pipeline.cd.independent_stages", p.CD.Enabled, p.CD.IndependentStages, p.CD.Stages, + ); err != nil { + return err + } if p.CD.Enabled && len(p.CD.Tags) == 0 { return fmt.Errorf("pipeline.cd.tags cannot be empty when CD is enabled; nothing would ever trigger it") } @@ -649,6 +689,36 @@ func validateStagePermissions( return nil } +// validateIndependentStages rejects a declaration that cannot mean anything: +// one naming a stage the pipeline does not run, a duplicate, or `preflight`, +// which has no sibling dependency to drop in the first place. +func validateIndependentStages(field string, enabled bool, got, stages []string) error { + if len(got) == 0 { + return nil + } + if !enabled { + return fmt.Errorf("%s: cannot name stages while the pipeline is disabled", field) + } + seen := map[string]bool{} + for i, stage := range got { + if !contains(stages, stage) { + return fmt.Errorf( + "%s[%d]: stage %q is not enabled (enabled: %s)", + field, i, stage, strings.Join(stages, ", ")) + } + if seen[stage] { + return fmt.Errorf("%s[%d]: duplicate stage %q", field, i, stage) + } + if stage == StagePreflight { + return fmt.Errorf( + "%s[%d]: %q has no stage dependency to drop; remove it", + field, i, stage) + } + seen[stage] = true + } + return nil +} + func validateSettings(s Settings) error { m := s.Merge if !m.Squash && !m.MergeCommit && !m.Rebase { diff --git a/tests/repogov_pipeline_test.go b/tests/repogov_pipeline_test.go index 89ac0f2..d2df3d9 100644 --- a/tests/repogov_pipeline_test.go +++ b/tests/repogov_pipeline_test.go @@ -313,6 +313,20 @@ func TestPipelineValidation(t *testing.T) { "build": {"security-events": "write"}, } }, "pipeline is disabled"}, + {"independent stage that is not enabled", func(s *repospec.Spec) { + s.Pipeline.CI.Stages = []string{"preflight", "build"} + s.Pipeline.CI.IndependentStages = []string{"end2end"} + }, "is not enabled"}, + {"independent preflight", func(s *repospec.Spec) { + s.Pipeline.CI.IndependentStages = []string{"preflight"} + }, "no stage dependency to drop"}, + {"duplicate independent stage", func(s *repospec.Spec) { + s.Pipeline.CI.IndependentStages = []string{"end2end", "end2end"} + }, "duplicate stage"}, + {"independent stages while ci is disabled", func(s *repospec.Spec) { + s.Pipeline.CI.Enabled = false + s.Pipeline.CI.IndependentStages = []string{"end2end"} + }, "pipeline is disabled"}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -860,3 +874,81 @@ func TestStagePermissionsRenderDeterministically(t *testing.T) { } } } + +// The reason this exists: wardnet's end2end suite rebuilds the daemon from +// source and consumes nothing build produces, so waiting on build added ~11 +// minutes to every daemon PR to prove an edge that does not exist. +func TestIndependentStageDropsSiblingDependenciesButNotPreflight(t *testing.T) { + spec := repospec.Default() + spec.Pipeline.CI.IndependentStages = []string{"end2end"} + + jobs := workflowJobs(t, pipelineFiles(t, spec)[".github/workflows/ci-orchestration.yml"]) + + e2e, ok := jobs["end2end"] + if !ok { + t.Fatalf("no end2end job; jobs = %v", jobs) + } + want := []string{"attest", "preflight"} + if !reflect.DeepEqual(e2e.Needs, want) { + t.Errorf("end2end needs = %v, want %v", e2e.Needs, want) + } + + // preflight must survive, or the gating condition references the outputs + // of a job this one does not wait for and is never true. + if !strings.Contains(e2e.If, "needs.preflight.outputs.run-end2end") { + t.Errorf("end2end lost its preflight gate; if = %q", e2e.If) + } + + // Independence is per-stage: test still consumes build's artefacts. + if test, ok := jobs["test"]; !ok { + t.Error("no test job") + } else if !contains(test.Needs, "build") { + t.Errorf("test needs = %v, want it to still wait on build", test.Needs) + } +} + +// Detaching a stage must not drop it from the gate. If it did, a failing +// end2end would stop blocking the merge — the exact defect branch protection +// exists to prevent, arriving as a side effect of a performance tweak. +func TestIndependentStageIsStillGated(t *testing.T) { + spec := repospec.Default() + spec.Pipeline.CI.IndependentStages = []string{"end2end"} + + jobs := workflowJobs(t, pipelineFiles(t, spec)[".github/workflows/ci-orchestration.yml"]) + gate, ok := jobs[repospec.GateCheckJob] + if !ok { + t.Fatalf("no %s job", repospec.GateCheckJob) + } + if !contains(gate.Needs, "end2end") { + t.Errorf("%s needs = %v, want end2end among them", repospec.GateCheckJob, gate.Needs) + } +} + +// Declaring nothing must render exactly what it rendered before, so this +// cannot quietly reshape the seventeen repos that do not use it. +func TestStagesKeepTheirDependenciesByDefault(t *testing.T) { + jobs := workflowJobs(t, pipelineFiles(t, repospec.Default())[".github/workflows/ci-orchestration.yml"]) + + for _, tc := range []struct{ stage, dep string }{ + {"test", "build"}, + {"end2end", "build"}, + {"build", "preflight"}, + } { + j, ok := jobs[tc.stage] + if !ok { + t.Fatalf("no %s job", tc.stage) + } + if !contains(j.Needs, tc.dep) { + t.Errorf("%s needs = %v, want %q among them", tc.stage, j.Needs, tc.dep) + } + } +} + +func contains(haystack []string, needle string) bool { + for _, h := range haystack { + if h == needle { + return true + } + } + return false +}