Skip to content

Extract shared WorkloadReference helpers into controllerutil#4520

Merged
ChrisJBurns merged 4 commits intomainfrom
cburns/refactor-referencing-workloads-helpers
Apr 3, 2026
Merged

Extract shared WorkloadReference helpers into controllerutil#4520
ChrisJBurns merged 4 commits intomainfrom
cburns/refactor-referencing-workloads-helpers

Conversation

@ChrisJBurns
Copy link
Copy Markdown
Collaborator

Summary

  • PR review feedback on Replace ReferencingServers with ReferencingWorkloads on MCPToolConfig #4506 and Replace ReferencingServers with ReferencingWorkloads on MCPExternalAuthConfig #4507 identified that the ReferencingWorkloads pattern had inconsistent sorting: MCPTelemetryConfig and MCPExternalAuthConfig sorted refs, but MCPToolConfig and MCPOIDCConfig did not. This caused unnecessary API server writes when the same workloads were discovered in different list order across reconcile runs.
  • The same list-filter-build logic was also duplicated across all four config controllers, making it easy for inconsistencies to creep in.
  • Extracts SortWorkloadRefs, CompareWorkloadRefs, WorkloadRefsEqual, and FindWorkloadRefsFromMCPServers into the shared controllerutil package. All four controllers now delegate to these helpers, ensuring consistent deterministic ordering and removing ~50 lines of duplicated code.

Type of change

  • Refactoring (no behavior change)

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)

Changes

File Change
cmd/thv-operator/pkg/controllerutil/config.go Add SortWorkloadRefs, CompareWorkloadRefs, WorkloadRefsEqual, FindWorkloadRefsFromMCPServers shared helpers
cmd/thv-operator/pkg/controllerutil/config_test.go Add tests for all new helpers
cmd/thv-operator/controllers/toolconfig_controller.go Delegate findReferencingWorkloads to shared helper; add sorting in handleConfigHashChange
cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go Replace inline sort and findReferencingWorkloads with shared helpers
cmd/thv-operator/controllers/mcpoidcconfig_controller.go Delegate MCPServer lookup to shared helper; add sorting after VirtualMCPServer append
cmd/thv-operator/controllers/mcptelemetryconfig_controller.go Replace inline sort, local workloadRefsEqual, and findReferencingWorkloads with shared helpers

Special notes for reviewers

  • This addresses the sorting inconsistency flagged in #4506 review and the duplication concern in #4507 review.
  • The stale-ref watch handler and handleDeletion patterns are also duplicated but require more involved generalization (interface/generic approaches for different config list types). Those are better addressed as part of the shared config resource lifecycle work in toolhive-rfcs#65.
  • The pre-existing goconst lint issue for "MCPServer" string in mcpexternalauthconfig_controller.go is not addressed here — it predates this change.

Generated with Claude Code

The ReferencingWorkloads pattern was duplicated across four config
controllers with inconsistent sorting behavior: MCPTelemetryConfig and
MCPExternalAuthConfig sorted refs, but MCPToolConfig and MCPOIDCConfig
did not. This caused unnecessary API server writes when the same set of
workloads was discovered in a different list order across reconcile runs.

Extract SortWorkloadRefs, WorkloadRefsEqual, and
FindWorkloadRefsFromMCPServers into the shared controllerutil package
so all controllers use deterministic, consistent ordering. Each
controller's findReferencingWorkloads now delegates to the shared
helper, removing ~50 lines of duplicated list-filter-build logic.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions bot added the size/M Medium PR: 300-599 lines changed label Apr 3, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 3, 2026

Codecov Report

❌ Patch coverage is 77.58621% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.10%. Comparing base (a948281) to head (e2a43fb).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...v-operator/controllers/mcpoidcconfig_controller.go 50.00% 4 Missing and 2 partials ⚠️
.../thv-operator/controllers/toolconfig_controller.go 77.77% 1 Missing and 1 partial ⚠️
cmd/thv-operator/pkg/controllerutil/config.go 89.47% 1 Missing and 1 partial ⚠️
...or/controllers/mcpexternalauthconfig_controller.go 88.88% 1 Missing ⚠️
...rator/controllers/mcptelemetryconfig_controller.go 85.71% 1 Missing ⚠️
...perator/controllers/virtualmcpserver_controller.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4520      +/-   ##
==========================================
+ Coverage   69.02%   69.10%   +0.07%     
==========================================
  Files         502      502              
  Lines       52008    51997      -11     
==========================================
+ Hits        35899    35933      +34     
+ Misses      13320    13276      -44     
+ Partials     2789     2788       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

JAORMX
JAORMX previously approved these changes Apr 3, 2026
Define WorkloadKindMCPServer, WorkloadKindVirtualMCPServer, and
WorkloadKindMCPRemoteProxy constants in the API types and use them
across all controllers. This fixes a goconst lint violation where
"MCPServer" appeared as a raw string literal 4+ times.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Apr 3, 2026
@github-actions github-actions bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Apr 3, 2026
@ChrisJBurns ChrisJBurns merged commit 2e8e487 into main Apr 3, 2026
41 of 42 checks passed
@ChrisJBurns ChrisJBurns deleted the cburns/refactor-referencing-workloads-helpers branch April 3, 2026 15:11
ChrisJBurns added a commit that referenced this pull request Apr 3, 2026
Replace hardcoded "MCPRemoteProxy" strings with the shared constant
from PR #4520 for consistency with all other controllers.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MatteoManzoni pushed a commit to DocPlanner/toolhive that referenced this pull request Apr 4, 2026
…k#4520)

* Extract shared WorkloadReference helpers into controllerutil

The ReferencingWorkloads pattern was duplicated across four config
controllers with inconsistent sorting behavior: MCPTelemetryConfig and
MCPExternalAuthConfig sorted refs, but MCPToolConfig and MCPOIDCConfig
did not. This caused unnecessary API server writes when the same set of
workloads was discovered in a different list order across reconcile runs.

Extract SortWorkloadRefs, WorkloadRefsEqual, and
FindWorkloadRefsFromMCPServers into the shared controllerutil package
so all controllers use deterministic, consistent ordering. Each
controller's findReferencingWorkloads now delegates to the shared
helper, removing ~50 lines of duplicated list-filter-build logic.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Add WorkloadKind constants to replace string literals

Define WorkloadKindMCPServer, WorkloadKindVirtualMCPServer, and
WorkloadKindMCPRemoteProxy constants in the API types and use them
across all controllers. This fixes a goconst lint violation where
"MCPServer" appeared as a raw string literal 4+ times.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Medium PR: 300-599 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants