From 6754760299d95b69a33b00c218c07367cd75d8fd Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Tue, 3 Nov 2026 23:40:32 -0500 Subject: [PATCH 1/6] fix(render): prune what the composition no longer declares The cell file was derived from the directory listing of deployments/modules/, not from the composition: a module removed from `modules:` left its render record behind and kept its namespace, workloads and consumer edges in every cell file derived after it, its delivered tree stayed in the delivery repository with the ApplicationSet that kept stamping Applications the cell refused, and once the file was right every publish refused until someone named the module back with --allow-cell-removal. The render now derives the cell file from the composition, deletes the records of modules it no longer declares once the file is written, and prints each removal. A removal the composition explains passes the hold without the flag and is reported as such; the flag keeps confirming every other removal. A forward publication to a hosted environment removes the delivered tree of such a module with everything else it writes and shows it in the plan; a rollback removes none. Co-Authored-By: Claude Fable 5.1 --- cmd/deploy/gitops.go | 11 ++- docs/cli-reference.md | 4 +- docs/commands.md | 26 ++++++- pkg/gitops/cell.go | 58 +++++++++++++++- pkg/gitops/cell_removals.go | 112 +++++++++++++++++++++++++++---- pkg/gitops/cell_removals_test.go | 86 ++++++++++++++++++++++-- pkg/gitops/cell_test.go | 46 +++++++++++++ pkg/gitops/publish.go | 27 +++++--- pkg/gitops/publish_cell_test.go | 71 ++++++++++++++++++-- pkg/gitops/types.go | 12 +++- 10 files changed, 411 insertions(+), 42 deletions(-) diff --git a/cmd/deploy/gitops.go b/cmd/deploy/gitops.go index a4837d85..84543f76 100644 --- a/cmd/deploy/gitops.go +++ b/cmd/deploy/gitops.go @@ -594,6 +594,12 @@ func printPublishPlan(plan *gitops.PublishPlan) { for _, removal := range plan.CellRemovals { cli.Warning("The cell file removes %s, as --allow-cell-removal names", removal) } + for _, removal := range plan.UndeclaredCellRemovals { + cli.Warning("The cell file removes %s", removal) + } + for _, removed := range plan.RemovedModules { + cli.Warning("Removed %s from the delivery: the composition no longer declares it", removed) + } for _, stale := range plan.StaleConsumers { cli.Warning("Delivered consumer %s bakes in another value of a workspace configuration group this publication carries; publish it next, so %s does not keep delivering two values of one group", stale, plan.Environment) } @@ -657,7 +663,7 @@ func init() { } for _, command := range []*cobra.Command{gitOpsPlanCmd, gitOpsPublishCmd} { command.Flags().StringSliceVar(&gitOpsAllowCellRemovals, "allow-cell-removal", nil, - "Module whose removal from the environment's cell file, relative to the base branch's, is intended (repeatable); any other removal refuses") + "Module whose removal from the environment's cell file, relative to the base branch's, is intended (repeatable); a module the composition no longer declares is removed without it, and any other removal refuses") } for _, command := range []*cobra.Command{gitOpsPublishCmd, gitOpsRollbackCmd} { command.Flags().StringVar(&gitOpsMessage, "message", "", "Signed commit message") @@ -696,6 +702,9 @@ func printCell(cell *gitops.CellResult) { if len(cell.Skipped) > 0 { cli.Info("Cell file leaves out %s: rendered for another environment", strings.Join(cell.Skipped, ", ")) } + for _, removed := range cell.Removed { + cli.Info("removed %s: the composition no longer declares it", removed) + } if !cell.Source { cli.Info("Cell file names no source commit: the environment declares no gitops repository and branch, or the workspace is not a git checkout") } diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 376398e3..7321a7ae 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -2274,7 +2274,7 @@ codefly deploy gitops plan [module] Flags: ``` - --allow-cell-removal strings Module whose removal from the environment's cell file, relative to the base branch's, is intended (repeatable); any other removal refuses + --allow-cell-removal strings Module whose removal from the environment's cell file, relative to the base branch's, is intended (repeatable); a module the composition no longer declares is removed without it, and any other removal refuses --allow-unresolved-contracts Downgrade a consumed contract whose exposing module is not yet deployed to a warning, for bootstrap ordering --env string Environment to promote (default "local") --local Use a disposable local file Git remote for k3d qualification @@ -2294,7 +2294,7 @@ codefly deploy gitops publish [module] Flags: ``` - --allow-cell-removal strings Module whose removal from the environment's cell file, relative to the base branch's, is intended (repeatable); any other removal refuses + --allow-cell-removal strings Module whose removal from the environment's cell file, relative to the base branch's, is intended (repeatable); a module the composition no longer declares is removed without it, and any other removal refuses --allow-unresolved-contracts Downgrade a consumed contract whose exposing module is not yet deployed to a warning, for bootstrap ordering --body string Promotion pull request body --env string Environment to promote (default "local") diff --git a/docs/commands.md b/docs/commands.md index 8af8d527..269b4a18 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -1150,6 +1150,16 @@ and egress the environment's), so an entry carried over is a claim about a composition that may no longer exist, and no digest is invented for a module that was not rendered. +The file is derived from the **composition**, not from whatever records the +checkout holds. A module removed from `modules:` leaves its render record +behind, and so does one that was never composed here; such a record is left +out of the file and deleted by the render that derives it — nothing could +render that module again — and the render says so, one line per record +(`removed deployments/modules/: the composition no longer declares +it`). Every render that re-derives the cell file prunes this way, a single +module's and `--all` alike, because that is where the claim about the cell is +made; an environment with no `host:` has no cell file and nothing is pruned. + **A publish carries the cell file.** A cell admits only the builds its cell file names, so a promotion that delivered a module's tree and left the file behind would deliver builds the cluster refuses — every refused workload @@ -1188,9 +1198,19 @@ committed with the rendered trees, and a cell admits only what it names, so and the one the delivery base branch carries at `/cells//cell.yaml`, and refuse when the workspace's removes a namespace, a workload, an endpoint or a consumer of an endpoint the -base admits — naming each removal and the modules it concerns. A removal that -is intended (a module the workspace no longer composes, a dependency dropped) -is named with `--allow-cell-removal ` (repeatable); a consumer edge is +base admits — naming each removal and the modules it concerns. A removal the +composition explains needs no naming: when the module it concerns is no +longer in `modules:` (the namespace of a module removed from the composition, +or its consumer edge on another module's endpoint), the hold lets it through +and the plan reports it (`undeclaredCellRemovals`), and the publication also +removes that module's delivered tree from `/modules/` with +everything else it writes (`removedModules`): the cell no longer admits +anything of it, and the ApplicationSet that tree carries would keep stamping +Applications whose every pod the cell refuses. A rollback removes none, and a +directory under the modules path with no render record is not a delivery and +is left alone. Any other removal — a module still composed whose render is +missing from the checkout, a dependency dropped, a service withdrawn — is +named with `--allow-cell-removal ` (repeatable); a consumer edge is covered by naming either the endpoint's module or the consumer's, and the plan lists every removal it lets through. A base with no cell file has nothing to hold the workspace's to. The file held is the file carried — the same bytes, diff --git a/pkg/gitops/cell.go b/pkg/gitops/cell.go index 7ac550a7..37faea73 100644 --- a/pkg/gitops/cell.go +++ b/pkg/gitops/cell.go @@ -52,6 +52,13 @@ type CellResult struct { // Skipped names module trees rendered for another environment, which // describe another cell. Skipped []string + // Removed are the render records of modules the composition no longer + // declares, deleted by this render (pruneUndeclaredRecords), as paths + // relative to the workspace. + Removed []string + // undeclared are those records before they are removed: deriveCell + // finds them, RenderCell deletes them once the cell file is written. + undeclared []string // Source reports whether the file names its repository, branch and base // revision. It does not when the environment declares no gitops // repository or the workspace is not a git checkout. @@ -94,12 +101,52 @@ func RenderCell(ctx context.Context, workspace *resources.Workspace, env *enviro if err != nil { return CellResult{}, err } - if err := writeCell(&file, result.Path); err != nil { + if err = writeCell(&file, result.Path); err != nil { + return CellResult{}, err + } + if result.Removed, err = pruneUndeclaredRecords(workspace, result.undeclared); err != nil { return CellResult{}, err } return result, nil } +// pruneUndeclaredRecords deletes the render records of modules the composition +// no longer declares, once the cell file they were left out of is written. +// Nothing could render such a module again (a render refuses a module the +// workspace does not compose), so its record describes a composition that no +// longer exists; left in place it was read back into every cell file derived +// here, and a module removed from `modules:` kept its namespace, workloads and +// consumer edges in the cell. It answers the paths removed, relative to the +// workspace, for the render to print. +func pruneUndeclaredRecords(workspace *resources.Workspace, records []string) ([]string, error) { + var removed []string + for _, tree := range records { + if err := os.RemoveAll(tree); err != nil { + return nil, fmt.Errorf("remove the render record of a module the composition no longer declares: %w", err) + } + relative, err := filepath.Rel(workspace.Dir(), tree) + if err != nil { + relative = tree + } + removed = append(removed, filepath.ToSlash(relative)) + } + sort.Strings(removed) + return removed, nil +} + +// composedModules names every module the composition declares, in this +// workspace or one it imports: the set a cell file describes and the set a +// removal from it is judged against. +func composedModules(workspace *resources.Workspace) map[string]bool { + declared := make(map[string]bool, len(workspace.Modules)) + for _, reference := range workspace.Modules { + if reference != nil { + declared[reference.Name] = true + } + } + return declared +} + // deriveCell is RenderCell without the write: the cell file the rendered // trees and the composition describe, and the path it belongs at. func deriveCell(ctx context.Context, workspace *resources.Workspace, env *environments.Environment) (cellfile.File, CellResult, error) { @@ -140,6 +187,7 @@ func deriveCell(ctx context.Context, workspace *resources.Workspace, env *enviro Source: cellSource(ctx, workspace, env), } result := CellResult{Source: file.Source != nil, ConfiguredEdges: configured} + declared := composedModules(workspace) for _, entry := range entries { if !entry.IsDir() { continue @@ -152,6 +200,14 @@ func deriveCell(ctx context.Context, workspace *resources.Workspace, env *enviro if loadErr != nil { return cellfile.File{}, CellResult{}, fmt.Errorf("the render of module %s cannot be read, so the cell file cannot describe what it delivers; render %s again for %s: %w", entry.Name(), entry.Name(), env.Name, loadErr) } + // The cell file is derived from the composition, not from whatever + // records the checkout holds: a record of a module no longer in + // `modules:` describes nothing the composition delivers, for this + // environment or any other, and is removed once the file is written. + if !declared[inventory.Module] { + result.undeclared = append(result.undeclared, tree) + continue + } if inventory.Environment != env.Name { result.Skipped = append(result.Skipped, inventory.Module) continue diff --git a/pkg/gitops/cell_removals.go b/pkg/gitops/cell_removals.go index e4e25637..31ef41a9 100644 --- a/pkg/gitops/cell_removals.go +++ b/pkg/gitops/cell_removals.go @@ -4,13 +4,20 @@ package gitops // cell admits only what it names, so a workload, a namespace or a consumer // absent from it is withdrawn. A publication promotes a render, so before it // does, the workspace's cell file is held to the one the delivery base branch -// carries, and a removal it does not name is refused: a cell file derived +// carries, and a removal nothing explains is refused: a cell file derived // from a checkout missing some modules' renders would otherwise remove those -// modules from admission without anyone having said so. +// modules from admission without anyone having said so. A removal the +// composition explains — the module left `modules:` — is carried without +// more, and the module's delivered tree goes with it: the cell no longer +// admits anything of it, and the ApplicationSet that tree carries would keep +// stamping Applications whose every pod the cell refuses. import ( "context" + "errors" "fmt" + "io/fs" + "os" "path/filepath" "slices" "strings" @@ -89,34 +96,48 @@ func deliveryCellPath(gitopsPath, environment string) string { // holdCellToBase refuses the publication when the cell file it carries for a // hosted environment removes anything the delivery base revision's file -// carries, unless every removal concerns a module allowed names. It returns -// the removals allowed, for the plan to show. A publication carrying no cell -// file (publicationCellFile), or a base with none, has nothing to hold. +// carries, unless the removal is explained: it concerns a module the +// composition no longer declares (declared is the composition's module set), +// or a module allowed names. It returns the removals allowed by name, and the +// removals the composition explains, for the plan to show. A publication +// carrying no cell file (publicationCellFile), or a base with none, has +// nothing to hold. +// +// A removal the composition explains needs no naming: the flag confirms a +// removal nothing else accounts for — a module still composed whose render is +// missing from the checkout, a dependency dropped — where a module gone from +// `modules:` is the removal's own statement of intent, and asking for it again +// made every publication after such a change refuse until someone typed the +// module's name back in. // // It is the cell file the publication CARRIES that is held — the same bytes, // read once — so the file delivered and the file held can never be two // different files, and the hold runs before anything is carried. -func holdCellToBase(ctx context.Context, cell *publicationCell, env *environments.Environment, repo, baseBranch, baseRevision string, allowed []string) ([]string, error) { +func holdCellToBase(ctx context.Context, cell *publicationCell, env *environments.Environment, repo, baseBranch, baseRevision string, allowed []string, declared map[string]bool) (named, undeclared []string, err error) { if cell == nil { - return nil, nil + return nil, nil, nil } path, next := cell.source, cell.file baseData, err := gitCommandBytes(ctx, repo, "show", baseRevision+":"+cell.delivered) if err != nil { if gitSaysAbsent(err) { - return nil, nil + return nil, nil, nil } - return nil, fmt.Errorf("read %s's cell file %s: %w", baseBranch, cell.delivered, err) + return nil, nil, fmt.Errorf("read %s's cell file %s: %w", baseBranch, cell.delivered, err) } base, err := cellfile.Parse(baseData) if err != nil { - return nil, fmt.Errorf("%s's cell file %s cannot be read: %w", baseBranch, cell.delivered, err) + return nil, nil, fmt.Errorf("%s's cell file %s cannot be read: %w", baseBranch, cell.delivered, err) } - var refused, admitted []string + var refused []string unnamed := map[string]bool{} for _, removal := range cellRemovals(base, next) { + if gone := slices.IndexFunc(removal.modules, func(module string) bool { return !declared[module] }); gone >= 0 { + undeclared = append(undeclared, fmt.Sprintf("%s: the composition no longer declares module %s", removal.what, removal.modules[gone])) + continue + } if slices.ContainsFunc(removal.modules, func(module string) bool { return slices.Contains(allowed, module) }) { - admitted = append(admitted, removal.what) + named = append(named, removal.what) continue } refused = append(refused, removal.what) @@ -128,8 +149,71 @@ func holdCellToBase(ctx context.Context, cell *publicationCell, env *environment modules = append(modules, module) } slices.Sort(modules) - return nil, fmt.Errorf("environment %s's cell file %s removes from the cell what %s's admits, and says nothing of it: %s. Publishing it would withdraw those from the cell's admission set. If a module's render is missing from this checkout, render it (`codefly deploy gitops render --all --env %s`); if the removal is intended — a module the workspace no longer composes, a dependency dropped — name each module it concerns with --allow-cell-removal (%s)", + return nil, nil, fmt.Errorf("environment %s's cell file %s removes from the cell what %s's admits, and says nothing of it: %s. Publishing it would withdraw those from the cell's admission set. If a module's render is missing from this checkout, render it (`codefly deploy gitops render --all --env %s`); if the removal is intended — a dependency dropped, a service withdrawn — name each module it concerns with --allow-cell-removal (%s)", env.Name, path, baseBranch, strings.Join(refused, "; "), env.Name, strings.Join(modules, ", ")) } - return admitted, nil + return named, undeclared, nil +} + +// undeclaredDeliveries names the module trees the delivery repository holds +// under the environment's modules path, as the promotion clone stands, whose +// module the composition no longer declares: delivered once, and left behind +// when the module left `modules:`. A tree is a delivery when it carries a +// render record (InventoryFilename); anything else under that path is not this +// package's to judge. The module being published is never among them: a +// publish of a module the composition does not compose is refused long before +// this. Paths are relative to the repository, slash-separated. +func undeclaredDeliveries(repo, gitopsPath, module string, declared map[string]bool) ([]string, error) { + modulesPath := deliveryModulesPath(gitopsPath) + modulesDir, err := confinedJoin(repo, modulesPath) + if err != nil { + return nil, err + } + entries, err := os.ReadDir(modulesDir) + if errors.Is(err, fs.ErrNotExist) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("list the delivered module trees under %s: %w", modulesPath, err) + } + var stale []string + for _, entry := range entries { + if !entry.IsDir() || entry.Name() == module || declared[entry.Name()] { + continue + } + if _, err := os.Stat(filepath.Join(modulesDir, entry.Name(), InventoryFilename)); err != nil { + continue + } + stale = append(stale, deliveryModulePath(gitopsPath, entry.Name())) + } + return stale, nil +} + +// removeUndeclaredDeliveries deletes from the promotion clone the delivered +// trees of modules the composition no longer declares (undeclaredDeliveries), +// and answers their paths for the publication to own, stage and show. A +// forward publication to a hosted environment carries the composition's claim +// whole — the cell file it carries no longer admits those modules — so it is +// the one that removes them; a rollback restores one module's tree and claims +// nothing about the rest, and an environment with no host has no cell whose +// claim the trees contradict. Nothing is staged here: the removal reaches the +// promotion only with everything else the publication writes, after the hold. +func removeUndeclaredDeliveries(repo, gitopsPath, module string, env *environments.Environment, restoreRevision string, declared map[string]bool) ([]string, error) { + if env == nil || env.Host == nil || restoreRevision != "" { + return nil, nil + } + deliveries, err := undeclaredDeliveries(repo, gitopsPath, module, declared) + if err != nil { + return nil, err + } + for _, delivery := range deliveries { + target, err := confinedJoin(repo, delivery) + if err != nil { + return nil, err + } + if err = os.RemoveAll(target); err != nil { + return nil, fmt.Errorf("remove the delivered tree %s of a module the composition no longer declares: %w", delivery, err) + } + } + return deliveries, nil } diff --git a/pkg/gitops/cell_removals_test.go b/pkg/gitops/cell_removals_test.go index f94d3887..1624e080 100644 --- a/pkg/gitops/cell_removals_test.go +++ b/pkg/gitops/cell_removals_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "strings" "testing" "github.com/codefly-dev/cli/pkg/environments" @@ -55,9 +56,11 @@ func TestPublishRefusesACellFileThatRemovesWhatTheBaseAdmits(t *testing.T) { require.NoError(t, err) repo, base := cellBase(t, complete) - removals, err := holdCellToBase(ctx, publicationCellOf(t, workspace, env), env, repo, "main", base, nil) + declared := composedModules(workspace) + removals, explained, err := holdCellToBase(ctx, publicationCellOf(t, workspace, env), env, repo, "main", base, nil, declared) require.NoError(t, err, "a full render removes nothing") require.Empty(t, removals) + require.Empty(t, explained) // The cell file a render of shop alone wrote before it refused to: billing // is gone, and so is billing/ledger as a caller of shop's grpc endpoint. @@ -73,25 +76,98 @@ func TestPublishRefusesACellFileThatRemovesWhatTheBaseAdmits(t *testing.T) { require.NoError(t, os.WriteFile(full.Path, dropped, 0o600)) dropping := publicationCellOf(t, workspace, env) - _, err = holdCellToBase(ctx, dropping, env, repo, "main", base, nil) + _, _, err = holdCellToBase(ctx, dropping, env, repo, "main", base, nil, declared) require.Error(t, err) require.Contains(t, err.Error(), "namespace acme-billing (module billing, 2 workload(s))") require.Contains(t, err.Error(), "consumer billing/ledger of acme-shop/api/grpc") require.Contains(t, err.Error(), "--allow-cell-removal (billing)") - _, err = holdCellToBase(ctx, dropping, env, repo, "main", base, []string{"shop"}) + _, _, err = holdCellToBase(ctx, dropping, env, repo, "main", base, []string{"shop"}, declared) require.Error(t, err, "naming the endpoint's module covers its consumer edge, but not billing's namespace") require.NotContains(t, err.Error(), "consumer billing/ledger") require.Contains(t, err.Error(), "namespace acme-billing") - removals, err = holdCellToBase(ctx, dropping, env, repo, "main", base, []string{"billing"}) + removals, explained, err = holdCellToBase(ctx, dropping, env, repo, "main", base, []string{"billing"}, declared) require.NoError(t, err) require.Equal(t, []string{"namespace acme-billing (module billing, 2 workload(s))", "consumer billing/ledger of acme-shop/api/grpc"}, removals) + require.Empty(t, explained, "billing is still composed: its removal is the flag's to confirm") empty, _ := cellBase(t, nil) require.NoError(t, os.Remove(filepath.Join(empty, filepath.FromSlash(deliveryCellPath("", "staging"))))) runGit(t, empty, "commit", "-am", "no cell file") emptyBase := runGit(t, empty, "rev-parse", "HEAD") - removals, err = holdCellToBase(ctx, dropping, env, empty, "main", emptyBase, nil) + removals, _, err = holdCellToBase(ctx, dropping, env, empty, "main", emptyBase, nil, declared) require.NoError(t, err, "a base with no cell file has nothing to hold the workspace's to") require.Empty(t, removals) } + +// A removal the composition explains needs no naming. Once legacy leaves +// `modules:`, a render of the composition derives a cell file without it — and +// prunes its record — and a publication of that file passes the hold with no +// flag: the namespace and legacy's consumer edge on shop's endpoint are +// reported as removals the composition explains, for the reviewer, rather than +// refused until someone names a module the workspace no longer composes. +func TestARemovalTheCompositionExplainsNeedsNoNaming(t *testing.T) { + ctx := context.Background() + workspace := writeCellWorkspace(t) + workspace = composeLegacyModule(t, workspace) + env := selectedEnvironment(t, workspace, "staging") + renderCellTree(t, workspace, "shop", "api", "acme-shop", "staging") + renderCellTree(t, workspace, "billing", "ledger", "acme-billing", "staging") + renderCellTree(t, workspace, "legacy", "worker", "acme-legacy", "staging") + full, err := RenderCell(ctx, workspace, env) + require.NoError(t, err) + require.Equal(t, []string{"billing", "legacy", "shop"}, full.Modules) + complete, err := os.ReadFile(full.Path) + require.NoError(t, err) + repo, base := cellBase(t, complete) + + // legacy leaves the composition; its module directory and its render + // record are still in the checkout, as they are after a `modules:` edit. + workspace = editWorkspaceConfiguration(t, workspace, " - name: legacy\n", "") + env = selectedEnvironment(t, workspace, "staging") + + rendered, err := RenderCell(ctx, workspace, env) + require.NoError(t, err) + require.Equal(t, []string{"billing", "shop"}, rendered.Modules) + require.Equal(t, []string{"deployments/modules/legacy"}, rendered.Removed) + require.NoDirExists(t, moduleRenderDestination(workspace, "legacy")) + + named, explained, err := holdCellToBase(ctx, publicationCellOf(t, workspace, env), env, repo, "main", base, nil, composedModules(workspace)) + require.NoError(t, err, "a removal the composition explains needs no --allow-cell-removal") + require.Empty(t, named) + require.Equal(t, []string{ + "namespace acme-legacy (module legacy, 2 workload(s)): the composition no longer declares module legacy", + "consumer legacy/worker of acme-shop/api/grpc: the composition no longer declares module legacy", + }, explained) +} + +// composeLegacyModule adds a third module, legacy, to the cell fixture: one +// service, worker, calling shop's api, so dropping the module later removes a +// namespace and a consumer edge at once. +func composeLegacyModule(t *testing.T, workspace *resources.Workspace) *resources.Workspace { + t.Helper() + files := map[string]string{ + filepath.Join("modules", "legacy", resources.ModuleConfigurationName): "kind: module\nname: legacy\nservices:\n - name: worker\n", + filepath.Join("modules", "legacy", "services", "worker", resources.ServiceConfigurationName): cellServiceYAML("worker") + "service-dependencies:\n - name: api\n module: shop\n endpoints:\n - name: grpc\n", + } + for rel, content := range files { + full := filepath.Join(workspace.Dir(), rel) + require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o755)) + require.NoError(t, os.WriteFile(full, []byte(content), 0o644)) + } + return editWorkspaceConfiguration(t, workspace, " - name: billing\n", " - name: billing\n - name: legacy\n") +} + +// editWorkspaceConfiguration replaces one passage of the workspace's +// configuration and loads the workspace again. +func editWorkspaceConfiguration(t *testing.T, workspace *resources.Workspace, old, replacement string) *resources.Workspace { + t.Helper() + path := filepath.Join(workspace.Dir(), resources.WorkspaceConfigurationName) + config, err := os.ReadFile(path) + require.NoError(t, err) + require.Contains(t, string(config), old) + require.NoError(t, os.WriteFile(path, []byte(strings.Replace(string(config), old, replacement, 1)), 0o644)) + edited, err := resources.LoadWorkspaceFromDir(context.Background(), workspace.Dir()) + require.NoError(t, err) + return edited +} diff --git a/pkg/gitops/cell_test.go b/pkg/gitops/cell_test.go index 142e186d..a4ef16f9 100644 --- a/pkg/gitops/cell_test.go +++ b/pkg/gitops/cell_test.go @@ -327,6 +327,52 @@ func TestCellFileRefusesToDropAComposedModuleWithNoRender(t *testing.T) { require.Equal(t, []string{"billing", "shop"}, again.Modules) } +// The cell file is derived from the composition, not from whatever records +// the checkout holds. A module removed from `modules:` leaves its render +// record behind (and so does one that was never composed here), and the file +// used to read it back: the module kept its namespace, workloads and consumer +// edges in the cell after the composition dropped it. The render now leaves +// such records out of the file, deletes them — nothing could render them again +// — and says so. +func TestRenderCellPrunesTheRecordOfAModuleTheCompositionNoLongerDeclares(t *testing.T) { + workspace := writeCellWorkspace(t) + env := selectedEnvironment(t, workspace, "staging") + renderCellTree(t, workspace, "shop", "api", "acme-shop", "staging") + renderCellTree(t, workspace, "billing", "ledger", "acme-billing", "staging") + // Records left by modules the composition does not declare: one for this + // environment, one rendered for another. Neither can be rendered again. + renderCellTreeWith(t, &RenderOptions{ + Destination: moduleRenderDestination(workspace, "proposals"), + Module: "proposals", Environment: "staging", Namespace: "acme-proposals", AppProject: "acme-staging", + Promotable: true, OwnedPath: "deployments/modules/proposals", Units: unitsWithPorts("proposals", "web"), + }, "web", "staging") + renderCellTreeWith(t, &RenderOptions{ + Destination: moduleRenderDestination(workspace, "compute"), + Module: "compute", Environment: "production", Namespace: "acme-compute", AppProject: "acme-production", + Promotable: true, OwnedPath: "deployments/modules/compute", Units: unitsWithPorts("compute", "worker"), + }, "worker", "production") + + result, err := RenderCell(context.Background(), workspace, env) + require.NoError(t, err) + require.Equal(t, []string{"billing", "shop"}, result.Modules) + require.Empty(t, result.Skipped, "a record of an undeclared module is pruned, not reported as another environment's") + require.Equal(t, []string{"deployments/modules/compute", "deployments/modules/proposals"}, result.Removed) + require.NoDirExists(t, moduleRenderDestination(workspace, "proposals")) + require.NoDirExists(t, moduleRenderDestination(workspace, "compute")) + require.DirExists(t, moduleRenderDestination(workspace, "shop")) + + data, err := os.ReadFile(result.Path) + require.NoError(t, err) + file, err := cellfile.Parse(data) + require.NoError(t, err) + names := make([]string, 0, len(file.Namespaces)) + for _, namespace := range file.Namespaces { + names = append(names, namespace.Name) + } + require.Equal(t, []string{"acme-billing", "acme-shop"}, names) + require.NotContains(t, string(data), "proposals") +} + // A workload whose authenticating container cannot be told from a sidecar is // refused by name rather than guessed. func TestCellFileRefusesAWorkloadWhoseAuthenticatingContainerCannotBeTold(t *testing.T) { diff --git a/pkg/gitops/publish.go b/pkg/gitops/publish.go index 308f6d99..ba43620b 100644 --- a/pkg/gitops/publish.go +++ b/pkg/gitops/publish.go @@ -220,6 +220,13 @@ func preparePublish( if err != nil { return fail(err) } + declared := composedModules(workspace) + removedModules, err := removeUndeclaredDeliveries(repo, pathRoot, request.Module, env, restoreRevision, declared) + if err != nil { + return fail(err) + } + owned = append(owned, removedModules...) + publishedPaths = append(publishedPaths, removedModules...) if err = refuseUnrelatedPromotionChanges(ctx, repo, baseRevision, promotionBranch, owned, branchRevision, replacesRevision); err != nil { return fail(err) } @@ -249,7 +256,7 @@ func preparePublish( return fail(err) } staleConsumers := describeStaleConsumers(staleOnBase) - allowedRemovals, err := holdCellToBase(ctx, cell, env, repo, baseBranch, baseRevision, request.AllowCellRemovals) + allowedRemovals, undeclaredRemovals, err := holdCellToBase(ctx, cell, env, repo, baseBranch, baseRevision, request.AllowCellRemovals, declared) if err != nil { return fail(err) } @@ -273,14 +280,16 @@ func preparePublish( Module: request.Module, Environment: request.Environment, RenderDigest: inventory.Digest, SnapshotRevision: snapshotRevision, Changed: changed, Diff: diff, ContractChecks: contractChecks, - Delivery: inventory.Delivery, - StaleConsumers: staleConsumers, - CellRemovals: allowedRemovals, - CellFile: carriedCell, - CellNotice: cellNotice(env, cell, restoreRevision, request.Module), - Carriers: publication.options.Carriers, - SelfPromotes: selfPromotes, - LocalRepository: bound, + Delivery: inventory.Delivery, + StaleConsumers: staleConsumers, + CellRemovals: allowedRemovals, + UndeclaredCellRemovals: undeclaredRemovals, + RemovedModules: removedModules, + CellFile: carriedCell, + CellNotice: cellNotice(env, cell, restoreRevision, request.Module), + Carriers: publication.options.Carriers, + SelfPromotes: selfPromotes, + LocalRepository: bound, } if bound != nil { if err = bound.verify(ctx, config.RepoURL); err != nil { diff --git a/pkg/gitops/publish_cell_test.go b/pkg/gitops/publish_cell_test.go index 16652c6f..28886dbe 100644 --- a/pkg/gitops/publish_cell_test.go +++ b/pkg/gitops/publish_cell_test.go @@ -19,22 +19,81 @@ import ( // repository, so a publication has a base branch's approved-build list to be // held to. func seedCellFile(t *testing.T, remote, gitopsPath, environment string, data []byte) { + t.Helper() + seedDelivered(t, remote, "seed the cell file", map[string][]byte{deliveryCellPath(gitopsPath, environment): data}) +} + +// seedDelivered commits files directly onto main of the bare delivery +// repository, at the repository-relative paths given. +func seedDelivered(t *testing.T, remote, message string, files map[string][]byte) { t.Helper() work := t.TempDir() gitRun(t, "", "clone", remote, work) gitRun(t, work, "config", "user.name", "Codefly Test") gitRun(t, work, "config", "user.email", "codefly@example.com") gitRun(t, work, "config", "commit.gpgsign", "false") - path := filepath.Join(work, filepath.FromSlash(deliveryCellPath(gitopsPath, environment))) - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + for relative, data := range files { + path := filepath.Join(work, filepath.FromSlash(relative)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, data, 0o644); err != nil { + t.Fatal(err) + } + } + gitRun(t, work, "add", "-A") + gitRun(t, work, "commit", "-m", message) + gitRun(t, work, "push", "origin", "main") +} + +// TestPublishRemovesADeliveredModuleTheCompositionNoLongerDeclares: a module +// removed from `modules:` left its delivered tree in the delivery repository, +// and with it the ApplicationSet that kept stamping Applications whose every +// pod the cell — which no longer named the module — refused. A forward +// publication to a hosted environment now removes such a tree with the rest +// of what it writes, shows it in the plan, and leaves alone a directory under +// the modules path that is not a delivery (no render record). +func TestPublishRemovesADeliveredModuleTheCompositionNoLongerDeclares(t *testing.T) { + ctx, workspace, remote := developerPublishWorkspace(t) + env := selectedEnvironment(t, workspace, "production") + renderedCell(t, ctx, workspace, env) + legacy := deliveryModulePath("environments", "legacy") + seedDelivered(t, remote, "a module since removed from the composition", map[string][]byte{ + legacy + "/" + InventoryFilename: []byte(`{"module":"legacy","environment":"production"}`), + legacy + "/services/worker/kustomization.yaml": []byte("resources: []\n"), + deliveryModulesPath("environments") + "/notes/README.md": []byte("not a delivery\n"), + }) + + request := PublishRequest{ + Module: "payments", Environment: "production", Local: true, Signer: &fakeSigner{nonce: true}, + PromotionBranch: "codefly/promote-payments-production", + } + plan, err := PlanPublish(ctx, workspace, &request) + if err != nil { t.Fatal(err) } - if err := os.WriteFile(path, data, 0o644); err != nil { + if !slices.Equal(plan.RemovedModules, []string{legacy}) { + t.Fatalf("the plan removes %v, want %v", plan.RemovedModules, []string{legacy}) + } + for _, path := range []string{legacy + "/" + InventoryFilename, legacy + "/services/worker/kustomization.yaml"} { + if !slices.Contains(plan.Changed, path) { + t.Fatalf("the plan's changed files %v do not carry the removal of %s", plan.Changed, path) + } + } + if slices.ContainsFunc(plan.Changed, func(path string) bool { return strings.Contains(path, "/notes/") }) { + t.Fatalf("a directory that is not a delivery was removed: %v", plan.Changed) + } + + result, err := Publish(ctx, workspace, &PublishMutation{Request: request, PlanID: plan.ID, Carriers: plan.Carriers}, preparedPermit) + if err != nil { t.Fatal(err) } - gitRun(t, work, "add", "-A") - gitRun(t, work, "commit", "-m", "seed the cell file") - gitRun(t, work, "push", "origin", "main") + if exec.Command("git", "--git-dir", remote, "cat-file", "-e", result.Commit+":"+legacy+"/"+InventoryFilename).Run() == nil { + t.Fatalf("the published commit still delivers %s", legacy) + } + if exec.Command("git", "--git-dir", remote, "cat-file", "-e", result.Commit+":"+deliveryModulesPath("environments")+"/notes/README.md").Run() != nil { + t.Fatal("the published commit removed a directory that is not a delivery") + } } // renderedCell renders the environment's cell file from the workspace's module diff --git a/pkg/gitops/types.go b/pkg/gitops/types.go index 26c38389..b314961f 100644 --- a/pkg/gitops/types.go +++ b/pkg/gitops/types.go @@ -478,7 +478,9 @@ type PublishRequest struct { // AllowCellRemovals names the modules whose removal from the hosted // environment's cell file this publish intends: a namespace, workload, // endpoint or consumer the delivery base branch's cell file carries and - // the workspace's does not. Any other removal refuses the publish. + // the workspace's does not. A removal of a module the composition no + // longer declares needs no naming; any other unnamed removal refuses the + // publish. AllowCellRemovals []string `json:"allowCellRemovals,omitempty"` } @@ -517,6 +519,14 @@ type PublishPlan struct { // CellRemovals are the removals from the base branch's cell file the // request named (AllowCellRemovals), for the reviewer to see. CellRemovals []string `json:"cellRemovals,omitempty"` + // UndeclaredCellRemovals are the removals from the base branch's cell + // file the composition explains — their module is no longer declared — + // which no request needs to name. + UndeclaredCellRemovals []string `json:"undeclaredCellRemovals,omitempty"` + // RemovedModules are the delivered module trees this publication removes, + // relative to the delivery repository: modules the composition no longer + // declares, which the cell file it carries no longer admits. + RemovedModules []string `json:"removedModules,omitempty"` // CellFile is the environment's cell file this publication carries into // the delivery repository, at the path the cell's infrastructure reads it // from. A cell admits only the builds that file names, so a promotion of From 9208ed2054d1acf59d9d475b8d5d37aa287cee0f Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Wed, 4 Nov 2026 00:00:08 -0500 Subject: [PATCH 2/6] fix(publish): judge the composition's removals from a checkout that saw the base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings of the independent review of the first commit. A publish continuing or replacing the module's own promotion branch refused the removal that branch already carried as an unrelated change, because the removed trees were owned only when the clone still held them; the branch guard now owns the delivered tree of any module the composition does not declare by rule. And a stale checkout that never had a module derived the same cell file as one that dropped it, so it could withdraw a live module with no flag; the hold and the pruning now require the checkout to descend from the revision the base's cell file came from — the delivery base commit itself when the workspace repository holds it, else the source revision the file names — and say which revision it lacks when it does not, where naming the module confirms the removal as before. Co-Authored-By: Claude Fable 5.1 --- docs/commands.md | 29 ++-- pkg/gitops/cell_removals.go | 228 ++++++++++++++++++++++++------- pkg/gitops/cell_removals_test.go | 92 ++++++++++++- pkg/gitops/publish.go | 15 +- pkg/gitops/publish_cell_test.go | 149 ++++++++++++++++++-- 5 files changed, 433 insertions(+), 80 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index 269b4a18..24609307 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -1201,15 +1201,26 @@ removes a namespace, a workload, an endpoint or a consumer of an endpoint the base admits — naming each removal and the modules it concerns. A removal the composition explains needs no naming: when the module it concerns is no longer in `modules:` (the namespace of a module removed from the composition, -or its consumer edge on another module's endpoint), the hold lets it through -and the plan reports it (`undeclaredCellRemovals`), and the publication also -removes that module's delivered tree from `/modules/` with -everything else it writes (`removedModules`): the cell no longer admits -anything of it, and the ApplicationSet that tree carries would keep stamping -Applications whose every pod the cell refuses. A rollback removes none, and a -directory under the modules path with no render record is not a delivery and -is left alone. Any other removal — a module still composed whose render is -missing from the checkout, a dependency dropped, a service withdrawn — is +or its consumer edge on another module's endpoint), **and the checkout has +seen the composition the base's file was rendered from**, the hold lets it +through and the plan reports it (`undeclaredCellRemovals`), and the +publication also removes that module's delivered tree from +`/modules/` with everything else it writes (`removedModules`): +the cell no longer admits anything of it, and the ApplicationSet that tree +carries would keep stamping Applications whose every pod the cell refuses. A +checkout that never had a module and one that dropped it derive the same cell +file, so the history decides: the checkout must contain the delivery base +commit when the workspace repository holds it (the workspace is the delivery +repository), or else the `source.revision` the base's cell file names; a +checkout that contains neither — a branch behind the base, a base file naming +no source — is held as before, the refusal says which revision it lacks, and +naming the module confirms the removal without pruning the tree. A promotion +of a module owns, on its own branch, the removal of such trees, so continuing +or retrying the publication that removed one is not refused as someone else's +change. A rollback removes none, and a directory under the modules path with +no render record is not a delivery and is left alone. Any other removal — a +module still composed whose render is missing from the checkout, a dependency +dropped, a service withdrawn — is named with `--allow-cell-removal ` (repeatable); a consumer edge is covered by naming either the endpoint's module or the consumer's, and the plan lists every removal it lets through. A base with no cell file has nothing to diff --git a/pkg/gitops/cell_removals.go b/pkg/gitops/cell_removals.go index 31ef41a9..c8e06cd0 100644 --- a/pkg/gitops/cell_removals.go +++ b/pkg/gitops/cell_removals.go @@ -24,6 +24,7 @@ import ( "github.com/codefly-dev/cli/pkg/environments" "github.com/codefly-dev/cli/pkg/gitops/cellfile" + "github.com/codefly-dev/core/resources" ) // cellRemoval is one thing the base cell file admits that the next one does @@ -94,14 +95,147 @@ func deliveryCellPath(gitopsPath, environment string) string { return filepath.ToSlash(filepath.Join(gitopsPath, cellsDir, environment, cellfile.FileName)) } +// compositionClaim is what a publication may remove from the cell and the +// delivery on the composition's word alone: the modules the composition no +// longer declares — provided this checkout's composition has moved FORWARD +// from the one that rendered the delivery base's cell file (advanced). A +// checkout that never had a module and one that dropped it derive the same +// cell file, and only the history tells them apart: a checkout that contains +// the commit the base's file came from has seen the composition that named +// the module, so its absence is a removal; one that does not is stale, and +// its publication keeps needing --allow-cell-removal, as every withdrawal +// nobody stated does. +type compositionClaim struct { + // module is the one being published: never a removal of its own. + module string + // declared is the composition's module set (composedModules). + declared map[string]bool + // advanced is whether the checkout descends from revision; revision is the + // commit it had to contain, empty when the base names none. + advanced bool + revision string +} + +// newCompositionClaim judges the checkout against the delivery base: the base +// revision itself when the workspace repository holds that commit (the +// workspace IS the delivery repository, and the file's render was committed +// there), or else the workspace revision the base's cell file says it was +// rendered from (cellSource). The base commit is preferred because it is +// exact: the source revision is the render's parent, and a `modules:` edit +// may ride in the render's own commit. A base with no cell file, or one +// naming no source, leaves nothing to descend from, and the claim is not +// advanced. +func newCompositionClaim(ctx context.Context, workspace *resources.Workspace, module, baseRevision string, base *cellfile.File) *compositionClaim { + claim := &compositionClaim{module: module, declared: composedModules(workspace)} + if base == nil { + return claim + } + dir := workspace.Dir() + head, err := gitCommand(ctx, dir, "rev-parse", "HEAD") + if err != nil { + return claim + } + switch { + case gitHasCommit(ctx, dir, baseRevision): + claim.revision = baseRevision + case base.Source != nil && base.Source.Revision != "": + claim.revision = base.Source.Revision + default: + return claim + } + claim.advanced = gitIsAncestor(ctx, dir, claim.revision, head) + return claim +} + +// gitHasCommit reports whether the repository holds the commit. +func gitHasCommit(ctx context.Context, repo, revision string) bool { + _, err := gitCommand(ctx, repo, "cat-file", "-e", revision+"^{commit}") + return err == nil +} + +// undeclared reports a module the composition does not declare, other than +// the one being published. +func (claim *compositionClaim) undeclared(module string) bool { + return module != claim.module && !claim.declared[module] +} + +// explains reports a removal the composition accounts for: the module is no +// longer declared, by a checkout that has seen it declared. +func (claim *compositionClaim) explains(module string) bool { + return claim.advanced && claim.undeclared(module) +} + +// owns answers, for the promotion-branch guard, whether a delivery-repository +// path lies in the delivered tree of a module the composition no longer +// declares. A promotion of this module owns those trees whether or not the +// clone still holds them: the branch it continues, or the stale tip it +// replaces, carries this module's own earlier removal of such a tree, and the +// base may since have taken it, so neither tree says what the branch removed. +func (claim *compositionClaim) owns(gitopsPath string) func(path string) bool { + prefix := deliveryModulesPath(gitopsPath) + "/" + return func(path string) bool { + rest, under := strings.CutPrefix(path, prefix) + if !under { + return false + } + name, _, _ := strings.Cut(rest, "/") + return name != "" && claim.undeclared(name) + } +} + +// compositionBase is where a publication's composition claim is judged: the +// promotion clone, the delivery base it was cut from, and the publication. +type compositionBase struct { + repo, gitopsPath, branch, revision string + module, restoreRevision string +} + +// judgeCompositionRemovals is the one step a publication takes on the +// composition's word: it reads the base's cell file, judges the checkout's +// claim against it and removes from the clone the delivered trees that claim +// explains, answering all three for the hold, the branch guard and the plan. +func judgeCompositionRemovals(ctx context.Context, workspace *resources.Workspace, env *environments.Environment, cell *publicationCell, base *compositionBase) (*cellfile.File, *compositionClaim, []string, error) { + baseCell, err := baseCellFile(ctx, cell, base.repo, base.branch, base.revision) + if err != nil { + return nil, nil, nil, err + } + claim := newCompositionClaim(ctx, workspace, base.module, base.revision, baseCell) + removed, err := removeUndeclaredDeliveries(base.repo, base.gitopsPath, env, base.restoreRevision, claim) + if err != nil { + return nil, nil, nil, err + } + return baseCell, claim, removed, nil +} + +// baseCellFile reads the cell file the delivery base revision carries at the +// path the publication delivers to, nil when the base has none. A publication +// carrying no cell file is held to nothing, and reads none. +func baseCellFile(ctx context.Context, cell *publicationCell, repo, baseBranch, baseRevision string) (*cellfile.File, error) { + if cell == nil { + return nil, nil + } + baseData, err := gitCommandBytes(ctx, repo, "show", baseRevision+":"+cell.delivered) + if err != nil { + if gitSaysAbsent(err) { + return nil, nil + } + return nil, fmt.Errorf("read %s's cell file %s: %w", baseBranch, cell.delivered, err) + } + base, err := cellfile.Parse(baseData) + if err != nil { + return nil, fmt.Errorf("%s's cell file %s cannot be read: %w", baseBranch, cell.delivered, err) + } + return base, nil +} + // holdCellToBase refuses the publication when the cell file it carries for a -// hosted environment removes anything the delivery base revision's file -// carries, unless the removal is explained: it concerns a module the -// composition no longer declares (declared is the composition's module set), -// or a module allowed names. It returns the removals allowed by name, and the -// removals the composition explains, for the plan to show. A publication -// carrying no cell file (publicationCellFile), or a base with none, has -// nothing to hold. +// hosted environment removes anything the delivery base revision's file (base) +// carries, unless the removal is explained: the composition no longer +// declares the module it concerns and the checkout has seen it declared +// (compositionClaim), or a module allowed names. It returns the removals +// allowed by name, and the removals the composition explains, for the plan to +// show. A publication carrying no cell file, or a base with none, has nothing +// to hold. // // A removal the composition explains needs no naming: the flag confirms a // removal nothing else accounts for — a module still composed whose render is @@ -113,26 +247,15 @@ func deliveryCellPath(gitopsPath, environment string) string { // It is the cell file the publication CARRIES that is held — the same bytes, // read once — so the file delivered and the file held can never be two // different files, and the hold runs before anything is carried. -func holdCellToBase(ctx context.Context, cell *publicationCell, env *environments.Environment, repo, baseBranch, baseRevision string, allowed []string, declared map[string]bool) (named, undeclared []string, err error) { - if cell == nil { +func holdCellToBase(cell *publicationCell, env *environments.Environment, baseBranch string, base *cellfile.File, allowed []string, claim *compositionClaim) (named, undeclared []string, err error) { + if cell == nil || base == nil { return nil, nil, nil } path, next := cell.source, cell.file - baseData, err := gitCommandBytes(ctx, repo, "show", baseRevision+":"+cell.delivered) - if err != nil { - if gitSaysAbsent(err) { - return nil, nil, nil - } - return nil, nil, fmt.Errorf("read %s's cell file %s: %w", baseBranch, cell.delivered, err) - } - base, err := cellfile.Parse(baseData) - if err != nil { - return nil, nil, fmt.Errorf("%s's cell file %s cannot be read: %w", baseBranch, cell.delivered, err) - } - var refused []string + var refused, stale []string unnamed := map[string]bool{} for _, removal := range cellRemovals(base, next) { - if gone := slices.IndexFunc(removal.modules, func(module string) bool { return !declared[module] }); gone >= 0 { + if gone := slices.IndexFunc(removal.modules, claim.explains); gone >= 0 { undeclared = append(undeclared, fmt.Sprintf("%s: the composition no longer declares module %s", removal.what, removal.modules[gone])) continue } @@ -142,28 +265,39 @@ func holdCellToBase(ctx context.Context, cell *publicationCell, env *environment } refused = append(refused, removal.what) unnamed[removal.modules[len(removal.modules)-1]] = true + if gone := slices.IndexFunc(removal.modules, claim.undeclared); gone >= 0 && !slices.Contains(stale, removal.modules[gone]) { + stale = append(stale, removal.modules[gone]) + } + } + if len(refused) == 0 { + return named, undeclared, nil + } + modules := make([]string, 0, len(unnamed)) + for module := range unnamed { + modules = append(modules, module) } - if len(refused) > 0 { - modules := make([]string, 0, len(unnamed)) - for module := range unnamed { - modules = append(modules, module) + slices.Sort(modules) + message := fmt.Sprintf("environment %s's cell file %s removes from the cell what %s's admits, and says nothing of it: %s. Publishing it would withdraw those from the cell's admission set. If a module's render is missing from this checkout, render it (`codefly deploy gitops render --all --env %s`); if the removal is intended — a dependency dropped, a service withdrawn — name each module it concerns with --allow-cell-removal (%s)", + env.Name, path, baseBranch, strings.Join(refused, "; "), env.Name, strings.Join(modules, ", ")) + if len(stale) > 0 { + slices.Sort(stale) + if claim.revision == "" { + message += fmt.Sprintf(". This checkout's composition does not declare %s, but %s's cell file names no revision it was rendered from, so whether the composition removed %s or never had it cannot be told from here", strings.Join(stale, ", "), baseBranch, strings.Join(stale, ", ")) + } else { + message += fmt.Sprintf(". This checkout's composition does not declare %s, but the checkout does not contain %s, which %s's cell file was rendered from, so the removal may be a stale checkout's rather than the composition's: bring the checkout up to date and render again, or name it", strings.Join(stale, ", "), claim.revision, baseBranch) } - slices.Sort(modules) - return nil, nil, fmt.Errorf("environment %s's cell file %s removes from the cell what %s's admits, and says nothing of it: %s. Publishing it would withdraw those from the cell's admission set. If a module's render is missing from this checkout, render it (`codefly deploy gitops render --all --env %s`); if the removal is intended — a dependency dropped, a service withdrawn — name each module it concerns with --allow-cell-removal (%s)", - env.Name, path, baseBranch, strings.Join(refused, "; "), env.Name, strings.Join(modules, ", ")) } - return named, undeclared, nil + return nil, nil, errors.New(message) } // undeclaredDeliveries names the module trees the delivery repository holds // under the environment's modules path, as the promotion clone stands, whose -// module the composition no longer declares: delivered once, and left behind -// when the module left `modules:`. A tree is a delivery when it carries a -// render record (InventoryFilename); anything else under that path is not this -// package's to judge. The module being published is never among them: a -// publish of a module the composition does not compose is refused long before -// this. Paths are relative to the repository, slash-separated. -func undeclaredDeliveries(repo, gitopsPath, module string, declared map[string]bool) ([]string, error) { +// removal the composition explains (compositionClaim): delivered once, and +// left behind when the module left `modules:`. A tree is a delivery when it +// carries a render record (InventoryFilename); anything else under that path +// is not this package's to judge. Paths are relative to the repository, +// slash-separated. +func undeclaredDeliveries(repo, gitopsPath string, claim *compositionClaim) ([]string, error) { modulesPath := deliveryModulesPath(gitopsPath) modulesDir, err := confinedJoin(repo, modulesPath) if err != nil { @@ -178,7 +312,7 @@ func undeclaredDeliveries(repo, gitopsPath, module string, declared map[string]b } var stale []string for _, entry := range entries { - if !entry.IsDir() || entry.Name() == module || declared[entry.Name()] { + if !entry.IsDir() || !claim.explains(entry.Name()) { continue } if _, err := os.Stat(filepath.Join(modulesDir, entry.Name(), InventoryFilename)); err != nil { @@ -191,18 +325,18 @@ func undeclaredDeliveries(repo, gitopsPath, module string, declared map[string]b // removeUndeclaredDeliveries deletes from the promotion clone the delivered // trees of modules the composition no longer declares (undeclaredDeliveries), -// and answers their paths for the publication to own, stage and show. A -// forward publication to a hosted environment carries the composition's claim -// whole — the cell file it carries no longer admits those modules — so it is -// the one that removes them; a rollback restores one module's tree and claims -// nothing about the rest, and an environment with no host has no cell whose -// claim the trees contradict. Nothing is staged here: the removal reaches the -// promotion only with everything else the publication writes, after the hold. -func removeUndeclaredDeliveries(repo, gitopsPath, module string, env *environments.Environment, restoreRevision string, declared map[string]bool) ([]string, error) { +// and answers their paths for the publication to stage and show. A forward +// publication to a hosted environment carries the composition's claim whole — +// the cell file it carries no longer admits those modules — so it is the one +// that removes them; a rollback restores one module's tree and claims nothing +// about the rest, and an environment with no host has no cell whose claim the +// trees contradict. Nothing is staged here: the removal reaches the promotion +// only with everything else the publication writes, after the hold. +func removeUndeclaredDeliveries(repo, gitopsPath string, env *environments.Environment, restoreRevision string, claim *compositionClaim) ([]string, error) { if env == nil || env.Host == nil || restoreRevision != "" { return nil, nil } - deliveries, err := undeclaredDeliveries(repo, gitopsPath, module, declared) + deliveries, err := undeclaredDeliveries(repo, gitopsPath, claim) if err != nil { return nil, err } diff --git a/pkg/gitops/cell_removals_test.go b/pkg/gitops/cell_removals_test.go index 1624e080..dd21899f 100644 --- a/pkg/gitops/cell_removals_test.go +++ b/pkg/gitops/cell_removals_test.go @@ -38,6 +38,22 @@ func publicationCellOf(t *testing.T, workspace *resources.Workspace, env *enviro return cell } +// holdAgainst holds the publication's cell file to the base revision's, as a +// publication does: the base file read from the delivery repository, the +// claim given. +func holdAgainst(t *testing.T, ctx context.Context, cell *publicationCell, env *environments.Environment, repo, baseRevision string, allowed []string, claim *compositionClaim) (named, explained []string, err error) { + t.Helper() + base, err := baseCellFile(ctx, cell, repo, "main", baseRevision) + require.NoError(t, err) + return holdCellToBase(cell, env, "main", base, allowed, claim) +} + +// advancedClaim is the claim of a checkout that has seen the base's +// composition: every module it does not declare was removed. +func advancedClaim(workspace *resources.Workspace) *compositionClaim { + return &compositionClaim{declared: composedModules(workspace), advanced: true, revision: strings.Repeat("b", 40)} +} + // A publication holds the workspace's cell file to the one its delivery base // branch carries: a file that drops a module's namespace (and with it that // module's consumer edges on another's endpoints) is refused, naming each @@ -56,8 +72,8 @@ func TestPublishRefusesACellFileThatRemovesWhatTheBaseAdmits(t *testing.T) { require.NoError(t, err) repo, base := cellBase(t, complete) - declared := composedModules(workspace) - removals, explained, err := holdCellToBase(ctx, publicationCellOf(t, workspace, env), env, repo, "main", base, nil, declared) + claim := advancedClaim(workspace) + removals, explained, err := holdAgainst(t, ctx, publicationCellOf(t, workspace, env), env, repo, base, nil, claim) require.NoError(t, err, "a full render removes nothing") require.Empty(t, removals) require.Empty(t, explained) @@ -76,17 +92,17 @@ func TestPublishRefusesACellFileThatRemovesWhatTheBaseAdmits(t *testing.T) { require.NoError(t, os.WriteFile(full.Path, dropped, 0o600)) dropping := publicationCellOf(t, workspace, env) - _, _, err = holdCellToBase(ctx, dropping, env, repo, "main", base, nil, declared) + _, _, err = holdAgainst(t, ctx, dropping, env, repo, base, nil, claim) require.Error(t, err) require.Contains(t, err.Error(), "namespace acme-billing (module billing, 2 workload(s))") require.Contains(t, err.Error(), "consumer billing/ledger of acme-shop/api/grpc") require.Contains(t, err.Error(), "--allow-cell-removal (billing)") - _, _, err = holdCellToBase(ctx, dropping, env, repo, "main", base, []string{"shop"}, declared) + _, _, err = holdAgainst(t, ctx, dropping, env, repo, base, []string{"shop"}, claim) require.Error(t, err, "naming the endpoint's module covers its consumer edge, but not billing's namespace") require.NotContains(t, err.Error(), "consumer billing/ledger") require.Contains(t, err.Error(), "namespace acme-billing") - removals, explained, err = holdCellToBase(ctx, dropping, env, repo, "main", base, []string{"billing"}, declared) + removals, explained, err = holdAgainst(t, ctx, dropping, env, repo, base, []string{"billing"}, claim) require.NoError(t, err) require.Equal(t, []string{"namespace acme-billing (module billing, 2 workload(s))", "consumer billing/ledger of acme-shop/api/grpc"}, removals) require.Empty(t, explained, "billing is still composed: its removal is the flag's to confirm") @@ -95,7 +111,7 @@ func TestPublishRefusesACellFileThatRemovesWhatTheBaseAdmits(t *testing.T) { require.NoError(t, os.Remove(filepath.Join(empty, filepath.FromSlash(deliveryCellPath("", "staging"))))) runGit(t, empty, "commit", "-am", "no cell file") emptyBase := runGit(t, empty, "rev-parse", "HEAD") - removals, _, err = holdCellToBase(ctx, dropping, env, empty, "main", emptyBase, nil, declared) + removals, _, err = holdAgainst(t, ctx, dropping, env, empty, emptyBase, nil, claim) require.NoError(t, err, "a base with no cell file has nothing to hold the workspace's to") require.Empty(t, removals) } @@ -132,13 +148,75 @@ func TestARemovalTheCompositionExplainsNeedsNoNaming(t *testing.T) { require.Equal(t, []string{"deployments/modules/legacy"}, rendered.Removed) require.NoDirExists(t, moduleRenderDestination(workspace, "legacy")) - named, explained, err := holdCellToBase(ctx, publicationCellOf(t, workspace, env), env, repo, "main", base, nil, composedModules(workspace)) + named, explained, err := holdAgainst(t, ctx, publicationCellOf(t, workspace, env), env, repo, base, nil, advancedClaim(workspace)) require.NoError(t, err, "a removal the composition explains needs no --allow-cell-removal") require.Empty(t, named) require.Equal(t, []string{ "namespace acme-legacy (module legacy, 2 workload(s)): the composition no longer declares module legacy", "consumer legacy/worker of acme-shop/api/grpc: the composition no longer declares module legacy", }, explained) + + // The same cell file from a checkout that has not seen the base's + // composition derives the same absence, and the composition explains + // nothing: the removal is the flag's to confirm, and the refusal says why. + stale := &compositionClaim{declared: composedModules(workspace), revision: strings.Repeat("c", 40)} + _, _, err = holdAgainst(t, ctx, publicationCellOf(t, workspace, env), env, repo, base, nil, stale) + require.Error(t, err) + require.Contains(t, err.Error(), "--allow-cell-removal (legacy)") + require.Contains(t, err.Error(), "does not contain "+strings.Repeat("c", 40)) + require.Contains(t, err.Error(), "may be a stale checkout's") + _, _, err = holdAgainst(t, ctx, publicationCellOf(t, workspace, env), env, repo, base, nil, &compositionClaim{declared: composedModules(workspace)}) + require.Error(t, err) + require.Contains(t, err.Error(), "names no revision it was rendered from") + named, explained, err = holdAgainst(t, ctx, publicationCellOf(t, workspace, env), env, repo, base, []string{"legacy"}, stale) + require.NoError(t, err, "naming the module confirms the removal as before") + require.Len(t, named, 2) + require.Empty(t, explained) +} + +// The composition's claim is advanced only from a checkout that has seen the +// composition the base's cell file was rendered from. The workspace +// repository holding the delivery base commit is the exact test (the file's +// render was committed there); otherwise the revision the base file names is +// what the checkout must descend from; a base naming none advances nothing. +func TestTheCompositionClaimIsAdvancedOnlyFromACheckoutThatSawTheBase(t *testing.T) { + ctx := context.Background() + workspace := writeCellWorkspace(t) + dir := workspace.Dir() + runGit(t, dir, "init", "--initial-branch=main") + runGit(t, dir, "add", "-A") + runGit(t, dir, "commit", "-q", "-m", "the composition that named legacy") + first := runGit(t, dir, "rev-parse", "HEAD") + elsewhere := strings.Repeat("d", 40) + + base := &cellfile.File{Source: &cellfile.Source{Revision: first}} + claim := newCompositionClaim(ctx, workspace, "shop", elsewhere, base) + require.True(t, claim.advanced, "a checkout at the revision the base was rendered from has seen its composition") + require.Equal(t, first, claim.revision) + require.True(t, claim.explains("legacy")) + require.False(t, claim.explains("shop"), "a composed module is never explained away") + require.False(t, claim.explains("billing")) + require.False(t, newCompositionClaim(ctx, workspace, "shop", elsewhere, nil).advanced, "a base with no cell file advances nothing") + require.False(t, newCompositionClaim(ctx, workspace, "shop", elsewhere, &cellfile.File{}).advanced, "a base naming no source advances nothing") + + // The workspace is the delivery repository: the base commit is the one + // that committed the render, a child of the revision the file names. A + // checkout at that revision has NOT seen it. + require.NoError(t, os.WriteFile(filepath.Join(dir, "rendered.txt"), []byte("the render, and the modules: edit that rode with it\n"), 0o644)) + runGit(t, dir, "add", "-A") + runGit(t, dir, "commit", "-q", "-m", "render") + second := runGit(t, dir, "rev-parse", "HEAD") + runGit(t, dir, "checkout", "-q", "--detach", first) + claim = newCompositionClaim(ctx, workspace, "shop", second, base) + require.False(t, claim.advanced, "the checkout does not contain the delivery base commit the workspace repository holds") + require.Equal(t, second, claim.revision, "the base commit is judged, not the parent the file names") + runGit(t, dir, "checkout", "-q", "main") + require.True(t, newCompositionClaim(ctx, workspace, "shop", second, base).advanced) + + // A checkout whose history does not contain the named revision at all. + runGit(t, dir, "checkout", "-q", "--orphan", "stale") + runGit(t, dir, "commit", "-q", "--allow-empty", "-m", "a branch that never saw it") + require.False(t, newCompositionClaim(ctx, workspace, "shop", elsewhere, base).advanced) } // composeLegacyModule adds a third module, legacy, to the cell fixture: one diff --git a/pkg/gitops/publish.go b/pkg/gitops/publish.go index ba43620b..25a683cc 100644 --- a/pkg/gitops/publish.go +++ b/pkg/gitops/publish.go @@ -220,14 +220,15 @@ func preparePublish( if err != nil { return fail(err) } - declared := composedModules(workspace) - removedModules, err := removeUndeclaredDeliveries(repo, pathRoot, request.Module, env, restoreRevision, declared) + baseCell, claim, removedModules, err := judgeCompositionRemovals(ctx, workspace, env, cell, &compositionBase{ + repo: repo, gitopsPath: pathRoot, branch: baseBranch, revision: baseRevision, + module: request.Module, restoreRevision: restoreRevision, + }) if err != nil { return fail(err) } - owned = append(owned, removedModules...) publishedPaths = append(publishedPaths, removedModules...) - if err = refuseUnrelatedPromotionChanges(ctx, repo, baseRevision, promotionBranch, owned, branchRevision, replacesRevision); err != nil { + if err = refuseUnrelatedPromotionChanges(ctx, repo, baseRevision, promotionBranch, owned, claim.owns(pathRoot), branchRevision, replacesRevision); err != nil { return fail(err) } startRevision := baseRevision @@ -256,7 +257,7 @@ func preparePublish( return fail(err) } staleConsumers := describeStaleConsumers(staleOnBase) - allowedRemovals, undeclaredRemovals, err := holdCellToBase(ctx, cell, env, repo, baseBranch, baseRevision, request.AllowCellRemovals, declared) + allowedRemovals, undeclaredRemovals, err := holdCellToBase(cell, env, baseBranch, baseCell, request.AllowCellRemovals, claim) if err != nil { return fail(err) } @@ -574,14 +575,14 @@ func refuseSharedDeliveryPath(workspace *resources.Workspace, environment string // tip a publish continues, or the stale tip it replaces — to the module's own // path and its cell: anything else on it is someone else's work, which this // publish must neither carry nor overwrite. An empty revision is no branch. -func refuseUnrelatedPromotionChanges(ctx context.Context, repo, baseRevision, promotionBranch string, published []string, branchRevisions ...string) error { +func refuseUnrelatedPromotionChanges(ctx context.Context, repo, baseRevision, promotionBranch string, published []string, alsoOwns func(string) bool, branchRevisions ...string) error { owned := func(changed string) bool { for _, path := range published { if changed == path || strings.HasPrefix(changed, path+"/") { return true } } - return false + return alsoOwns != nil && alsoOwns(changed) } for _, branchRevision := range branchRevisions { if branchRevision == "" { diff --git a/pkg/gitops/publish_cell_test.go b/pkg/gitops/publish_cell_test.go index 28886dbe..7106ef62 100644 --- a/pkg/gitops/publish_cell_test.go +++ b/pkg/gitops/publish_cell_test.go @@ -46,23 +46,68 @@ func seedDelivered(t *testing.T, remote, message string, files map[string][]byte gitRun(t, work, "push", "origin", "main") } +// committedWorkspace makes the fixture workspace a git checkout with its +// composition committed, so the cell file it renders names the revision it +// was rendered from and a later publication can be judged against it. +func committedWorkspace(t *testing.T, workspace *resources.Workspace) string { + t.Helper() + dir := workspace.Dir() + if exec.Command("git", "-C", dir, "rev-parse", "HEAD").Run() != nil { + gitRun(t, dir, "init", "-q", "--initial-branch=main") + gitRun(t, dir, "config", "user.name", "Codefly Test") + gitRun(t, dir, "config", "user.email", "codefly@example.com") + gitRun(t, dir, "config", "commit.gpgsign", "false") + gitRun(t, dir, "add", "-A") + gitRun(t, dir, "commit", "-q", "-m", "the composition") + } + return gitOutput(t, dir, "rev-parse", "HEAD") +} + +// renderedFrom is the cell file data as the base carries it after a render +// from revision: the base's provenance, stated as the render that produced it +// states it (cellSource), whatever this fixture's own render recorded. +func renderedFrom(t *testing.T, data []byte, revision string) []byte { + t.Helper() + file, err := cellfile.Parse(data) + if err != nil { + t.Fatal(err) + } + file.Source = &cellfile.Source{Repo: "https://example.test/delivery.git", Revision: revision, Branch: "main"} + encoded, err := file.Encode() + if err != nil { + t.Fatal(err) + } + return encoded +} + +// legacyDelivery is a delivered tree of a module the fixture's composition +// does not declare, plus a directory under the modules path that is not a +// delivery, as files to seed on the delivery base. +func legacyDelivery() (string, map[string][]byte) { + legacy := deliveryModulePath("environments", "legacy") + return legacy, map[string][]byte{ + legacy + "/" + InventoryFilename: []byte(`{"module":"legacy","environment":"production"}`), + legacy + "/services/worker/kustomization.yaml": []byte("resources: []\n"), + deliveryModulesPath("environments") + "/notes/README.md": []byte("not a delivery\n"), + } +} + // TestPublishRemovesADeliveredModuleTheCompositionNoLongerDeclares: a module // removed from `modules:` left its delivered tree in the delivery repository, // and with it the ApplicationSet that kept stamping Applications whose every // pod the cell — which no longer named the module — refused. A forward -// publication to a hosted environment now removes such a tree with the rest -// of what it writes, shows it in the plan, and leaves alone a directory under -// the modules path that is not a delivery (no render record). +// publication to a hosted environment, from a checkout that has seen the +// base's composition, now removes such a tree with the rest of what it +// writes, shows it in the plan, and leaves alone a directory under the +// modules path that is not a delivery (no render record). func TestPublishRemovesADeliveredModuleTheCompositionNoLongerDeclares(t *testing.T) { ctx, workspace, remote := developerPublishWorkspace(t) + head := committedWorkspace(t, workspace) env := selectedEnvironment(t, workspace, "production") - renderedCell(t, ctx, workspace, env) - legacy := deliveryModulePath("environments", "legacy") - seedDelivered(t, remote, "a module since removed from the composition", map[string][]byte{ - legacy + "/" + InventoryFilename: []byte(`{"module":"legacy","environment":"production"}`), - legacy + "/services/worker/kustomization.yaml": []byte("resources: []\n"), - deliveryModulesPath("environments") + "/notes/README.md": []byte("not a delivery\n"), - }) + _, data := renderedCell(t, ctx, workspace, env) + legacy, files := legacyDelivery() + files[deliveryCellPath("environments", "production")] = renderedFrom(t, data, head) + seedDelivered(t, remote, "a module since removed from the composition", files) request := PublishRequest{ Module: "payments", Environment: "production", Local: true, Signer: &fakeSigner{nonce: true}, @@ -96,6 +141,90 @@ func TestPublishRemovesADeliveredModuleTheCompositionNoLongerDeclares(t *testing } } +// TestAPromotionOwnsTheRemovalItsOwnBranchCarries: the branch guard holds a +// promotion branch to what this module's publication writes, and a removed +// tree is no longer in the clone to be listed — the branch's earlier commit +// deleted it — so a second publish on the module's own branch (its promotion +// still open, or a retry) must own the removal by rule rather than by what it +// finds on disk, or it refuses its own earlier work as someone else's. +func TestAPromotionOwnsTheRemovalItsOwnBranchCarries(t *testing.T) { + ctx, workspace, remote := developerPublishWorkspace(t) + head := committedWorkspace(t, workspace) + env := selectedEnvironment(t, workspace, "production") + _, data := renderedCell(t, ctx, workspace, env) + legacy, files := legacyDelivery() + files[deliveryCellPath("environments", "production")] = renderedFrom(t, data, head) + seedDelivered(t, remote, "a module since removed from the composition", files) + + // The module's earlier promotion: the cell file carried, legacy removed. + work := t.TempDir() + gitRun(t, "", "clone", "-q", remote, work) + gitRun(t, work, "config", "user.name", "Codefly Test") + gitRun(t, work, "config", "user.email", "codefly@example.com") + gitRun(t, work, "config", "commit.gpgsign", "false") + gitRun(t, work, "checkout", "-q", "-b", "codefly/promote-payments-production") + gitRun(t, work, "rm", "-r", "-q", legacy) + gitRun(t, work, "commit", "-q", "-m", "an earlier promotion of payments, removing legacy") + gitRun(t, work, "push", "-q", "origin", "codefly/promote-payments-production") + + plan, err := PlanPublish(ctx, workspace, &PublishRequest{ + Module: "payments", Environment: "production", Local: true, Signer: &fakeSigner{nonce: true}, + PromotionBranch: "codefly/promote-payments-production", + }) + if err != nil { + t.Fatalf("a promotion branch carrying the module's own removal was refused: %v", err) + } + if len(plan.RemovedModules) != 0 { + t.Fatalf("the continued branch already removed %s; the plan removes %v again", legacy, plan.RemovedModules) + } +} + +// TestAStaleCheckoutStillNeedsTheFlagToWithdrawAModule: a checkout that never +// had a module and one that dropped it derive the same cell file. Only a +// checkout that has seen the composition the base's file was rendered from +// may remove on the composition's word; from any other the removal is still +// the flag's to confirm, and even named, the delivered tree is not pruned. +func TestAStaleCheckoutStillNeedsTheFlagToWithdrawAModule(t *testing.T) { + ctx, workspace, remote := developerPublishWorkspace(t) + committedWorkspace(t, workspace) + env := selectedEnvironment(t, workspace, "production") + _, data := renderedCell(t, ctx, workspace, env) + // The base's file: rendered from a composition this checkout never saw, + // which named a module legacy this checkout does not declare. + file, err := cellfile.Parse(renderedFrom(t, data, strings.Repeat("e", 40))) + if err != nil { + t.Fatal(err) + } + file.Namespaces = append(file.Namespaces, cellfile.Namespace{Name: "legacy", Module: "legacy", Workloads: []cellfile.Workload{{Name: "worker", Kind: "Deployment"}}}) + base, err := file.Encode() + if err != nil { + t.Fatal(err) + } + legacy, files := legacyDelivery() + files[deliveryCellPath("environments", "production")] = base + seedDelivered(t, remote, "a composition this checkout never saw", files) + + request := PublishRequest{ + Module: "payments", Environment: "production", Local: true, Signer: &fakeSigner{nonce: true}, + PromotionBranch: "codefly/promote-payments-production", + } + _, err = PlanPublish(ctx, workspace, &request) + if err == nil || !strings.Contains(err.Error(), "--allow-cell-removal (legacy)") || !strings.Contains(err.Error(), "may be a stale checkout's") { + t.Fatalf("a stale checkout withdrew a module on the composition's word: %v", err) + } + request.AllowCellRemovals = []string{"legacy"} + plan, err := PlanPublish(ctx, workspace, &request) + if err != nil { + t.Fatal(err) + } + if len(plan.RemovedModules) != 0 || slices.ContainsFunc(plan.Changed, func(path string) bool { return strings.HasPrefix(path, legacy) }) { + t.Fatalf("a removal the flag confirms pruned the delivered tree: %v %v", plan.RemovedModules, plan.Changed) + } + if len(plan.CellRemovals) != 1 || len(plan.UndeclaredCellRemovals) != 0 { + t.Fatalf("the plan reports %v named and %v explained; want one named", plan.CellRemovals, plan.UndeclaredCellRemovals) + } +} + // renderedCell renders the environment's cell file from the workspace's module // trees and answers it with its bytes. func renderedCell(t *testing.T, ctx context.Context, workspace *resources.Workspace, env *environments.Environment) (CellResult, []byte) { From cbef7b98a2c0140f2ef2d1b2ff6e718f23ef16e5 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Wed, 4 Nov 2026 00:06:13 -0500 Subject: [PATCH 3/6] fix(publish): an unfetched checkout of the delivery repository is not advanced The base's cell file names the render's parent as its source, and a `modules:` edit rides in the render's own commit, so a checkout of the one repository that had not fetched the base commit sat at that parent, descended from it, and was judged to have seen a composition it never had. The delivery clone holding the workspace revision the file names is what tells one repository from two: then only the base commit counts, and a checkout that lacks it is held as before, told which commit it lacks. The source revision is judged only for separate repositories. Co-Authored-By: Claude Fable 5.1 --- docs/commands.md | 14 ++++++---- pkg/gitops/cell_removals.go | 37 +++++++++++++++----------- pkg/gitops/cell_removals_test.go | 29 +++++++++++++++----- pkg/gitops/publish_cell_test.go | 45 ++++++++++++++++++++++++++++++++ 4 files changed, 99 insertions(+), 26 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index 24609307..4184ece4 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -1209,11 +1209,15 @@ publication also removes that module's delivered tree from the cell no longer admits anything of it, and the ApplicationSet that tree carries would keep stamping Applications whose every pod the cell refuses. A checkout that never had a module and one that dropped it derive the same cell -file, so the history decides: the checkout must contain the delivery base -commit when the workspace repository holds it (the workspace is the delivery -repository), or else the `source.revision` the base's cell file names; a -checkout that contains neither — a branch behind the base, a base file naming -no source — is held as before, the refusal says which revision it lacks, and +file, so the history decides: when the workspace repository is the delivery +repository (it holds the base commit, or the delivery holds the workspace +revision the base's file names as its `source.revision`), the checkout must +contain the delivery base commit itself — the file's source revision is the +render's parent, and a `modules:` edit rides in the render's own commit, so an +unfetched checkout at that parent has not seen it; when the two repositories +are separate, the checkout must contain that source revision. A checkout that +does not — a branch behind the base, an unfetched one, a base file naming no +source — is held as before, the refusal says which revision it lacks, and naming the module confirms the removal without pruning the tree. A promotion of a module owns, on its own branch, the removal of such trees, so continuing or retrying the publication that removed one is not refused as someone else's diff --git a/pkg/gitops/cell_removals.go b/pkg/gitops/cell_removals.go index c8e06cd0..1da19bd8 100644 --- a/pkg/gitops/cell_removals.go +++ b/pkg/gitops/cell_removals.go @@ -116,16 +116,19 @@ type compositionClaim struct { revision string } -// newCompositionClaim judges the checkout against the delivery base: the base -// revision itself when the workspace repository holds that commit (the -// workspace IS the delivery repository, and the file's render was committed -// there), or else the workspace revision the base's cell file says it was -// rendered from (cellSource). The base commit is preferred because it is -// exact: the source revision is the render's parent, and a `modules:` edit -// may ride in the render's own commit. A base with no cell file, or one -// naming no source, leaves nothing to descend from, and the claim is not -// advanced. -func newCompositionClaim(ctx context.Context, workspace *resources.Workspace, module, baseRevision string, base *cellfile.File) *compositionClaim { +// newCompositionClaim judges the checkout against the delivery base. The +// commit the checkout must contain is the base revision itself whenever the +// workspace repository and the delivery repository are one — the workspace +// holds the base commit, or the delivery clone holds the workspace revision +// the base's cell file says it was rendered from (cellSource) — because that +// revision is the render's PARENT and a `modules:` edit rides in the render's +// own commit, so a checkout forked at the parent has not seen it; an unfetched +// checkout of one repository is therefore not advanced, and the hold says +// which commit it lacks. Only when the two repositories are separate (the +// clone does not hold the source revision) is the source revision what the +// checkout must contain. A base with no cell file, or one naming no source +// while the workspace lacks the base commit, leaves nothing to descend from. +func newCompositionClaim(ctx context.Context, workspace *resources.Workspace, clone, module, baseRevision string, base *cellfile.File) *compositionClaim { claim := &compositionClaim{module: module, declared: composedModules(workspace)} if base == nil { return claim @@ -135,11 +138,15 @@ func newCompositionClaim(ctx context.Context, workspace *resources.Workspace, mo if err != nil { return claim } + source := "" + if base.Source != nil { + source = base.Source.Revision + } switch { - case gitHasCommit(ctx, dir, baseRevision): + case gitHasCommit(ctx, dir, baseRevision), source != "" && gitHasCommit(ctx, clone, source): claim.revision = baseRevision - case base.Source != nil && base.Source.Revision != "": - claim.revision = base.Source.Revision + case source != "": + claim.revision = source default: return claim } @@ -199,7 +206,7 @@ func judgeCompositionRemovals(ctx context.Context, workspace *resources.Workspac if err != nil { return nil, nil, nil, err } - claim := newCompositionClaim(ctx, workspace, base.module, base.revision, baseCell) + claim := newCompositionClaim(ctx, workspace, base.repo, base.module, base.revision, baseCell) removed, err := removeUndeclaredDeliveries(base.repo, base.gitopsPath, env, base.restoreRevision, claim) if err != nil { return nil, nil, nil, err @@ -284,7 +291,7 @@ func holdCellToBase(cell *publicationCell, env *environments.Environment, baseBr if claim.revision == "" { message += fmt.Sprintf(". This checkout's composition does not declare %s, but %s's cell file names no revision it was rendered from, so whether the composition removed %s or never had it cannot be told from here", strings.Join(stale, ", "), baseBranch, strings.Join(stale, ", ")) } else { - message += fmt.Sprintf(". This checkout's composition does not declare %s, but the checkout does not contain %s, which %s's cell file was rendered from, so the removal may be a stale checkout's rather than the composition's: bring the checkout up to date and render again, or name it", strings.Join(stale, ", "), claim.revision, baseBranch) + message += fmt.Sprintf(". This checkout's composition does not declare %s, but the checkout does not contain %s, which %s's cell file comes from, so the removal may be a stale checkout's rather than the composition's: bring the checkout up to date and render again, or name it", strings.Join(stale, ", "), claim.revision, baseBranch) } } return nil, nil, errors.New(message) diff --git a/pkg/gitops/cell_removals_test.go b/pkg/gitops/cell_removals_test.go index dd21899f..23042e88 100644 --- a/pkg/gitops/cell_removals_test.go +++ b/pkg/gitops/cell_removals_test.go @@ -188,16 +188,20 @@ func TestTheCompositionClaimIsAdvancedOnlyFromACheckoutThatSawTheBase(t *testing runGit(t, dir, "commit", "-q", "-m", "the composition that named legacy") first := runGit(t, dir, "rev-parse", "HEAD") elsewhere := strings.Repeat("d", 40) + // A delivery repository of its own, holding none of the workspace's history. + separate := t.TempDir() + runGit(t, separate, "init", "--initial-branch=main") + runGit(t, separate, "commit", "-q", "--allow-empty", "-m", "delivery") base := &cellfile.File{Source: &cellfile.Source{Revision: first}} - claim := newCompositionClaim(ctx, workspace, "shop", elsewhere, base) + claim := newCompositionClaim(ctx, workspace, separate, "shop", elsewhere, base) require.True(t, claim.advanced, "a checkout at the revision the base was rendered from has seen its composition") require.Equal(t, first, claim.revision) require.True(t, claim.explains("legacy")) require.False(t, claim.explains("shop"), "a composed module is never explained away") require.False(t, claim.explains("billing")) - require.False(t, newCompositionClaim(ctx, workspace, "shop", elsewhere, nil).advanced, "a base with no cell file advances nothing") - require.False(t, newCompositionClaim(ctx, workspace, "shop", elsewhere, &cellfile.File{}).advanced, "a base naming no source advances nothing") + require.False(t, newCompositionClaim(ctx, workspace, separate, "shop", elsewhere, nil).advanced, "a base with no cell file advances nothing") + require.False(t, newCompositionClaim(ctx, workspace, separate, "shop", elsewhere, &cellfile.File{}).advanced, "a base naming no source advances nothing") // The workspace is the delivery repository: the base commit is the one // that committed the render, a child of the revision the file names. A @@ -207,16 +211,29 @@ func TestTheCompositionClaimIsAdvancedOnlyFromACheckoutThatSawTheBase(t *testing runGit(t, dir, "commit", "-q", "-m", "render") second := runGit(t, dir, "rev-parse", "HEAD") runGit(t, dir, "checkout", "-q", "--detach", first) - claim = newCompositionClaim(ctx, workspace, "shop", second, base) + claim = newCompositionClaim(ctx, workspace, separate, "shop", second, base) require.False(t, claim.advanced, "the checkout does not contain the delivery base commit the workspace repository holds") require.Equal(t, second, claim.revision, "the base commit is judged, not the parent the file names") runGit(t, dir, "checkout", "-q", "main") - require.True(t, newCompositionClaim(ctx, workspace, "shop", second, base).advanced) + require.True(t, newCompositionClaim(ctx, workspace, separate, "shop", second, base).advanced) + + // The same one repository, from a checkout that has not fetched the base + // commit: the delivery clone holds the workspace revision the file names, + // which is what tells one repository from two, so the base commit is still + // the one to contain, and the checkout at the render's parent lacks it. + clone := t.TempDir() + runGit(t, "", "clone", "-q", dir, clone) + runGit(t, dir, "checkout", "-q", "--detach", first) + unfetched := strings.Repeat("f", 40) + claim = newCompositionClaim(ctx, workspace, clone, "shop", unfetched, base) + require.False(t, claim.advanced, "an unfetched checkout of the delivery repository is at the render's parent, not past it") + require.Equal(t, unfetched, claim.revision) + runGit(t, dir, "checkout", "-q", "main") // A checkout whose history does not contain the named revision at all. runGit(t, dir, "checkout", "-q", "--orphan", "stale") runGit(t, dir, "commit", "-q", "--allow-empty", "-m", "a branch that never saw it") - require.False(t, newCompositionClaim(ctx, workspace, "shop", elsewhere, base).advanced) + require.False(t, newCompositionClaim(ctx, workspace, separate, "shop", elsewhere, base).advanced) } // composeLegacyModule adds a third module, legacy, to the cell fixture: one diff --git a/pkg/gitops/publish_cell_test.go b/pkg/gitops/publish_cell_test.go index 7106ef62..1d92861d 100644 --- a/pkg/gitops/publish_cell_test.go +++ b/pkg/gitops/publish_cell_test.go @@ -225,6 +225,51 @@ func TestAStaleCheckoutStillNeedsTheFlagToWithdrawAModule(t *testing.T) { } } +// TestAnUnfetchedCheckoutOfTheDeliveryRepositoryStillNeedsTheFlag: when the +// workspace repository is the delivery repository, the base's cell file names +// the render's parent as its source, and a checkout at that parent that has +// not fetched the base commit descends from it — yet never saw the `modules:` +// edit that rode in the render's commit. The delivery clone holding that +// workspace revision is what tells one repository from two, so the base +// commit stays the one to contain, and the removal is the flag's to confirm. +func TestAnUnfetchedCheckoutOfTheDeliveryRepositoryStillNeedsTheFlag(t *testing.T) { + ctx, workspace, remote := developerPublishWorkspace(t) + head := committedWorkspace(t, workspace) + // One repository: the delivery remote holds the workspace's history. + gitRun(t, workspace.Dir(), "push", "-q", remote, "HEAD:refs/heads/workspace-history") + env := selectedEnvironment(t, workspace, "production") + _, data := renderedCell(t, ctx, workspace, env) + file, err := cellfile.Parse(renderedFrom(t, data, head)) + if err != nil { + t.Fatal(err) + } + file.Namespaces = append(file.Namespaces, cellfile.Namespace{Name: "legacy", Module: "legacy", Workloads: []cellfile.Workload{{Name: "worker", Kind: "Deployment"}}}) + base, err := file.Encode() + if err != nil { + t.Fatal(err) + } + legacy, files := legacyDelivery() + files[deliveryCellPath("environments", "production")] = base + seedDelivered(t, remote, "the render that named legacy, committed past this checkout", files) + + request := PublishRequest{ + Module: "payments", Environment: "production", Local: true, Signer: &fakeSigner{nonce: true}, + PromotionBranch: "codefly/promote-payments-production", + } + _, err = PlanPublish(ctx, workspace, &request) + if err == nil || !strings.Contains(err.Error(), "--allow-cell-removal (legacy)") || !strings.Contains(err.Error(), "may be a stale checkout's") { + t.Fatalf("an unfetched checkout of the delivery repository withdrew a module on the composition's word: %v", err) + } + request.AllowCellRemovals = []string{"legacy"} + plan, err := PlanPublish(ctx, workspace, &request) + if err != nil { + t.Fatal(err) + } + if len(plan.RemovedModules) != 0 || slices.ContainsFunc(plan.Changed, func(path string) bool { return strings.HasPrefix(path, legacy) }) { + t.Fatalf("a removal the flag confirms pruned the delivered tree: %v %v", plan.RemovedModules, plan.Changed) + } +} + // renderedCell renders the environment's cell file from the workspace's module // trees and answers it with its bytes. func renderedCell(t *testing.T, ctx context.Context, workspace *resources.Workspace, env *environments.Environment) (CellResult, []byte) { From bdeebdbefea7363a3b11a935296140c405929942 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Wed, 4 Nov 2026 00:10:51 -0500 Subject: [PATCH 4/6] fix(render): prune before the derivation holds the tree #1290 made the derivation refuse a tree of a module the workspace does not compose and ask for its removal. The prune now runs before the derivation, so a render satisfies that hold by removing the tree rather than skipping it; the hold stays for any derivation nothing pruned first, and its test asserts it there. Co-Authored-By: Claude Fable 5.1 --- pkg/gitops/cell.go | 62 ++++++++++++++++++++++++----------------- pkg/gitops/cell_test.go | 7 +++-- 2 files changed, 41 insertions(+), 28 deletions(-) diff --git a/pkg/gitops/cell.go b/pkg/gitops/cell.go index 37faea73..27e9b8e4 100644 --- a/pkg/gitops/cell.go +++ b/pkg/gitops/cell.go @@ -56,9 +56,6 @@ type CellResult struct { // declares, deleted by this render (pruneUndeclaredRecords), as paths // relative to the workspace. Removed []string - // undeclared are those records before they are removed: deriveCell - // finds them, RenderCell deletes them once the cell file is written. - undeclared []string // Source reports whether the file names its repository, branch and base // revision. It does not when the environment declares no gitops // repository or the workspace is not a git checkout. @@ -97,35 +94,57 @@ func CellPath(workspace *resources.Workspace, env *environments.Environment) (st // composition that produced them, and writes it. It runs after a module // render, so the file always describes the trees as they stand. func RenderCell(ctx context.Context, workspace *resources.Workspace, env *environments.Environment) (CellResult, error) { - file, result, err := deriveCell(ctx, workspace, env) + removed, err := pruneUndeclaredRecords(workspace) if err != nil { return CellResult{}, err } - if err = writeCell(&file, result.Path); err != nil { + file, result, err := deriveCell(ctx, workspace, env) + if err != nil { return CellResult{}, err } - if result.Removed, err = pruneUndeclaredRecords(workspace, result.undeclared); err != nil { + if err = writeCell(&file, result.Path); err != nil { return CellResult{}, err } + result.Removed = removed return result, nil } -// pruneUndeclaredRecords deletes the render records of modules the composition -// no longer declares, once the cell file they were left out of is written. +// pruneUndeclaredRecords deletes, before the cell file is derived, the render +// records of modules the composition no longer declares, for any environment. // Nothing could render such a module again (a render refuses a module the // workspace does not compose), so its record describes a composition that no // longer exists; left in place it was read back into every cell file derived // here, and a module removed from `modules:` kept its namespace, workloads and -// consumer edges in the cell. It answers the paths removed, relative to the -// workspace, for the render to print. -func pruneUndeclaredRecords(workspace *resources.Workspace, records []string) ([]string, error) { +// consumer edges in the cell. The derivation's own hold on such a tree +// (refuseUncomposedRender) stays what it is: a render satisfies it by removing +// the tree, which is what that refusal asks for, and answers the paths it +// removed, relative to the workspace, for the render to print. A record that +// cannot be read is left for the derivation to name. +func pruneUndeclaredRecords(workspace *resources.Workspace) ([]string, error) { + modulesDir := filepath.Dir(moduleRenderDestination(workspace, "module")) + entries, err := os.ReadDir(modulesDir) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return nil, nil + } + return nil, fmt.Errorf("list rendered modules: %w", err) + } + declared := composedModules(workspace) var removed []string - for _, tree := range records { - if err := os.RemoveAll(tree); err != nil { - return nil, fmt.Errorf("remove the render record of a module the composition no longer declares: %w", err) + for _, entry := range entries { + if !entry.IsDir() { + continue } - relative, err := filepath.Rel(workspace.Dir(), tree) - if err != nil { + tree := filepath.Join(modulesDir, entry.Name()) + inventory, loadErr := LoadInventory(tree) + if loadErr != nil || declared[inventory.Module] { + continue + } + if err = os.RemoveAll(tree); err != nil { + return nil, fmt.Errorf("remove the render record of module %s, which the composition no longer declares: %w", inventory.Module, err) + } + relative, relErr := filepath.Rel(workspace.Dir(), tree) + if relErr != nil { relative = tree } removed = append(removed, filepath.ToSlash(relative)) @@ -187,7 +206,6 @@ func deriveCell(ctx context.Context, workspace *resources.Workspace, env *enviro Source: cellSource(ctx, workspace, env), } result := CellResult{Source: file.Source != nil, ConfiguredEdges: configured} - declared := composedModules(workspace) for _, entry := range entries { if !entry.IsDir() { continue @@ -200,19 +218,11 @@ func deriveCell(ctx context.Context, workspace *resources.Workspace, env *enviro if loadErr != nil { return cellfile.File{}, CellResult{}, fmt.Errorf("the render of module %s cannot be read, so the cell file cannot describe what it delivers; render %s again for %s: %w", entry.Name(), entry.Name(), env.Name, loadErr) } - // The cell file is derived from the composition, not from whatever - // records the checkout holds: a record of a module no longer in - // `modules:` describes nothing the composition delivers, for this - // environment or any other, and is removed once the file is written. - if !declared[inventory.Module] { - result.undeclared = append(result.undeclared, tree) - continue - } if inventory.Environment != env.Name { result.Skipped = append(result.Skipped, inventory.Module) continue } - if err := refuseUncomposedRender(workspace, env, tree, &inventory); err != nil { + if err = refuseUncomposedRender(workspace, env, tree, &inventory); err != nil { return cellfile.File{}, CellResult{}, err } // A tree rendered before the environment excluded one of its services diff --git a/pkg/gitops/cell_test.go b/pkg/gitops/cell_test.go index a4ef16f9..be85ec0a 100644 --- a/pkg/gitops/cell_test.go +++ b/pkg/gitops/cell_test.go @@ -839,14 +839,17 @@ func TestCellFileRefusesATreeOfAModuleTheWorkspaceDoesNotCompose(t *testing.T) { renderCellTree(t, workspace, "billing", "ledger", "acme-billing", "staging") withdrawn := renderCellTree(t, workspace, "proposals", "api", "acme-proposals", "staging") - _, err := RenderCell(context.Background(), workspace, env) + // The derivation holds the tree; a render satisfies the hold by removing + // it (TestRenderCellPrunesTheRecordOfAModuleTheCompositionNoLongerDeclares). + _, _, err := deriveCell(context.Background(), workspace, env) require.Error(t, err) require.Contains(t, err.Error(), "cell file is not written") require.Contains(t, err.Error(), withdrawn.Path+" holds a render of module proposals for staging, but the workspace composes no module of that name") require.Contains(t, err.Error(), "remove that render and the module's delivered tree") - require.NoError(t, os.RemoveAll(withdrawn.Path)) again, err := RenderCell(context.Background(), workspace, env) require.NoError(t, err) require.Equal(t, []string{"billing", "shop"}, again.Modules) + require.Equal(t, []string{"deployments/modules/proposals"}, again.Removed) + require.NoDirExists(t, withdrawn.Path) } From 2e9690877e8b93088eaeada166cad1ece1edab4d Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Wed, 4 Nov 2026 00:17:19 -0500 Subject: [PATCH 5/6] fix(publish): a removal is vouched for by a composition that declared the module With a separate delivery repository the base's cell file names the render's parent, and a render may run on an uncommitted `modules:` edit, so a checkout at that parent descended from it without ever having composed the module the render named, and could withdraw it on the composition's word. The judged revision's own committed configuration must now have declared the module; a composition rendered before it was committed, or a module of an imported workspace, vouches for nothing and the removal stays the flag's to confirm, with the refusal saying so. Co-Authored-By: Claude Fable 5.1 --- docs/commands.md | 13 ++-- pkg/gitops/cell_removals.go | 103 ++++++++++++++++++++++++++----- pkg/gitops/cell_removals_test.go | 27 ++++++-- pkg/gitops/publish_cell_test.go | 88 +++++++++++++++++++++----- 4 files changed, 190 insertions(+), 41 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index 4184ece4..ae8248c7 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -1215,10 +1215,15 @@ revision the base's file names as its `source.revision`), the checkout must contain the delivery base commit itself — the file's source revision is the render's parent, and a `modules:` edit rides in the render's own commit, so an unfetched checkout at that parent has not seen it; when the two repositories -are separate, the checkout must contain that source revision. A checkout that -does not — a branch behind the base, an unfetched one, a base file naming no -source — is held as before, the refusal says which revision it lacks, and -naming the module confirms the removal without pruning the tree. A promotion +are separate, the checkout must contain that source revision, and the +workspace's own configuration **as committed at that revision** must have +declared the module (a render may run on an uncommitted `modules:` edit, and +a module of an imported workspace is another repository's history), or +nothing in this history vouches the module was removed rather than never +composed. A checkout that fails either test — a branch behind the base, an +unfetched one, a base file naming no source, a composition never committed — +is held as before, the refusal says what the history lacked, and naming the +module confirms the removal without pruning the tree. A promotion of a module owns, on its own branch, the removal of such trees, so continuing or retrying the publication that removed one is not refused as someone else's change. A rollback removes none, and a directory under the modules path with diff --git a/pkg/gitops/cell_removals.go b/pkg/gitops/cell_removals.go index 1da19bd8..684fd7b0 100644 --- a/pkg/gitops/cell_removals.go +++ b/pkg/gitops/cell_removals.go @@ -18,6 +18,7 @@ import ( "fmt" "io/fs" "os" + "path" "path/filepath" "slices" "strings" @@ -25,6 +26,7 @@ import ( "github.com/codefly-dev/cli/pkg/environments" "github.com/codefly-dev/cli/pkg/gitops/cellfile" "github.com/codefly-dev/core/resources" + "gopkg.in/yaml.v3" ) // cellRemoval is one thing the base cell file admits that the next one does @@ -96,15 +98,17 @@ func deliveryCellPath(gitopsPath, environment string) string { } // compositionClaim is what a publication may remove from the cell and the -// delivery on the composition's word alone: the modules the composition no +// delivery on the composition's word alone: a module the composition no // longer declares — provided this checkout's composition has moved FORWARD -// from the one that rendered the delivery base's cell file (advanced). A -// checkout that never had a module and one that dropped it derive the same -// cell file, and only the history tells them apart: a checkout that contains -// the commit the base's file came from has seen the composition that named -// the module, so its absence is a removal; one that does not is stale, and -// its publication keeps needing --allow-cell-removal, as every withdrawal -// nobody stated does. +// from the one that rendered the delivery base's cell file, and that +// composition DECLARED the module. A checkout that never had a module and one +// that dropped it derive the same cell file, and only the history tells them +// apart: a checkout that contains the commit the base's file came from, whose +// composition named the module, has seen it declared, so its absence is a +// removal; one that does not is stale, or saw a composition that named the +// module only in an uncommitted edit or an imported workspace, and its +// publication keeps needing --allow-cell-removal, as every withdrawal nobody +// stated does. type compositionClaim struct { // module is the one being published: never a removal of its own. module string @@ -114,6 +118,9 @@ type compositionClaim struct { // commit it had to contain, empty when the base names none. advanced bool revision string + // declaredAt is the module set the workspace's own configuration named at + // revision (declaredAtRevision): what the checkout can vouch was removed. + declaredAt map[string]bool } // newCompositionClaim judges the checkout against the delivery base. The @@ -126,8 +133,12 @@ type compositionClaim struct { // checkout of one repository is therefore not advanced, and the hold says // which commit it lacks. Only when the two repositories are separate (the // clone does not hold the source revision) is the source revision what the -// checkout must contain. A base with no cell file, or one naming no source -// while the workspace lacks the base commit, leaves nothing to descend from. +// checkout must contain — and there a render may have run on an uncommitted +// `modules:` edit, so the judged revision's own configuration must also have +// declared the module (declaredAtRevision), or nothing vouches the module was +// ever composed in this history. A base with no cell file, or one naming no +// source while the workspace lacks the base commit, leaves nothing to descend +// from. func newCompositionClaim(ctx context.Context, workspace *resources.Workspace, clone, module, baseRevision string, base *cellfile.File) *compositionClaim { claim := &compositionClaim{module: module, declared: composedModules(workspace)} if base == nil { @@ -151,9 +162,55 @@ func newCompositionClaim(ctx context.Context, workspace *resources.Workspace, cl return claim } claim.advanced = gitIsAncestor(ctx, dir, claim.revision, head) + if claim.advanced { + claim.declaredAt = declaredAtRevision(ctx, dir, claim.revision) + } return claim } +// committedComposition is the part of a workspace configuration that names the +// modules it composes itself: a modules layout lists them, a flat layout IS +// one module named after the workspace. +type committedComposition struct { + Name string `yaml:"name"` + Layout string `yaml:"layout"` + Modules []struct{ Name string } `yaml:"modules"` + Solutions []struct{ Name string } `yaml:"solutions"` +} + +// declaredAtRevision reads the workspace's own configuration as committed at +// revision and names the modules it declared there: its `modules:` and +// `solutions:`, or the workspace itself when flat. A module reached only +// through an imported workspace is not among them — that import's history is +// another repository's — and a configuration the revision does not carry, or +// that does not parse, declares nothing, so the claim explains nothing. +func declaredAtRevision(ctx context.Context, dir, revision string) map[string]bool { + prefix, err := gitCommand(ctx, dir, "rev-parse", "--show-prefix") + if err != nil { + return nil + } + data, err := gitCommandBytes(ctx, dir, "show", revision+":"+path.Join(prefix, resources.WorkspaceConfigurationName)) + if err != nil { + return nil + } + var committed committedComposition + if err = yaml.Unmarshal(data, &committed); err != nil { + return nil + } + declared := map[string]bool{} + if committed.Layout == string(resources.LayoutKindFlat) && committed.Name != "" { + declared[committed.Name] = true + } + for _, list := range [][]struct{ Name string }{committed.Modules, committed.Solutions} { + for _, reference := range list { + if reference.Name != "" { + declared[reference.Name] = true + } + } + } + return declared +} + // gitHasCommit reports whether the repository holds the commit. func gitHasCommit(ctx context.Context, repo, revision string) bool { _, err := gitCommand(ctx, repo, "cat-file", "-e", revision+"^{commit}") @@ -167,9 +224,25 @@ func (claim *compositionClaim) undeclared(module string) bool { } // explains reports a removal the composition accounts for: the module is no -// longer declared, by a checkout that has seen it declared. +// longer declared, by a checkout that descends from a composition that +// declared it. func (claim *compositionClaim) explains(module string) bool { - return claim.advanced && claim.undeclared(module) + return claim.advanced && claim.undeclared(module) && claim.declaredAt[module] +} + +// whyNotExplained says, for the modules the checkout does not declare whose +// removal it could not vouch for, what the history lacked: the revision to +// contain, or a composition at it that named them. +func (claim *compositionClaim) whyNotExplained(baseBranch string, modules []string) string { + names := strings.Join(modules, ", ") + switch { + case claim.revision == "": + return fmt.Sprintf(". This checkout's composition does not declare %s, but %s's cell file names no revision it was rendered from, so whether the composition removed %s or never had it cannot be told from here", names, baseBranch, names) + case !claim.advanced: + return fmt.Sprintf(". This checkout's composition does not declare %s, but the checkout does not contain %s, which %s's cell file comes from, so the removal may be a stale checkout's rather than the composition's: bring the checkout up to date and render again, or name it", names, claim.revision, baseBranch) + default: + return fmt.Sprintf(". This checkout's composition does not declare %s, and neither did its own configuration at %s, which %s's cell file comes from (a composition rendered before it was committed, or a module of an imported workspace), so nothing here vouches that %s was removed rather than never composed: name it", names, claim.revision, baseBranch, names) + } } // owns answers, for the promotion-branch guard, whether a delivery-repository @@ -288,11 +361,7 @@ func holdCellToBase(cell *publicationCell, env *environments.Environment, baseBr env.Name, path, baseBranch, strings.Join(refused, "; "), env.Name, strings.Join(modules, ", ")) if len(stale) > 0 { slices.Sort(stale) - if claim.revision == "" { - message += fmt.Sprintf(". This checkout's composition does not declare %s, but %s's cell file names no revision it was rendered from, so whether the composition removed %s or never had it cannot be told from here", strings.Join(stale, ", "), baseBranch, strings.Join(stale, ", ")) - } else { - message += fmt.Sprintf(". This checkout's composition does not declare %s, but the checkout does not contain %s, which %s's cell file comes from, so the removal may be a stale checkout's rather than the composition's: bring the checkout up to date and render again, or name it", strings.Join(stale, ", "), claim.revision, baseBranch) - } + message += claim.whyNotExplained(baseBranch, stale) } return nil, nil, errors.New(message) } diff --git a/pkg/gitops/cell_removals_test.go b/pkg/gitops/cell_removals_test.go index 23042e88..2ba1bb1d 100644 --- a/pkg/gitops/cell_removals_test.go +++ b/pkg/gitops/cell_removals_test.go @@ -48,10 +48,14 @@ func holdAgainst(t *testing.T, ctx context.Context, cell *publicationCell, env * return holdCellToBase(cell, env, "main", base, allowed, claim) } -// advancedClaim is the claim of a checkout that has seen the base's -// composition: every module it does not declare was removed. -func advancedClaim(workspace *resources.Workspace) *compositionClaim { - return &compositionClaim{declared: composedModules(workspace), advanced: true, revision: strings.Repeat("b", 40)} +// advancedClaim is the claim of a checkout that descends from a composition +// that declared the modules named (removed) and no longer declares them. +func advancedClaim(workspace *resources.Workspace, removed ...string) *compositionClaim { + claim := &compositionClaim{declared: composedModules(workspace), advanced: true, revision: strings.Repeat("b", 40), declaredAt: map[string]bool{}} + for _, module := range removed { + claim.declaredAt[module] = true + } + return claim } // A publication holds the workspace's cell file to the one its delivery base @@ -148,7 +152,7 @@ func TestARemovalTheCompositionExplainsNeedsNoNaming(t *testing.T) { require.Equal(t, []string{"deployments/modules/legacy"}, rendered.Removed) require.NoDirExists(t, moduleRenderDestination(workspace, "legacy")) - named, explained, err := holdAgainst(t, ctx, publicationCellOf(t, workspace, env), env, repo, base, nil, advancedClaim(workspace)) + named, explained, err := holdAgainst(t, ctx, publicationCellOf(t, workspace, env), env, repo, base, nil, advancedClaim(workspace, "legacy")) require.NoError(t, err, "a removal the composition explains needs no --allow-cell-removal") require.Empty(t, named) require.Equal(t, []string{ @@ -168,6 +172,12 @@ func TestARemovalTheCompositionExplainsNeedsNoNaming(t *testing.T) { _, _, err = holdAgainst(t, ctx, publicationCellOf(t, workspace, env), env, repo, base, nil, &compositionClaim{declared: composedModules(workspace)}) require.Error(t, err) require.Contains(t, err.Error(), "names no revision it was rendered from") + // A checkout past the revision whose composition never named legacy + // either: the render that named it ran on an uncommitted edit. + _, _, err = holdAgainst(t, ctx, publicationCellOf(t, workspace, env), env, repo, base, nil, advancedClaim(workspace)) + require.Error(t, err) + require.Contains(t, err.Error(), "--allow-cell-removal (legacy)") + require.Contains(t, err.Error(), "neither did its own configuration at "+strings.Repeat("b", 40)) named, explained, err = holdAgainst(t, ctx, publicationCellOf(t, workspace, env), env, repo, base, []string{"legacy"}, stale) require.NoError(t, err, "naming the module confirms the removal as before") require.Len(t, named, 2) @@ -181,12 +191,15 @@ func TestARemovalTheCompositionExplainsNeedsNoNaming(t *testing.T) { // what the checkout must descend from; a base naming none advances nothing. func TestTheCompositionClaimIsAdvancedOnlyFromACheckoutThatSawTheBase(t *testing.T) { ctx := context.Background() - workspace := writeCellWorkspace(t) + workspace := composeLegacyModule(t, writeCellWorkspace(t)) dir := workspace.Dir() runGit(t, dir, "init", "--initial-branch=main") runGit(t, dir, "add", "-A") runGit(t, dir, "commit", "-q", "-m", "the composition that named legacy") first := runGit(t, dir, "rev-parse", "HEAD") + // legacy leaves the composition, uncommitted: HEAD is the composition + // that named it, the working tree the one that does not. + workspace = editWorkspaceConfiguration(t, workspace, " - name: legacy\n", "") elsewhere := strings.Repeat("d", 40) // A delivery repository of its own, holding none of the workspace's history. separate := t.TempDir() @@ -200,6 +213,8 @@ func TestTheCompositionClaimIsAdvancedOnlyFromACheckoutThatSawTheBase(t *testing require.True(t, claim.explains("legacy")) require.False(t, claim.explains("shop"), "a composed module is never explained away") require.False(t, claim.explains("billing")) + require.False(t, claim.explains("compute"), "a module the judged composition never named is not vouched for") + require.Equal(t, map[string]bool{"shop": true, "billing": true, "legacy": true}, claim.declaredAt) require.False(t, newCompositionClaim(ctx, workspace, separate, "shop", elsewhere, nil).advanced, "a base with no cell file advances nothing") require.False(t, newCompositionClaim(ctx, workspace, separate, "shop", elsewhere, &cellfile.File{}).advanced, "a base naming no source advances nothing") diff --git a/pkg/gitops/publish_cell_test.go b/pkg/gitops/publish_cell_test.go index 1d92861d..feb64b8c 100644 --- a/pkg/gitops/publish_cell_test.go +++ b/pkg/gitops/publish_cell_test.go @@ -46,19 +46,37 @@ func seedDelivered(t *testing.T, remote, message string, files map[string][]byte gitRun(t, work, "push", "origin", "main") } -// committedWorkspace makes the fixture workspace a git checkout with its -// composition committed, so the cell file it renders names the revision it -// was rendered from and a later publication can be judged against it. -func committedWorkspace(t *testing.T, workspace *resources.Workspace) string { +// committedWorkspace makes the fixture workspace a git checkout whose HEAD +// commits a composition that also declares the modules named (removed), so a +// cell file rendered there names that revision, and a later publication from +// the working tree — which no longer declares them — can be judged against it. +// The fixture is flat, so a declared module is a sibling `modules:` entry. +func committedWorkspace(t *testing.T, workspace *resources.Workspace, removed ...string) string { t.Helper() dir := workspace.Dir() - if exec.Command("git", "-C", dir, "rev-parse", "HEAD").Run() != nil { - gitRun(t, dir, "init", "-q", "--initial-branch=main") - gitRun(t, dir, "config", "user.name", "Codefly Test") - gitRun(t, dir, "config", "user.email", "codefly@example.com") - gitRun(t, dir, "config", "commit.gpgsign", "false") - gitRun(t, dir, "add", "-A") - gitRun(t, dir, "commit", "-q", "-m", "the composition") + config := filepath.Join(dir, resources.WorkspaceConfigurationName) + current, err := os.ReadFile(config) + if err != nil { + t.Fatal(err) + } + committed := string(current) + if len(removed) > 0 { + committed += "modules:\n" + for _, module := range removed { + committed += " - name: " + module + "\n" + } + } + if err = os.WriteFile(config, []byte(committed), 0o644); err != nil { + t.Fatal(err) + } + gitRun(t, dir, "init", "-q", "--initial-branch=main") + gitRun(t, dir, "config", "user.name", "Codefly Test") + gitRun(t, dir, "config", "user.email", "codefly@example.com") + gitRun(t, dir, "config", "commit.gpgsign", "false") + gitRun(t, dir, "add", "-A") + gitRun(t, dir, "commit", "-q", "-m", "the composition") + if err = os.WriteFile(config, current, 0o644); err != nil { + t.Fatal(err) } return gitOutput(t, dir, "rev-parse", "HEAD") } @@ -102,7 +120,7 @@ func legacyDelivery() (string, map[string][]byte) { // modules path that is not a delivery (no render record). func TestPublishRemovesADeliveredModuleTheCompositionNoLongerDeclares(t *testing.T) { ctx, workspace, remote := developerPublishWorkspace(t) - head := committedWorkspace(t, workspace) + head := committedWorkspace(t, workspace, "legacy") env := selectedEnvironment(t, workspace, "production") _, data := renderedCell(t, ctx, workspace, env) legacy, files := legacyDelivery() @@ -149,7 +167,7 @@ func TestPublishRemovesADeliveredModuleTheCompositionNoLongerDeclares(t *testing // finds on disk, or it refuses its own earlier work as someone else's. func TestAPromotionOwnsTheRemovalItsOwnBranchCarries(t *testing.T) { ctx, workspace, remote := developerPublishWorkspace(t) - head := committedWorkspace(t, workspace) + head := committedWorkspace(t, workspace, "legacy") env := selectedEnvironment(t, workspace, "production") _, data := renderedCell(t, ctx, workspace, env) legacy, files := legacyDelivery() @@ -225,6 +243,48 @@ func TestAStaleCheckoutStillNeedsTheFlagToWithdrawAModule(t *testing.T) { } } +// TestACompositionThatNeverDeclaredTheModuleDoesNotRemoveIt: with a separate +// delivery repository the base's cell file names the render's parent, and a +// render may run on an uncommitted `modules:` edit — so a checkout at that +// parent descends from it without ever having composed the module the render +// named. Nothing in its history vouches the module was removed rather than +// never composed, and the removal stays the flag's to confirm. +func TestACompositionThatNeverDeclaredTheModuleDoesNotRemoveIt(t *testing.T) { + ctx, workspace, remote := developerPublishWorkspace(t) + head := committedWorkspace(t, workspace) + env := selectedEnvironment(t, workspace, "production") + _, data := renderedCell(t, ctx, workspace, env) + file, err := cellfile.Parse(renderedFrom(t, data, head)) + if err != nil { + t.Fatal(err) + } + file.Namespaces = append(file.Namespaces, cellfile.Namespace{Name: "legacy", Module: "legacy", Workloads: []cellfile.Workload{{Name: "worker", Kind: "Deployment"}}}) + base, err := file.Encode() + if err != nil { + t.Fatal(err) + } + legacy, files := legacyDelivery() + files[deliveryCellPath("environments", "production")] = base + seedDelivered(t, remote, "a render on an uncommitted composition that named legacy", files) + + request := PublishRequest{ + Module: "payments", Environment: "production", Local: true, Signer: &fakeSigner{nonce: true}, + PromotionBranch: "codefly/promote-payments-production", + } + _, err = PlanPublish(ctx, workspace, &request) + if err == nil || !strings.Contains(err.Error(), "--allow-cell-removal (legacy)") || !strings.Contains(err.Error(), "neither did its own configuration at "+head) { + t.Fatalf("a checkout whose composition never named legacy withdrew it on the composition's word: %v", err) + } + request.AllowCellRemovals = []string{"legacy"} + plan, err := PlanPublish(ctx, workspace, &request) + if err != nil { + t.Fatal(err) + } + if len(plan.RemovedModules) != 0 || slices.ContainsFunc(plan.Changed, func(path string) bool { return strings.HasPrefix(path, legacy) }) { + t.Fatalf("a removal the flag confirms pruned the delivered tree: %v %v", plan.RemovedModules, plan.Changed) + } +} + // TestAnUnfetchedCheckoutOfTheDeliveryRepositoryStillNeedsTheFlag: when the // workspace repository is the delivery repository, the base's cell file names // the render's parent as its source, and a checkout at that parent that has @@ -234,7 +294,7 @@ func TestAStaleCheckoutStillNeedsTheFlagToWithdrawAModule(t *testing.T) { // commit stays the one to contain, and the removal is the flag's to confirm. func TestAnUnfetchedCheckoutOfTheDeliveryRepositoryStillNeedsTheFlag(t *testing.T) { ctx, workspace, remote := developerPublishWorkspace(t) - head := committedWorkspace(t, workspace) + head := committedWorkspace(t, workspace, "legacy") // One repository: the delivery remote holds the workspace's history. gitRun(t, workspace.Dir(), "push", "-q", remote, "HEAD:refs/heads/workspace-history") env := selectedEnvironment(t, workspace, "production") From f50fa22a89bd204189c85619083eac9a82f2542d Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Wed, 4 Nov 2026 00:20:56 -0500 Subject: [PATCH 6/6] fix(publish): a source revision reaches git only as a commit hash The revision is read from the delivery branch's cell file and spliced into git arguments; anything but a full hash is treated as no source. Co-Authored-By: Claude Fable 5.1 --- pkg/gitops/cell_removals.go | 18 +++++++++++++++++- pkg/gitops/cell_removals_test.go | 3 +++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/pkg/gitops/cell_removals.go b/pkg/gitops/cell_removals.go index 684fd7b0..e325725c 100644 --- a/pkg/gitops/cell_removals.go +++ b/pkg/gitops/cell_removals.go @@ -149,8 +149,10 @@ func newCompositionClaim(ctx context.Context, workspace *resources.Workspace, cl if err != nil { return claim } + // The source revision is read from the delivery branch's file and spliced + // into git arguments: only a full commit hash is one. source := "" - if base.Source != nil { + if base.Source != nil && isCommitHash(base.Source.Revision) { source = base.Source.Revision } switch { @@ -211,6 +213,20 @@ func declaredAtRevision(ctx context.Context, dir, revision string) map[string]bo return declared } +// isCommitHash reports whether value is a full lowercase SHA-1 commit hash, +// the one shape a revision read from a file may take before it reaches git. +func isCommitHash(value string) bool { + if len(value) != 40 { + return false + } + for _, r := range value { + if (r < '0' || r > '9') && (r < 'a' || r > 'f') { + return false + } + } + return true +} + // gitHasCommit reports whether the repository holds the commit. func gitHasCommit(ctx context.Context, repo, revision string) bool { _, err := gitCommand(ctx, repo, "cat-file", "-e", revision+"^{commit}") diff --git a/pkg/gitops/cell_removals_test.go b/pkg/gitops/cell_removals_test.go index 2ba1bb1d..5b6b417d 100644 --- a/pkg/gitops/cell_removals_test.go +++ b/pkg/gitops/cell_removals_test.go @@ -217,6 +217,9 @@ func TestTheCompositionClaimIsAdvancedOnlyFromACheckoutThatSawTheBase(t *testing require.Equal(t, map[string]bool{"shop": true, "billing": true, "legacy": true}, claim.declaredAt) require.False(t, newCompositionClaim(ctx, workspace, separate, "shop", elsewhere, nil).advanced, "a base with no cell file advances nothing") require.False(t, newCompositionClaim(ctx, workspace, separate, "shop", elsewhere, &cellfile.File{}).advanced, "a base naming no source advances nothing") + option := newCompositionClaim(ctx, workspace, separate, "shop", elsewhere, &cellfile.File{Source: &cellfile.Source{Revision: "--output=/tmp/x"}}) + require.False(t, option.advanced, "a source revision that is not a commit hash never reaches git") + require.Empty(t, option.revision) // The workspace is the delivery repository: the base commit is the one // that committed the render, a child of the revision the file names. A