Skip to content

Conversation

@ChrisJBurns
Copy link
Collaborator

@ChrisJBurns ChrisJBurns commented Nov 29, 2025

Summary

This PR addresses the CRD lifecycle management issues described in stacklok/stacklok-epics#175 by moving CRDs from crds/ into templates/ to take advantage of standard Helm upgrade capability.

Problem: Helm does not upgrade CRDs during helm upgrade operations (#2518). CRDs placed in the crds/ 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 of crds/. This ensures CRDs are upgraded alongside the chart during helm 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 - Adds helm.sh/resource-policy: keep annotation to prevent CRD deletion
    on uninstall

Shared CRDs like mcpexternalauthconfigs use or conditionals to install when either
server or virtualMcp is enabled.

Implementation:

  • Created a Go tool in deploy/charts/operator-crds/crd-helm-wrapper/ that wraps raw
    CRD YAML files with Helm conditionals
  • Uses embedded .tpl template files for maintainability
  • Automatically escapes Go template syntax in CRD descriptions

Related issues: #2518, #1779, #2311

Large PR Justification

The changes in this PR are all auto-generated CRD definitions.

@github-actions github-actions bot added the size/XL Extra large PR: 1000+ lines changed label Nov 29, 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.

@codecov
Copy link

codecov bot commented Nov 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.10%. Comparing base (c914d42) to head (4caf5c9).
⚠️ Report is 1 commits behind head on main.

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

@github-actions github-actions bot dismissed their stale review December 2, 2025 16:55

Large PR justification has been provided. Thank you!

@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2025

✅ 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 added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Dec 2, 2025
@github-actions github-actions bot removed the size/XL Extra large PR: 1000+ lines changed label Dec 3, 2025
@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 8, 2025
@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 8, 2025
Copy link
Contributor

@jhrozek jhrozek left a 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.

ChrisJBurns and others added 3 commits December 10, 2025 16:27
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>
@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 10, 2025
@ChrisJBurns
Copy link
Collaborator Author

@jhrozek Have added some error handling for the cases you identified. Also added some docs for the readme about why we're using templates. @amirejaz Would you be able to review too as I believe you've got a blocking requested changes signal on the PR

@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 10, 2025
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 10, 2025
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 10, 2025
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 10, 2025
@ChrisJBurns ChrisJBurns dismissed amirejaz’s stale review December 10, 2025 21:51

All comments have been addressed and have an approval from Jakub.

@ChrisJBurns ChrisJBurns merged commit a291ed7 into main Dec 10, 2025
38 of 39 checks passed
@ChrisJBurns ChrisJBurns deleted the wrap-crds branch December 10, 2025 22:06
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.

4 participants