diff --git a/cmd/mesh-controller/planner_rules_test.go b/cmd/mesh-controller/planner_rules_test.go index f6c0c5cf..1622f336 100644 --- a/cmd/mesh-controller/planner_rules_test.go +++ b/cmd/mesh-controller/planner_rules_test.go @@ -218,14 +218,9 @@ func TestAPlanIsWhatTheChangeTouchedAndWhatIsBuiltOnIt(t *testing.T) { if u := strings.Join(r.Unread, ","); u != c.unread { t.Errorf("%s: unread %q, wanted %q", c.what, u, c.unread) } - // BUG found by this test on main (8170fc5): hasCycle reads any edge between two modules of the last - // tier as a cycle — a packages edge too, which puts both there by rule — so the merge handler logs - // "the last tier depends on itself … built together, in no order" for a plan with no cycle (the - // packages rows and the mixed diamond fail here). ADR 0162: a cycle is one last tier, *and said*; - // one said where there is none is the rule broken. The expectation stays. + // A packages edge in the last tier is no cycle; hasCycle said one on main at 8170fc5. if hasCycle(r.Plan.Tiers, c.edges) != c.cycle { - t.Errorf("%s: a cycle said %v, wanted %v (%v) [BUG: hasCycle counts a packages edge]", - c.what, !c.cycle, c.cycle, r.Plan.Tiers) + t.Errorf("%s: a cycle said %v, wanted %v (%v)", c.what, !c.cycle, c.cycle, r.Plan.Tiers) } } } @@ -310,7 +305,7 @@ var sharedRepositoryEdges = []inventory.Edge{ // packages — never along built-by or worker-of; // - every stands-on, declared, built-by and worker-of edge with both ends in the plan has the module // depended on in an earlier tier; -// - no cycle is said (this one fails on main: see the BUG note below). +// - no cycle is said. // // Seeded, so a failure is replayed by its seed and case. func TestAPlanIsTheTouchedModulesAndWhatIsReachableAlongTheWideningEdges(t *testing.T) { @@ -413,10 +408,10 @@ func TestAPlanIsTheTouchedModulesAndWhatIsReachableAlongTheWideningEdges(t *test } } } - // BUG found by this property on main (8170fc5); see the table's cycle check. Said once, after the - // other invariants have run over every case, so it hides none of them. + // Said once, after the other invariants have run over every case, so it hides none of them (hasCycle + // counted a packages edge on main at 8170fc5: 19 of these 500 cases). if falseCycles > 0 { - t.Errorf("a cycle said of a graph with none in %d of 500 cases [BUG: hasCycle counts a packages edge]; "+ + t.Errorf("a cycle said of a graph with none in %d of 500 cases; "+ "the first:\n%s", falseCycles, firstFalseCycle) } } diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index c240d478..8688705b 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -157,7 +157,9 @@ func reachableFrom(moved []string, edges []inventory.Edge) []string { return out } -// hasCycle says whether the tiers' last tier holds modules that still depend on each other. +// hasCycle says whether the tiers' last tier holds modules that still depend on each other. A packages +// edge orders nothing (tiersOf), so a module and what packages its source share a tier by rule: that is +// no cycle, and saying one was is a false report in every such plan's log. func hasCycle(tiers [][]string, edges []inventory.Edge) bool { if len(tiers) == 0 { return false @@ -167,6 +169,9 @@ func hasCycle(tiers [][]string, edges []inventory.Edge) bool { last[m] = true } for _, e := range edges { + if e.Kind == inventory.EdgePackages { + continue + } if last[e.From] && last[e.To] { return true }