Skip to content

refactor(recipe): unify the two topological-sort paths (#1466) - #1728

Merged
mchmarny merged 2 commits into
mainfrom
refactor/unify-topo-sort
Jul 13, 2026
Merged

refactor(recipe): unify the two topological-sort paths (#1466)#1728
mchmarny merged 2 commits into
mainfrom
refactor/unify-topo-sort

Conversation

@lockwobr

Copy link
Copy Markdown
Contributor

Summary

Unify the two topological-sort implementations in pkg/recipe/metadata.go behind one shared graph builder, so TopologicalSort and ComponentRefsTopologicalLevels stop duplicating Kahn's algorithm and its enabled-filtering logic.

Motivation / Context

TopologicalSort (flat DeploymentOrder) and ComponentRefsTopologicalLevels (level-grouped, consumed by the helmfile deployer) each build the dependency graph, apply enabled-filtering, and detect cycles independently. That duplication forced the overrides: {enabled: false} fix in #1465 to be written twice. This extracts the shared work into buildDependencyGraph; the two functions become thin wrappers over it.

Fixes: #1466
Related: #1465

Type of Change

  • Refactoring (no functional changes)

Component(s) Affected

  • Recipe engine / data (pkg/recipe)

Implementation Notes

  • buildDependencyGraph(refs) returns (inDegree, dependents, enabledCount) and centralizes enabled-filtering, edgeSatisfiedExternally, and in-degree/dependents construction.
  • Deliberately did NOT derive one function from the other. TopologicalSort uses a globally re-sorted FIFO queue whose flat order can interleave levels alphabetically (e.g. [a, b, z]), which is not the same as flattening the levels ([a, z, b]). Deriving the flat order from the levels would change DeploymentOrder and churn every deployer's output. Both traversals are preserved exactly; only the graph build is shared.
  • No behavior change: flat order and level grouping stay byte-identical; the cross-check test TestRecipeMetadataSpecTopologicalLevelsMatchesSort passes unchanged.

Testing

go test -race ./pkg/recipe/...                          # ok
go test -race ./pkg/bundler/deployer/...                # ok (DeploymentOrder consumers unaffected)
golangci-lint run -c .golangci.yaml ./pkg/recipe/...    # 0 issues

All TopologicalSort / TopologicalLevels / MatchesSort subtests pass; every deployer golden is unchanged (ordering preserved).

Risk Assessment

  • Low — Pure refactor in a single file, covered by existing tests, trivially reversible.

Rollout notes: N/A. No functional, API, or output change.

Checklist

  • Tests pass locally (make test with -race)
  • Linter passes (make lint)
  • I did not skip/disable tests to make CI green
  • I added/updated tests for new functionality
  • I updated docs if user-facing behavior changed
  • Changes follow existing patterns in the codebase
  • Commits are cryptographically signed (git commit -S)

Extract buildDependencyGraph, the shared enabled-filtering,
external-satisfaction, and in-degree/dependents construction, so
TopologicalSort and ComponentRefsTopologicalLevels become thin wrappers
over one graph builder instead of two parallel Kahn implementations.

No behavior change: the flat sort keeps its globally-re-sorted FIFO order
and the level grouping keeps its BFS tiers, so DeploymentOrder and every
deployer's output stay byte-identical. The cross-check test
TestRecipeMetadataSpecTopologicalLevelsMatchesSort passes unchanged.

This removes the duplication that forced the enabled-filtering fix in

Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
#1465 to be written twice.
@lockwobr
lockwobr requested a review from a team as a code owner July 11, 2026 00:15
@lockwobr lockwobr added the theme/recipes Recipe expansion, overlays, mixins, and component registry label Jul 11, 2026
@lockwobr lockwobr self-assigned this Jul 11, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Recipe evidence check

No leaf overlays affected by this PR.

This gate is warning-only and never blocks merge.

@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: aa9dc114-3790-439d-9b54-89ad5ff1e7b0

📥 Commits

Reviewing files that changed from the base of the PR and between 471a57c and a2021b7.

📒 Files selected for processing (1)
  • pkg/recipe/metadata.go

📝 Walkthrough

Walkthrough

Refactored dependency-graph construction in pkg/recipe/metadata.go into the shared buildDependencyGraph helper. The helper filters enabled nodes, handles declared-but-disabled dependencies, preserves undeclared dependency edges, and returns graph metadata plus the enabled-node count. Both ComponentRefsTopologicalLevels and RecipeMetadataSpec.TopologicalSort now use it, with completion checks and result sizing based on enabledCount.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR only shares graph construction; the two Kahn traversal loops remain duplicated, so it doesn't fully unify the topological-sort paths. Refactor one traversal to derive from the other, or move the full Kahn loop into a shared implementation so both paths become thin wrappers.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the refactor in pkg/recipe.
Description check ✅ Passed The description matches the change set and explains the shared graph-builder refactor.
Out of Scope Changes check ✅ Passed All changes stay within pkg/recipe/metadata.go and support the stated refactor.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/unify-topo-sort

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Coverage Report ✅

Metric Value
Coverage 78.7%
Threshold 75%
Status Pass
Coverage Badge
![Coverage](https://img.shields.io/badge/coverage-78.7%25-green)

No Go source files changed in this PR.

@mchmarny
mchmarny merged commit 3e8e64d into main Jul 13, 2026
7 of 10 checks passed
@mchmarny
mchmarny deleted the refactor/unify-topo-sort branch July 13, 2026 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M theme/recipes Recipe expansion, overlays, mixins, and component registry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unify the two topological-sort paths in pkg/recipe

2 participants