issue 1025: the merge gate judges a group's move of a module as a move (ADR 0420 §6)

cmd/mesh-controller/merge_gate.go

@@ -271,6 +271,25 @@ func judgeChange(ctx context.Context, in mergeCheckInput) (mergeVerdict, error)
 	// The change, then each other head of its delivery group laid over it in turn (novox/hq ADR 0239): one
 	// future state of the mesh, judged as the group would leave it.
 	heads := append([]groupChange{{Repository: in.repository, Tree: in.tree, Changed: in.changed}}, in.group...)
+	// **A module one head removes and another defines is a move** (novox/hq ADR 0420 §6, issue 1025): neither
+	// the removal of a module machines run (ADR 0236) nor a second definition of one module name (issue 239).
+	// The change is judged with the module built from the repository that now defines it.
+	moves, err := movesInGroup(heads, sources)
+	if err != nil {
+		return v, err
+	}
+	changeSources := sources
+	if len(moves) > 0 {
+		changeSources = make(map[string]inventory.Source, len(sources))
+		for k, o := range sources {
+			changeSources[k] = o
+		}
+		for name, mv := range moves {
+			o := changeSources[name]
+			o.Repository, o.Path = mv.To, mv.Dir
+			changeSources[name] = o
+		}
+	}
 	for i, h := range heads {
 		var r *mergeReach
 		if h.Repository != "" && len(h.Changed) > 0 {
@@ -289,13 +308,23 @@ func judgeChange(ctx context.Context, in mergeCheckInput) (mergeVerdict, error)
 			continue
 		}
 		var failures []string
-		change, failures, err = shelfWithChange(change, sources, in.facts, h.Repository, h.Tree, v.Modules,
+		scope := scopeOfReach(r, h.Repository)
-			scopeOfReach(r, h.Repository))
+		if scope != nil {
+			for _, mv := range moves {
+				if mv.To == h.Repository {
+					scope[mv.Dir] = true
+				}
+			}
+		}
+		change, failures, err = shelfWithChange(change, changeSources, in.facts, h.Repository, h.Tree, v.Modules, scope)
 		if err != nil {
 			return v, err
 		}
 		v.Failures = append(v.Failures, failures...)
 	}
+	for name := range moves {
+		v.Modules[name] = "moved"
+	}
 	if len(in.group) > 0 {
 		say("a delivery group: %d head(s) composed together", len(heads))
 	}
@@ -332,7 +361,7 @@ func judgeChange(ctx context.Context, in mergeCheckInput) (mergeVerdict, error)
 		}
 	}
 	sort.Strings(added)
-	after, tried, notes, err := composeEveryMachine(ctx, in, change, sources, added)
+	after, tried, notes, err := composeEveryMachine(ctx, in, change, changeSources, added)
 	if err != nil {
 		return v, fmt.Errorf("the mesh with the change could not be raised: %w", err)
 	}
@@ -531,6 +560,98 @@ func sourcesOfFacts(f snapshot.Facts) map[string]inventory.Source {
 	return out
 }
 
