Skip to content

Conversation

@ChrisJBurns
Copy link
Collaborator

@ChrisJBurns ChrisJBurns commented Dec 12, 2025

Consolidated ConfigMap upsert logic into pkg/kubernetes/configmaps package across 5 locations.

Deleted Files

  • pkg/runconfig/configmap/configmap.go + tests
  • pkg/registryapi/config/configmap.go + tests

Refactored Controllers

File Change
controllers/mcpserver_runconfig.go Use configmaps.Client directly
controllers/mcpremoteproxy_runconfig.go Removed ensureRunConfigConfigMapResource (~50 lines)
controllers/virtualmcpserver_vmcpconfig.go Removed ensureVmcpConfigConfigMapResource (~50 lines)
pkg/registryapi/manager.go Use configmaps.Client directly
pkg/controllerutil/authz.go Removed manual Get/Create/Update in EnsureAuthzConfigMap (~25 lines)

Simplified Interfaces

  • pkg/registryapi/config/ConfigManager: Removed UpsertConfigMap method, simplified NewConfigManager to single parameter

Checksum Handling

The previous implementations manually compared checksum annotations to detect changes before updating ConfigMaps. This logic has been removed because controllerutil.CreateOrUpdate (used internally by configmaps.Client) already compares the existing and desired resources to detect differences. It only performs an update when the content has actually changed, making the manual checksum comparison redundant. The checksum annotations are still added to ConfigMaps for use by pod template annotations to trigger rollouts when config changes.

Benefits

  • ~150 lines of duplicate Get/Create/Update logic removed
  • Centralized ConfigMap operations with controllerutil.CreateOrUpdate for atomic operations
  • Consistent owner reference handling across all controllers

Large PR Justification

  • It's mostly removing files and code in favour of reusable functions

Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
@github-actions github-actions bot added the size/XL Extra large PR: 1000+ lines changed label Dec 12, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.


This review will be automatically dismissed once you add the justification section.

Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Dec 12, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.44%. Comparing base (6053cdc) to head (59ea053).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cmd/thv-operator/pkg/controllerutil/authz.go 0.00% 3 Missing ⚠️
...v-operator/controllers/mcpremoteproxy_runconfig.go 66.66% 1 Missing and 1 partial ⚠️
...perator/controllers/virtualmcpserver_vmcpconfig.go 33.33% 1 Missing and 1 partial ⚠️
...md/thv-operator/controllers/mcpserver_runconfig.go 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3021      +/-   ##
==========================================
- Coverage   56.49%   56.44%   -0.06%     
==========================================
  Files         335      333       -2     
  Lines       33171    33001     -170     
==========================================
- Hits        18741    18626     -115     
+ Misses      12836    12793      -43     
+ Partials     1594     1582      -12     

☔ 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.

Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Dec 12, 2025
Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
@github-actions
Copy link
Contributor

✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review.

@github-actions github-actions bot dismissed their stale review December 12, 2025 19:34

Large PR justification has been provided. Thank you!

@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Dec 12, 2025
@ChrisJBurns ChrisJBurns merged commit cc991e2 into main Dec 12, 2025
53 of 56 checks passed
@ChrisJBurns ChrisJBurns deleted the removes-duplicated-configmap branch December 12, 2025 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants