Skip to content

Commit 0578af6

Browse files
ChrisJBurnsclaude
andauthored
Opt 12 v1beta1 CRDs into storage-version migration + CI guard (#5391)
* Label v1beta1 CRDs for auto storage version migration Adds the kubebuilder marker +kubebuilder:metadata:labels=toolhive.stacklok.dev/auto-migrate-storage-version=true to every v1beta1 root type and regenerates the CRD manifests. The label is the opt-in signal for an upcoming StorageVersionMigrator controller that will reconcile each CRD's status.storedVersions down to the current storage version. List types are intentionally not labelled — the label applies to the CRD itself, which is keyed on the root type. Part of #4969. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add CI test enforcing migrate-label coverage on v1beta1 root types Guards against the main footgun of the opt-in-label design for storage-version migration: if a new CRD root type is added to cmd/thv-operator/api/v1beta1/ without the migrate-marker, the StorageVersionMigrator controller silently excludes it. The problem surfaces only when a future release tries to drop a deprecated version — at which point it is far too late to fix. The test scans every root type (marker block contains both +kubebuilder:object:root=true and +kubebuilder:storageversion) and fails unless the block also contains either the migrate marker or an explicit +thv:storage-version-migrator:exclude sibling marker. Part of #4969. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Scan all api/v* directories, label MCPWebhookConfig The marker-coverage test was only scanning cmd/thv-operator/api/v1beta1/ and filtering for files matching *_types.go. This had two gaps: 1. Future graduations (v1beta1 → v1beta2) would silently break the test: when +kubebuilder:storageversion moves to v1beta2/, the v1beta1 root types lose the storage marker and become invisible to the scanner. The test would either pass vacuously (other version dirs unscanned) or hard-fail on the "no roots found" guard with no clear path forward. 2. MCPWebhookConfig lives in api/v1alpha1/types.go (not _types.go) and carries +kubebuilder:storageversion as the only never-graduated CRD. The original PR-B commit skipped it entirely; the migrator could not act on its CRs even when explicitly opted in. Changes: - Walk cmd/thv-operator/api/v*/ instead of hardcoding v1beta1. - File filter accepts both exact "types.go" and "*_types.go" suffix. - Rename TestV1beta1TypesMarkerCoverage → TestStorageVersionRootMarkerCoverage. - Update failure message + docstring to reflect the broader scope. - Add the migrate marker to MCPWebhookConfig in v1alpha1/types.go and regenerate its two CRD YAML files under deploy/charts/operator-crds/. Future-proof: when v1beta2 lands and +kubebuilder:storageversion moves to its root types, the scanner picks them up without code changes here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Drop the exclude marker — every storage root must opt in The previous test accepted either the migrate marker or an explicit +thv:storage-version-migrator:exclude sibling. That permissive contract opens a graduation-time hole: a single-version CRD created with exclude passes CI, but at v1→v2 graduation time the developer has to remember to swap exclude→migrate. Forgetting the swap leaves the CRD silently un-migrated — exactly the failure mode this test is meant to prevent. No CRD in the codebase uses the exclude marker today. Removing it costs nothing and closes the hole. If a legitimate exclusion ever comes up, handle it as a one-off PR conversation and a hardcoded allowlist in this test — not a self-serve marker that future contributors can misuse. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Tighten marker match to whole-token boundary + stub allowlist Two defensive additions identified during stress-testing the opt-in-label design: 1. Strict marker matching. containsMarker previously used strings.Contains, which would accept "=truee" (or any extra character) as a substring match for "=true". The CI test would pass, the generated CRD YAML would carry label value "truee", and the controller's runtime check (labels[key] == "true") would silently exclude the CRD. The bug only surfaces at version-drop time months later. containsMarker now requires the marker to be followed by end-of-line or whitespace. TestContainsMarkerStrictBoundary defends the contract against future regressions to substring semantics. 2. excludedRootTypes allowlist stub. The previous commit removed the self-serve +thv:storage-version-migrator:exclude marker. This adds the documented alternative — a hardcoded allowlist in the test file — as an empty map with an example entry and a comment explaining the deliberate, reviewed-decision policy for adding entries. If a future contributor needs to exclude a CRD, the path is "add an entry, justify it in PR review"; they don't have to invent the mechanism from scratch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address review nits: marker order + drop pre-1.22 loop shadow - v1alpha1/types.go MCPWebhookConfig: move the migrate marker to AFTER +kubebuilder:subresource:status, matching the v1beta1 convention. controller-gen ignores order so behaviour is unchanged; this is purely consistency for greppability and future readers. - marker_coverage_test.go: drop `tc := tc` shadowing in TestContainsMarkerStrictBoundary. go.mod is on go 1.26; loop variables have been per-iteration since 1.22, so the shadow is dead code from older-Go muscle memory. go/ast or controller-tools/pkg/markers refactor (raised separately) deliberately deferred: the bufio scanner is correct for every CRD in the codebase today; switching to AST is a ~40-line restructure better landed as a follow-up if/when we actually need generics or grouped type decl support in a CRD. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 83aaf96 commit 0578af6

40 files changed

Lines changed: 325 additions & 0 deletions

File tree

cmd/thv-operator/api/v1alpha1/types.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,7 @@ type MCPTelemetryConfigList struct {
281281
//+kubebuilder:object:root=true
282282
//+kubebuilder:storageversion
283283
//+kubebuilder:subresource:status
284+
//+kubebuilder:metadata:labels=toolhive.stacklok.dev/auto-migrate-storage-version=true
284285
//+kubebuilder:resource:shortName=mwc,categories=toolhive
285286
//+kubebuilder:printcolumn:name="Valid",type=string,JSONPath=`.status.conditions[?(@.type=='Valid')].status`
286287
//+kubebuilder:printcolumn:name="References",type=string,JSONPath=`.status.referencingWorkloads`

cmd/thv-operator/api/v1beta1/embeddingserver_types.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,7 @@ const (
211211
//+kubebuilder:object:root=true
212212
//+kubebuilder:storageversion
213213
//+kubebuilder:subresource:status
214+
//+kubebuilder:metadata:labels=toolhive.stacklok.dev/auto-migrate-storage-version=true
214215
//+kubebuilder:resource:shortName=emb;embedding,categories=toolhive
215216
//+kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.phase"
216217
//+kubebuilder:printcolumn:name="Model",type="string",JSONPath=".spec.model"

cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1117,6 +1117,7 @@ type MCPExternalAuthConfigStatus struct {
11171117
// +kubebuilder:object:root=true
11181118
// +kubebuilder:storageversion
11191119
// +kubebuilder:subresource:status
1120+
// +kubebuilder:metadata:labels=toolhive.stacklok.dev/auto-migrate-storage-version=true
11201121
// +kubebuilder:resource:shortName=extauth;mcpextauth,categories=toolhive
11211122
// +kubebuilder:printcolumn:name="Type",type=string,JSONPath=`.spec.type`
11221123
// +kubebuilder:printcolumn:name="Valid",type=string,JSONPath=`.status.conditions[?(@.type=='Valid')].status`

cmd/thv-operator/api/v1beta1/mcpgroup_types.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ const (
8888
//+kubebuilder:object:root=true
8989
//+kubebuilder:storageversion
9090
//+kubebuilder:subresource:status
91+
//+kubebuilder:metadata:labels=toolhive.stacklok.dev/auto-migrate-storage-version=true
9192
//+kubebuilder:resource:shortName=mcpg;mcpgroup,categories=toolhive
9293
//+kubebuilder:printcolumn:name="Servers",type="integer",JSONPath=".status.serverCount"
9394
//+kubebuilder:printcolumn:name="Phase",type="string",JSONPath=".status.phase"

cmd/thv-operator/api/v1beta1/mcpoidcconfig_types.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,7 @@ type MCPOIDCConfigStatus struct {
200200
// +kubebuilder:object:root=true
201201
// +kubebuilder:storageversion
202202
// +kubebuilder:subresource:status
203+
// +kubebuilder:metadata:labels=toolhive.stacklok.dev/auto-migrate-storage-version=true
203204
// +kubebuilder:resource:shortName=mcpoidc,categories=toolhive
204205
// +kubebuilder:printcolumn:name="Source",type=string,JSONPath=`.spec.type`
205206
// +kubebuilder:printcolumn:name="Valid",type=string,JSONPath=`.status.conditions[?(@.type=='Valid')].status`

cmd/thv-operator/api/v1beta1/mcpregistry_types.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,7 @@ const (
205205
//+kubebuilder:object:root=true
206206
//+kubebuilder:storageversion
207207
//+kubebuilder:subresource:status
208+
//+kubebuilder:metadata:labels=toolhive.stacklok.dev/auto-migrate-storage-version=true
208209
//+kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.phase"
209210
//+kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.conditions[?(@.type=='Ready')].status"
210211
//+kubebuilder:printcolumn:name="Replicas",type="integer",JSONPath=".status.readyReplicas"

cmd/thv-operator/api/v1beta1/mcpremoteproxy_types.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,6 +356,7 @@ const (
356356
//+kubebuilder:object:root=true
357357
//+kubebuilder:storageversion
358358
//+kubebuilder:subresource:status
359+
//+kubebuilder:metadata:labels=toolhive.stacklok.dev/auto-migrate-storage-version=true
359360
//+kubebuilder:resource:shortName=rp;mcprp,categories=toolhive
360361
//+kubebuilder:printcolumn:name="Phase",type="string",JSONPath=".status.phase"
361362
//+kubebuilder:printcolumn:name="Remote URL",type="string",JSONPath=".spec.remoteUrl"

cmd/thv-operator/api/v1beta1/mcpserver_types.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -929,6 +929,7 @@ const (
929929
//+kubebuilder:object:root=true
930930
//+kubebuilder:storageversion
931931
//+kubebuilder:subresource:status
932+
//+kubebuilder:metadata:labels=toolhive.stacklok.dev/auto-migrate-storage-version=true
932933
//+kubebuilder:resource:shortName=mcpserver;mcpservers,categories=toolhive
933934
//+kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.phase"
934935
//+kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.conditions[?(@.type=='Ready')].status"

cmd/thv-operator/api/v1beta1/mcpserverentry_types.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,7 @@ const (
164164
//+kubebuilder:object:root=true
165165
//+kubebuilder:storageversion
166166
//+kubebuilder:subresource:status
167+
//+kubebuilder:metadata:labels=toolhive.stacklok.dev/auto-migrate-storage-version=true
167168
//+kubebuilder:resource:shortName=mcpentry,categories=toolhive
168169
//+kubebuilder:printcolumn:name="Phase",type="string",JSONPath=".status.phase"
169170
//+kubebuilder:printcolumn:name="Transport",type="string",JSONPath=".spec.transport"

cmd/thv-operator/api/v1beta1/mcptelemetryconfig_types.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ type MCPTelemetryConfigStatus struct {
139139
// +kubebuilder:object:root=true
140140
// +kubebuilder:storageversion
141141
// +kubebuilder:subresource:status
142+
// +kubebuilder:metadata:labels=toolhive.stacklok.dev/auto-migrate-storage-version=true
142143
// +kubebuilder:resource:shortName=mcpotel,categories=toolhive
143144
// +kubebuilder:printcolumn:name="Endpoint",type=string,JSONPath=`.spec.openTelemetry.endpoint`
144145
// +kubebuilder:printcolumn:name="Valid",type=string,JSONPath=`.status.conditions[?(@.type=='Valid')].status`

0 commit comments

Comments
 (0)