+// moveOfModule is a module a delivery group moves: the repository the mesh builds it from, the one whose
+// head now defines it, and its directory there.
+type moveOfModule struct{ From, To, Dir string }
+
+// movesInGroup is the modules one head of a group removes and another defines (novox/hq ADR 0420 §6): a name
+// the mesh builds from a head's repository that the head's tree no longer defines, and some other head's tree
+// defines. A name removed by a head with no other head defining it, or defined by a head the mesh builds from
+// another repository with none removing it, is no move.
+func movesInGroup(heads []groupChange, sources map[string]inventory.Source) (map[string]moveOfModule, error) {
+	if len(heads) < 2 {
+		return nil, nil
+	}
+	defined := make([]map[string]string, len(heads))
+	for i, h := range heads {
+		if h.Tree == "" {
+			continue
+		}
+		d, err := namesDefinedIn(h.Tree)
+		if err != nil {
+			return nil, err
+		}
+		defined[i] = d
+	}
+	var moves map[string]moveOfModule
+	for name, s := range sources {
+		if s.Repository == "" {
+			continue
+		}
+		for i, h := range heads {
+			if h.Tree == "" || !sameRepository(s.Repository, link.SourceMoved{Owner: ownerOf(h.Repository), Repo: repoOf(h.Repository)}) {
+				continue
+			}
+			if _, still := defined[i][name]; still {
+				break
+			}
+			// Removed by head i: defined by another?
+			for j, o := range heads {
+				if dir, ok := defined[j][name]; ok && j != i && o.Repository != h.Repository {
+					if moves == nil {
+						moves = map[string]moveOfModule{}
+					}
+					moves[name] = moveOfModule{From: h.Repository, To: o.Repository, Dir: dir}
+				}
+			}
+			break
+		}
+	}
+	return moves, nil
+}
+
+// namesDefinedIn is the module names a tree's module.json files define, each with its directory; a manifest
+// that does not read is left to shelfWithChange to refuse.
+func namesDefinedIn(tree string) (map[string]string, error) {
+	found := map[string]string{}
+	err := filepath.WalkDir(tree, func(path string, d fs.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+		if d.IsDir() {
+			if path != tree && (ignoredInTree[d.Name()] || strings.HasPrefix(d.Name(), ".")) {
+				return filepath.SkipDir
+			}
+			return nil
+		}
+		if d.Name() != "module.json" {
+			return nil
+		}
+		raw, err := os.ReadFile(path)
+		if err != nil {
+			return err
+		}
+		var m struct {
+			Module string `json:"module"`
+		}
+		if json.Unmarshal(raw, &m) != nil || m.Module == "" {
+			return nil
+		}
+		dir, err := filepath.Rel(tree, filepath.Dir(path))
+		if err != nil {
+			return err
+		}
+		if dir == "." {
+			dir = ""
+		}
+		if _, twice := found[m.Module]; !twice {
+			found[m.Module] = dir
+		}
+		return nil
+	})
+	return found, err
+}
+
 // ignoredInTree are directories a module.json in is not a module of the repository.
 var ignoredInTree = map[string]bool{".git": true, "vendor": true, "node_modules": true, "testdata": true,
 	"examples": true, "fixtures": true}

cmd/mesh-controller/merge_gate_test.go

@@ -434,3 +434,124 @@ func sdkPinnedByGoMod(t *testing.T) string {
 	t.Fatalf("go.mod requires no %s", sdkModule)
 	return ""
 }
