-
Notifications
You must be signed in to change notification settings - Fork 160
Refactors ConfigMap management in the Operator to use reusable configmaps package #3021
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
There was a problem hiding this 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 transformationAlternative:
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 Report❌ Patch coverage is 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. 🚀 New features to boost your workflow:
|
Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
Large PR justification has been provided. Thank you!
Consolidated ConfigMap upsert logic into
pkg/kubernetes/configmapspackage across 5 locations.Deleted Files
pkg/runconfig/configmap/configmap.go+ testspkg/registryapi/config/configmap.go+ testsRefactored Controllers
controllers/mcpserver_runconfig.goconfigmaps.Clientdirectlycontrollers/mcpremoteproxy_runconfig.goensureRunConfigConfigMapResource(~50 lines)controllers/virtualmcpserver_vmcpconfig.goensureVmcpConfigConfigMapResource(~50 lines)pkg/registryapi/manager.goconfigmaps.Clientdirectlypkg/controllerutil/authz.goEnsureAuthzConfigMap(~25 lines)Simplified Interfaces
pkg/registryapi/config/ConfigManager: RemovedUpsertConfigMapmethod, simplifiedNewConfigManagerto single parameterChecksum Handling
The previous implementations manually compared checksum annotations to detect changes before updating ConfigMaps. This logic has been removed because
controllerutil.CreateOrUpdate(used internally byconfigmaps.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
controllerutil.CreateOrUpdatefor atomic operationsLarge PR Justification