Graduate CRDs from v1alpha1 to v1beta1#4849
Conversation
There was a problem hiding this comment.
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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4849 +/- ##
==========================================
+ Coverage 69.50% 69.56% +0.06%
==========================================
Files 551 552 +1
Lines 55933 55949 +16
==========================================
+ Hits 38874 38921 +47
+ Misses 14066 14035 -31
Partials 2993 2993 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ChrisJBurns
left a comment
There was a problem hiding this comment.
Review: Graduate CRDs from v1alpha1 to v1beta1
Verified:
- Core
GroupVersionconstant correctly changed tov1beta1 - All Go imports and aliases consistently updated (
mcpv1alpha1→mcpv1beta1) - Hardcoded
apiVersionstring inpkg/export/k8s.goupdated - Webhook manifests (both copies) correctly reference
v1beta1paths and apiVersions - Helm chart CRDs, swagger docs, and skill docs updated
- Old
v1alpha1/directory fully removed — zero stragglingv1alpha1references in production code - All 377 files show symmetric add/delete counts, confirming purely mechanical changes
Two items to address — both in the design doc, not in production code.
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. |
5fc1e31 to
ab532b0
Compare
f05ade1 to
f17269c
Compare
f17269c to
9eca7a0
Compare
Signal API stability by promoting all ToolHive CRDs to v1beta1 while preserving backwards compatibility — the CRDs serve both versions simultaneously so existing v1alpha1 resources survive the upgrade untouched and users migrate manifests at their own pace. v1alpha1 and v1beta1 are schema-identical. The new cmd/thv-operator/api/v1alpha1/ package defines only the root resource types (MCPServer, MCPGroup, ...) as thin wrappers whose Spec and Status fields reference the canonical types from v1beta1. controller-gen walks field types when building the OpenAPI schema, so both versions resolve to the same schema without any duplication of the actual model — I verified empirically that all 12 CRDs produce structurally identical schemas (modulo the legitimately-different root descriptions). The upgrade is orchestrated through the `served` and `storage` flags on each version entry: the new CRDs list both versions as served, mark v1beta1 as storage, and mark v1alpha1 as deprecated with a warning. No conversion webhook is configured, which defaults to strategy: None — safe because the schemas are identical. Kubernetes swaps the apiVersion string when serving an object at a different version than it was stored at. Users can upgrade the CRD chart without deleting anything, keep reading/writing at v1alpha1 indefinitely, and migrate to v1beta1 by re-applying manifests. Once status.storedVersions no longer contains v1alpha1, a future release can drop the v1alpha1 entry and remove the wrapper package. What changed: - Mechanical rename from v1alpha1 to v1beta1 across all Go imports, YAML apiVersion strings, Helm deploy manifests, chainsaw fixtures, examples, docs, and generated artifacts. - New cmd/thv-operator/api/v1alpha1/ package: 12 root types + 12 list types as thin wrappers over v1beta1 Spec/Status, plus scheme registration. Each root type carries a +kubebuilder:deprecatedversion warning so kubectl prints a migration hint on every access. - Register both schemes in cmd/thv-operator/main.go. - Regenerated CRD manifests list both versions (v1alpha1 deprecated and non-storage, v1beta1 storage) with conversion.strategy: None. - Removed the obsolete pre-upgrade Helm hook that blocked upgrades when v1alpha1 resources existed — no longer needed since those resources now survive the upgrade. - Drive-by: regenerate pkg/audit/zz_generated.deepcopy.go which had drifted after DetectApplicationErrors was added to Config without re-running controller-gen. Closes #2556
6e60a47 to
8280aab
Compare
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
jhrozek
left a comment
There was a problem hiding this comment.
I haven't actually tested the migration (I know you asked for testers), but I did spend a fair bit of time reviewing the PR in an interactive session with CC - mostly because I wasn't familiar with this serve-old-store-new approach. It seems clever and CC called it "a textbook implementation". I really could poke any holes it in, I like how the diff, if you exclude the mechanical replaces, is actually minimal and you can reason about the important things (I did review the annotations myself, old skool). The
I'm going to approve the patch, I guess the longer we hold off the harder it gets for you to merge. Let's chat if you want/need more testing.
Two test files that came in via the merge from main still reference the v1alpha1 Go package, which no longer exists on this branch. The operator binaries compile fine, but CI lint/test fail on the missing mcpv1alpha1 symbol in the test package. - cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go - cmd/thv-operator/controllers/mcpserver_resource_overrides_test.go Applied the same mcpv1alpha1 → mcpv1beta1 rename that the rest of the branch uses. No behavioural change.
# Conflicts: # cmd/thv-operator/pkg/controllerutil/authserver_test.go
Summary
Signal API stability by promoting all ToolHive CRDs from
v1alpha1tov1beta1— without breaking existing users. Originally scoped as a clean break (delete and recreate all resources), the PR now serves both versions simultaneously so operators can upgrade with zero downtime and migrate manifests at their own pace.The core insight is that
v1alpha1andv1beta1are schema-identical — only the version string differs. Rather than duplicate every field across both packages, the newcmd/thv-operator/api/v1alpha1/package defines only the root resource types (e.g.MCPServer,MCPGroup) as thin wrappers whoseSpecandStatusfields reference the canonical types fromv1beta1(e.g.Spec v1beta1.MCPServerSpec). Because controller-gen walks field types when building the OpenAPI schema, both versions resolve to the exact same generated schema without any duplication of the actual model. The "migration" is then orchestrated entirely through two flags on each CRD version entry:servedandstorage. The new CRD manifest lists both versions asserved: true— so existingv1alpha1clients keep working — but marks onlyv1beta1asstorage: true, meaning new writes go into etcd atv1beta1.v1alpha1additionally carriesdeprecated: truewith adeprecationWarning, which is what makeskubectlprint the migration nudge on every access. Combined withconversion.strategy: None(which is safe precisely because the schemas are identical — Kubernetes just swaps theapiVersionstring when serving an object at a different version than it was stored at), users can upgrade the CRD chart without deleting anything, keep reading and writing atv1alpha1indefinitely, and migrate tov1beta1at their own pace by simply re-applying their manifests. Once all objects have been re-stored atv1beta1(tracked via the CRD'sstatus.storedVersions), a future release can drop thev1alpha1entry from theversionslist and remove the wrapper package — completing the graduation with zero downtime at any point in the process.Closes #2556
What changed across commits
cmd/thv-operator/api/v1alpha1/→v1beta1/, all Go import paths and aliases, YAMLapiVersionstrings in examples/chainsaw fixtures/deploy manifests, hardcoded version strings inpkg/export/k8s.go, and every generated artifact (deepcopy, CRD manifests, webhook manifests, Helm chart CRDs, swagger docs, CRD API reference docs).pkg/audit.Config.DetectApplicationErrorswas added without re-runningcontroller-gen; this commit catches it up.v1alpha1wrapper types — new minimal package: 12 root types + 12 list types (~380 hand-written lines), each reusingv1beta1Spec/Status. Each root type carries+kubebuilder:deprecatedversion:warningsokubectlprints a migration hint.main.goregisters both schemes.task operator-manifests. Every CRD now listsv1alpha1(served, non-storage, deprecated) andv1beta1(served, storage). No conversion webhook (defaults tostrategy: None), which is safe because the schemas are identical.v1alpha1resources were present is no longer needed, since those resources now survive the upgrade untouched.Type of change
v1alpha1tov1beta1via multi-version servingTest plan
task test)task build— operator binary + all Go packages compile cleanly after the rebase onmain)task operator-generateandtask operator-manifestsre-run cleanly and produce the multi-version CRDstask operator-deploy-local, verified every Deployment UID was unchanged (zero-downtime), then re-applied the resources atv1beta1and confirmed still no restarts andstatus.storedVersionsadvanced to includev1beta1. 16 of 16 assertions passed.Does this introduce a user-facing change?
Yes, but non-breaking. Existing
v1alpha1resources continue to work after the upgrade; no deletion or recreation is required. Users will see a deprecation warning fromkubectlon every access to av1alpha1resource, and should migrate their manifests toapiVersion: toolhive.stacklok.dev/v1beta1at their convenience.v1alpha1will be removed in a future release once all stored objects have migrated.Special notes for reviewers
This PR is large by line count but most of the volume is either generated output (
controller-genCRD manifests,zz_generated.deepcopy.go) or the original mechanical rename. Worth reviewing carefully:cmd/thv-operator/api/v1alpha1/types.go— the only non-trivial hand-written file. Verify theSpec/Statusfield references point tov1beta1and that the+kubebuilder:deprecatedversionand other markers matchv1beta1's (minus+kubebuilder:storageversion, which must be on exactly one version).cmd/thv-operator/main.go— confirms both schemes are registered.deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpgroups.yaml) — confirm the structure: both versions listed,v1beta1markedstorage: true,v1alpha1markeddeprecated: truewith a warning andstorage: false, and theopenAPIV3Schemasections under both versions are identical.The remaining 23 CRD files follow the same pattern.
Large PR Justification
Generated with Claude Code