-
Notifications
You must be signed in to change notification settings - Fork 156
enable CRD upgrades via Helm with feature flags #2809
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
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2809 +/- ##
==========================================
- Coverage 56.11% 56.10% -0.01%
==========================================
Files 328 328
Lines 32399 32399
==========================================
- Hits 18181 18179 -2
- Misses 12670 12678 +8
+ Partials 1548 1542 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpremoteproxies.yaml
Outdated
Show resolved
Hide resolved
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml
Show resolved
Hide resolved
deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml
Outdated
Show resolved
Hide resolved
deploy/charts/operator-crds/crd-helm-wrapper/templates/header-feature.tpl
Outdated
Show resolved
Hide resolved
jhrozek
left a comment
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.
Nice work on this! The approach of moving CRDs to templates/ for Helm-managed upgrades is solid and follows what other major projects do (cert-manager, Prometheus Operator, etc.).
I spotted a few things while reviewing - mostly minor, but one could cause silent data issues.
Co-authored-by: Jakub Hrozek <jakub.hrozek@posteo.se>
- we want to safeguard against larger files - we want error if there is a CRD that isn't in the feature flag map - adds some docs to the main readme about why we put crds in template Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Signed-off-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
All comments have been addressed and have an approval from Jakub.
Summary
This PR addresses the CRD lifecycle management issues described in stacklok/stacklok-epics#175 by moving CRDs from
crds/intotemplates/to take advantage of standard Helm upgrade capability.Problem: Helm does not upgrade CRDs during
helm upgradeoperations (#2518). CRDs placed in thecrds/directory are only processed during initial installation - subsequent upgrades silently leave CRDs unchanged, even when the chart contains updates. This led to users having stale CRDs without any indication (#2311).Solution: Following common patterns, CRDs are now placed in
templates/instead ofcrds/. This ensures CRDs are upgraded alongside the chart duringhelm upgrade.The CRDs are wrapped with Helm template conditionals:
crds.install.server- Server CRDs (mcpservers, mcpremoteproxies, mcptoolconfigs,mcpgroups)
crds.install.registry- Registry CRDs (mcpregistries)crds.install.virtualMcp- VirtualMCP CRDs (virtualmcpservers,virtualmcpcompositetooldefinitions)
crds.keep- Addshelm.sh/resource-policy: keepannotation to prevent CRD deletionon uninstall
Shared CRDs like
mcpexternalauthconfigsuseorconditionals to install when eitherserverorvirtualMcpis enabled.Implementation:
deploy/charts/operator-crds/crd-helm-wrapper/that wraps rawCRD YAML files with Helm conditionals
.tpltemplate files for maintainabilityRelated issues: #2518, #1779, #2311
Large PR Justification
The changes in this PR are all auto-generated CRD definitions.