Skip to content

Commit bb5e980

Browse files
ChrisJBurnsclaude
andcommitted
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>
1 parent dc2b496 commit bb5e980

1 file changed

Lines changed: 151 additions & 0 deletions

File tree

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package controllers
5+
6+
import (
7+
"bufio"
8+
"os"
9+
"path/filepath"
10+
"strings"
11+
"testing"
12+
13+
"github.com/stretchr/testify/assert"
14+
"github.com/stretchr/testify/require"
15+
)
16+
17+
// TestV1beta1TypesMarkerCoverage guards against a subtle failure mode in the
18+
// opt-in-label design for storage-version migration: if a new CRD root type is
19+
// added to cmd/thv-operator/api/v1beta1/ without the
20+
//
21+
// +kubebuilder:metadata:labels=toolhive.stacklok.dev/auto-migrate-storage-version=true
22+
//
23+
// marker, the StorageVersionMigrator controller silently excludes it from
24+
// reconciliation. The problem surfaces only when a future release tries to
25+
// drop a deprecated version — at which point it is far too late.
26+
//
27+
// The test scans every root type (marker block contains
28+
// +kubebuilder:object:root=true AND +kubebuilder:storageversion) and fails if
29+
// any lacks either the migrate marker or an explicit
30+
// +thv:storage-version-migrator:exclude sibling marker. List types
31+
// (+kubebuilder:object:root=true WITHOUT +kubebuilder:storageversion) are
32+
// intentionally skipped — CRD-level labels are keyed on the root type, not
33+
// the list type.
34+
func TestV1beta1TypesMarkerCoverage(t *testing.T) {
35+
t.Parallel()
36+
37+
typesDir := filepath.Join("..", "api", "v1beta1")
38+
entries, err := os.ReadDir(typesDir)
39+
require.NoError(t, err, "reading %s", typesDir)
40+
41+
const (
42+
rootMarker = "+kubebuilder:object:root=true"
43+
storageMarker = "+kubebuilder:storageversion"
44+
migrateMarker = "+kubebuilder:metadata:labels=toolhive.stacklok.dev/auto-migrate-storage-version=true"
45+
excludeMarker = "+thv:storage-version-migrator:exclude"
46+
groupversionInfoGo = "groupversion_info.go"
47+
zzGeneratedPrefix = "zz_generated"
48+
suffixTypesGo = "_types.go"
49+
)
50+
51+
type rootType struct {
52+
file string
53+
typeName string
54+
markerBlock []string
55+
}
56+
57+
var roots []rootType
58+
59+
for _, e := range entries {
60+
if e.IsDir() {
61+
continue
62+
}
63+
name := e.Name()
64+
if !strings.HasSuffix(name, suffixTypesGo) {
65+
continue
66+
}
67+
if name == groupversionInfoGo || strings.HasPrefix(name, zzGeneratedPrefix) {
68+
continue
69+
}
70+
71+
path := filepath.Join(typesDir, name)
72+
f, err := os.Open(path)
73+
require.NoError(t, err, "open %s", path)
74+
75+
scanner := bufio.NewScanner(f)
76+
// Raise the max token size — some of the *_types.go files have very
77+
// long comment lines (printcolumn JSONPath expressions).
78+
scanner.Buffer(make([]byte, 64*1024), 1024*1024)
79+
80+
var block []string
81+
for scanner.Scan() {
82+
line := strings.TrimSpace(scanner.Text())
83+
84+
switch {
85+
case strings.HasPrefix(line, "//"):
86+
// Accumulate every comment/marker line. kubebuilder convention
87+
// often separates markers from the godoc comment with a blank
88+
// line, so we keep the block alive across blanks and only
89+
// reset on a non-comment, non-blank, non-type line.
90+
block = append(block, strings.TrimSpace(strings.TrimPrefix(line, "//")))
91+
case strings.HasPrefix(line, "type "):
92+
// Only root-level types matter (must contain object:root=true
93+
// AND storageversion markers). List types have only object:root=true.
94+
if containsMarker(block, rootMarker) && containsMarker(block, storageMarker) {
95+
typeName := extractTypeName(line)
96+
if typeName != "" {
97+
copied := append([]string(nil), block...)
98+
roots = append(roots, rootType{file: name, typeName: typeName, markerBlock: copied})
99+
}
100+
}
101+
block = nil
102+
case line == "":
103+
// Blank line — keep block alive (comment-then-blank-then-type
104+
// is idiomatic Go + kubebuilder).
105+
default:
106+
// Anything else (e.g. struct body, package clause, import) —
107+
// drop any in-flight comment block.
108+
block = nil
109+
}
110+
}
111+
require.NoError(t, scanner.Err(), "scan %s", path)
112+
require.NoError(t, f.Close())
113+
}
114+
115+
require.NotEmpty(t, roots,
116+
"no v1beta1 root types found — scanner likely broken; this test is meaningless without coverage")
117+
118+
for _, r := range roots {
119+
hasMigrate := containsMarker(r.markerBlock, migrateMarker)
120+
hasExclude := containsMarker(r.markerBlock, excludeMarker)
121+
assert.Truef(t, hasMigrate || hasExclude,
122+
"v1beta1 root type %s.%s is missing either\n"+
123+
" %s\n"+
124+
"(opt in to storage-version migration) or\n"+
125+
" %s\n"+
126+
"(explicit opt-out). Every root type must declare one. See\n"+
127+
"cmd/thv-operator/controllers/storageversionmigrator_controller.go for context.",
128+
r.file, r.typeName, migrateMarker, excludeMarker)
129+
}
130+
}
131+
132+
// containsMarker returns true if any line in block contains the given
133+
// marker substring.
134+
func containsMarker(block []string, marker string) bool {
135+
for _, l := range block {
136+
if strings.Contains(l, marker) {
137+
return true
138+
}
139+
}
140+
return false
141+
}
142+
143+
// extractTypeName returns the identifier in a line of the form `type Foo struct {`.
144+
// Returns empty string if the line is not a type declaration we care about.
145+
func extractTypeName(line string) string {
146+
fields := strings.Fields(line)
147+
if len(fields) < 2 || fields[0] != "type" {
148+
return ""
149+
}
150+
return fields[1]
151+
}

0 commit comments

Comments
 (0)