+
+// **A module one head of a group removes and another defines is a move** (novox/hq ADR 0420 §6, issue 1025):
+// neither a second definition of one module name (issue 239) nor the removal of a module machines run
+// (ADR 0236). Alone, each head is still refused. This lays the heads over the shelf as judgeChange does,
+// which needs no database.
+func TestAGroupsMoveOfAModuleIsNeitherTwoDefinitionsNorARemoval(t *testing.T) {
+	const catalogueRepo, controllerRepo = "novox/mesh-catalog", "novox/mesh-controller"
+	built := func(repo, path string) inventory.Source {
+		return inventory.Source{Repository: "http://forge.internal:20000/" + repo + ".git", Path: path, BuiltFrom: "c0ffee"}
+	}
+	sources := map[string]inventory.Source{"mover": built(catalogueRepo, "modules/mover")}
+	raw := `{"module":"mover","version":"1"}`
+	base := map[string]catalogue.Manifest{}
+	m, err := catalogue.ParseManifest([]byte(raw))
+	if err != nil {
+		t.Fatal(err)
+	}
+	base["mover"] = m
+	facts := snapshot.Facts{Machines: []snapshot.Machine{{Name: "anchor", Assigned: []string{"mover"}}}}
+
+	catalogueWithout := aTree(t, map[string]string{"other": `{"module":"other","version":"1"}`})
+	controllerWith := func() string {
+		dir := t.TempDir()
+		if err := os.MkdirAll(filepath.Join(dir, "modules", "mover"), 0o755); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.WriteFile(filepath.Join(dir, "modules", "mover", "module.json"), []byte(raw), 0o644); err != nil {
+			t.Fatal(err)
+		}
+		return dir
+	}()
+	removes := groupChange{Repository: catalogueRepo, Tree: catalogueWithout, Changed: []string{"modules/mover/module.json"}}
+	defines := groupChange{Repository: controllerRepo, Tree: controllerWith, Changed: []string{"modules/mover/module.json"}}
+
+	lay := func(heads ...groupChange) (map[string]catalogue.Manifest, []string, map[string]string) {
+		t.Helper()
+		moves, err := movesInGroup(heads, sources)
+		if err != nil {
+			t.Fatal(err)
+		}
+		at := sources
+		if len(moves) > 0 {
+			at = map[string]inventory.Source{}
+			for k, v := range sources {
+				at[k] = v
+			}
+			for name, mv := range moves {
+				o := at[name]
+				o.Repository, o.Path = mv.To, mv.Dir
+				at[name] = o
+			}
+		}
+		shelf, said := base, map[string]string{}
+		var failures []string
+		for _, h := range heads {
+			var f []string
+			if shelf, f, err = shelfWithChange(shelf, at, facts, h.Repository, h.Tree, said, nil); err != nil {
+				t.Fatal(err)
+			}
+			failures = append(failures, f...)
+		}
+		for name := range moves {
+			said[name] = "moved"
+		}
+		return shelf, failures, said
+	}
+	for _, order := range [][]groupChange{{removes, defines}, {defines, removes}} {
+		shelf, failures, said := lay(order...)
+		if len(failures) > 0 {
+			t.Fatalf("a move of a module machines run, laid %s first, was refused: %v", order[0].Repository, failures)
+		}
+		if said["mover"] != "moved" {
+			t.Errorf("the move reads %v, not moved", said)
+		}
+		if _, held := shelf["mover"]; !held {
+			t.Errorf("the moved module is not on the shelf after laying %s first", order[0].Repository)
+		}
+	}
+	// Alone, a head is still refused: a second definition (issue 239), and a removal of what runs (ADR 0236).
+	if _, failures, _ := lay(defines); len(failures) != 1 || !strings.Contains(failures[0], "issue 239") {
+		t.Errorf("a definition of a module another repository builds, alone: %v", failures)
+	}
+	if _, failures, _ := lay(removes); len(failures) != 1 || !strings.Contains(failures[0], "ADR 0236") {
+		t.Errorf("the removal of a module machines run, alone: %v", failures)
+	}
+}
+
+// A group that moves a module machines run passes the gate whole, in either order of its heads, and says the
+// module moved (novox/hq ADR 0420 §6, issue 1025); a head alone is still refused. Needs the test store.
+func TestADeliveryGroupMovingAModuleMachinesRunPassesTheGate(t *testing.T) {
+	f, manifests := catalogueMeshWithAnApp(t)
+	catTree := aTree(t, withEdit(manifests, "album", ""))
+	app := t.TempDir()
+	if err := os.MkdirAll(filepath.Join(app, "modules", "album"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(app, "modules", "album", "module.json"), []byte(manifests["album"]), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	removes := groupChange{Repository: "novox/mesh-catalog", Tree: catTree, Changed: []string{"modules/album/module.json"}}
+	defines := groupChange{Repository: "novox/app", Tree: app, Changed: []string{"modules/album/module.json"}}
+	judge := func(first groupChange, rest ...groupChange) mergeVerdict {
+		v, err := judgeChange(t.Context(), mergeCheckInput{facts: f, admin: os.Getenv("MESH_TEST_POSTGRES"),
+			repository: first.Repository, tree: first.Tree, changed: first.Changed, group: rest})
+		if err != nil {
+			t.Fatal(err)
+		}
+		return v
+	}
+	for _, v := range []mergeVerdict{judge(removes, defines), judge(defines, removes)} {
+		if v.Verdict == "fail" || v.Modules["album"] != "moved" {
+			t.Fatalf("a group moving a module machines run: %v\n%s", v.Modules, v.Report())
+		}
+	}
+	if v := judge(defines); v.Verdict != "fail" {
+		t.Errorf("a definition of a module another repository builds passed alone:\n%s", v.Report())
+	}
+	if v := judge(removes); v.Verdict != "fail" {
+		t.Errorf("the removal of a module machines run passed alone:\n%s", v.Report())
+	}
+}