CNTRLPLANE-502: Add CRD breaking changes validation to HyperShift CI - #8535
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-502 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request adds a new crd-schema-check CLI (hack/tools/crd-schema-check) that scans specified directories for CRD YAML, filters versions with OpenAPI v3 schemas, loads the corresponding CRDs from a base git commit, and runs schema comparisons via the comparator registry. The tool prints warnings, aggregates errors, and exits non‑zero on breaking changes. The Makefile is updated to build the tool and add a verify-crd-schema target included in verify-parallel. Tests and YAML helper builders were added, and hack/tools/go.mod promotes required dependencies. Sequence Diagram(s)sequenceDiagram
participant User as CLI (crd-schema-check)
participant Repo as git.Repository
participant FS as Filesystem
participant Comparator as comparator.Registry
User->>Repo: findRepoRoot / resolve base commit
User->>FS: scan --crd-dir for .yaml files
FS->>User: CRD YAML bytes
User->>User: isCRDYAML / filterVersionsWithSchema
User->>Repo: loadCRDFromCommit(path, baseCommit)
User->>Comparator: CompareCRDs(oldCRD, newCRD, config)
Comparator->>User: ComparisonResults, Errors/Warnings
User->>User: aggregate and exit with status
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
area/ci-tooling
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
hack/tools/crd-schema-check/main_test.go (1)
67-68: ⚡ Quick winStrengthen
filterVersionsWithSchemaassertions to validate retained version names, not only count.Current checks can pass even if the wrong version is retained. Add expected version names per case and assert exact match (order or set).
Suggested test hardening
tests := []struct { name string yaml string expectedVersions int + expectedNames []string }{ { name: "When all versions have schemas it should keep all versions", yaml: baseCRDWithVersions([]versionSpec{ {name: "v1", hasSchema: true}, {name: "v1beta1", hasSchema: true}, }), expectedVersions: 2, + expectedNames: []string{"v1", "v1beta1"}, }, { name: "When some versions lack schemas it should filter them out", yaml: baseCRDWithVersions([]versionSpec{ {name: "v1", hasSchema: true}, {name: "v1beta1", hasSchema: false}, }), expectedVersions: 1, + expectedNames: []string{"v1"}, }, { name: "When no versions have schemas it should return zero versions", yaml: baseCRDWithVersions([]versionSpec{ {name: "v1", hasSchema: false}, }), expectedVersions: 0, + expectedNames: nil, }, } @@ result := filterVersionsWithSchema(crd) if len(result.Spec.Versions) != tt.expectedVersions { t.Errorf("filterVersionsWithSchema() returned %d versions, want %d", len(result.Spec.Versions), tt.expectedVersions) } + got := make([]string, 0, len(result.Spec.Versions)) + for _, v := range result.Spec.Versions { + got = append(got, v.Name) + } + if len(got) != len(tt.expectedNames) { + t.Fatalf("filterVersionsWithSchema() names count = %d, want %d", len(got), len(tt.expectedNames)) + } + for i := range got { + if got[i] != tt.expectedNames[i] { + t.Fatalf("filterVersionsWithSchema() names = %v, want %v", got, tt.expectedNames) + } + }Also applies to: 100-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/tools/crd-schema-check/main_test.go` around lines 67 - 68, The tests for filterVersionsWithSchema only assert the count; change the test cases in main_test.go to include expected version names (e.g., replace expectedVersions int with expectedVersions []string) and update the assertion after calling filterVersionsWithSchema to compare the actual retained version names against the expected slice (either by exact order or by set equality). Locate the test table entries and the assertion block that currently checks len(filtered) and replace it with logic that extracts the version names from filtered and asserts equality with the expectedVersions for each test case (also update the additional cases around the other occurrence referenced in the comment).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/tools/crd-schema-check/main.go`:
- Around line 167-170: The call-site currently treats any error from
loadCRDFromCommit as "new CRD" and continues with oldCRD == nil; instead, change
loadCRDFromCommit so it returns (nil, nil) when the file truly does not exist by
checking errors.Is(err, object.ErrFileNotFound) inside loadCRDFromCommit, and
for any other error propagate it upward; then update the caller (where oldCRD,
err := loadCRDFromCommit(baseCommit, relPath) is invoked) to return or fail on
non-nil err rather than logging "new", and only log "new CRD" when
loadCRDFromCommit returned (nil, nil) meaning file-not-found.
- Line 104: The printed success message in main.main (the fmt.Printf call that
uses totalChecked and allWarnings) writes non-JSON output to stdout; change it
to write to stderr instead so stdout remains JSON-only—replace the
fmt.Printf(...) with fmt.Fprintf(os.Stderr, ...) or fmt.Fprintln(os.Stderr, ...)
(importing os if needed) or use the logger that writes to stderr; ensure no
other non-JSON prints remain in main().
---
Nitpick comments:
In `@hack/tools/crd-schema-check/main_test.go`:
- Around line 67-68: The tests for filterVersionsWithSchema only assert the
count; change the test cases in main_test.go to include expected version names
(e.g., replace expectedVersions int with expectedVersions []string) and update
the assertion after calling filterVersionsWithSchema to compare the actual
retained version names against the expected slice (either by exact order or by
set equality). Locate the test table entries and the assertion block that
currently checks len(filtered) and replace it with logic that extracts the
version names from filtered and asserts equality with the expectedVersions for
each test case (also update the additional cases around the other occurrence
referenced in the comment).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f14c7b00-9679-4518-af7d-51f8e7eaa728
📒 Files selected for processing (3)
Makefilehack/tools/crd-schema-check/main.gohack/tools/crd-schema-check/main_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8535 +/- ##
==========================================
+ Coverage 40.34% 41.86% +1.51%
==========================================
Files 755 759 +4
Lines 93167 94101 +934
==========================================
+ Hits 37587 39392 +1805
+ Misses 52877 51949 -928
- Partials 2703 2760 +57 see 95 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1898e5a to
70586fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hack/tools/crd-schema-check/main_test.go (1)
19-297: ⚡ Quick winConsider using gomega assertions and enabling parallel execution for all test functions.
The test functions use standard Go testing assertions (
t.Errorf,t.Fatalf) and do not callt.Parallel(). These tests are independent with no shared state and would benefit from:
- gomega matchers for more expressive assertions and clearer failure messages
t.Parallel()calls to improve test execution performance in CIExample refactor for one test function:
Suggested pattern with gomega and parallel execution
+import ( + . "github.com/onsi/gomega" +) func TestIsCRDYAML(t *testing.T) { + RegisterTestingT(t) + tests := []struct { name string yaml string expected bool }{ // ... test cases ... } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) result := isCRDYAML([]byte(tt.yaml)) - if result != tt.expected { - t.Errorf("isCRDYAML() = %v, want %v", result, tt.expected) - } + g.Expect(result).To(Equal(tt.expected)) }) } }Apply similar changes to all test functions in this file. As per coding guidelines, prefer gomega for unit test assertions and use race detection and parallel execution for unit tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/tools/crd-schema-check/main_test.go` around lines 19 - 297, All tests in this file should use Gomega matchers and run in parallel: add t.Parallel() at the start of each top-level test (TestIsCRDYAML, TestFilterVersionsWithSchema, TestCompareCRDs_NonBreakingChanges, TestCompareCRDs_BreakingChanges, TestCompareCRDs_NewCRDWithNilOld, TestBuildComparatorConfig) and inside each t.Run subtest where appropriate, replace t.Fatalf/t.Errorf assertions with Gomega expectations (use NewGomegaWithT(t) or GomegaWithT(t) to create a matcher bound to the test and assert equality, empty error lists, presence/absence of comparator names, lengths, etc.), and add the gomega import; keep existing test logic and helper functions (isCRDYAML, filterVersionsWithSchema, CompareCRDs, buildComparatorConfig, resourceread usage) but convert all assertion checks to gomega.Expect(...) calls for clearer failures and concurrency safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/tools/crd-schema-check/main.go`:
- Around line 171-176: The code logs that a CRD is new and should skip
comparison but then still calls config.ComparatorRegistry.Compare(oldCRD,
newCRD, ...) and increments results.checked; fix by adding an immediate continue
after the fmt.Fprintf(...) when oldCRD == nil so Compare is not invoked and
results.checked is not incremented for new CRDs, and if you want to track those
files add a separate counter (e.g., results.new) instead of counting them in
results.checked; reference oldCRD, newCRD, relPath,
config.ComparatorRegistry.Compare, compResults/compErrors and results.checked
when applying the change.
---
Nitpick comments:
In `@hack/tools/crd-schema-check/main_test.go`:
- Around line 19-297: All tests in this file should use Gomega matchers and run
in parallel: add t.Parallel() at the start of each top-level test
(TestIsCRDYAML, TestFilterVersionsWithSchema,
TestCompareCRDs_NonBreakingChanges, TestCompareCRDs_BreakingChanges,
TestCompareCRDs_NewCRDWithNilOld, TestBuildComparatorConfig) and inside each
t.Run subtest where appropriate, replace t.Fatalf/t.Errorf assertions with
Gomega expectations (use NewGomegaWithT(t) or GomegaWithT(t) to create a matcher
bound to the test and assert equality, empty error lists, presence/absence of
comparator names, lengths, etc.), and add the gomega import; keep existing test
logic and helper functions (isCRDYAML, filterVersionsWithSchema, CompareCRDs,
buildComparatorConfig, resourceread usage) but convert all assertion checks to
gomega.Expect(...) calls for clearer failures and concurrency safety.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4d17fc14-c76c-429c-befe-e740b884a2d9
📒 Files selected for processing (4)
Makefilehack/tools/crd-schema-check/main.gohack/tools/crd-schema-check/main_test.gohack/tools/go.mod
70586fe to
0b4378f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hack/tools/go.mod (1)
18-18: ⚡ Quick winAlign declared and replaced Kubernetes API extension versions.
Line 18 declares
k8s.io/apiextensions-apiserver v0.34.3, but Line 335 forcesv0.34.2viareplace. Align these to the same version to avoid version-skew confusion in tooling and maintenance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/tools/go.mod` at line 18, The go.mod currently declares k8s.io/apiextensions-apiserver at v0.34.3 while a replace directive forces v0.34.2, causing a version mismatch; update one so both match—either change the declared module version k8s.io/apiextensions-apiserver to v0.34.2 or (preferably) update the replace directive to v0.34.3—ensure the declared module version and the replace entry for k8s.io/apiextensions-apiserver use the identical semver string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/tools/crd-schema-check/main.go`:
- Around line 171-173: The current logic treats oldCRD == nil the same whether
the base file is missing or was filtered to no versions with OpenAPIV3Schema by
loadCRDFromCommit; update loadCRDFromCommit to return a distinct error/value
(e.g., ErrNoSchema or a typed sentinel) when the file exists but all versions
lack OpenAPIV3Schema, and then change the caller handling the oldCRD check in
main.go to distinguish the two cases: if the error is ErrNoSchema, print a
specific message like "info: %s present in base but no versions with
OpenAPIV3Schema, skipping schema comparison", while only using "not found in
base" for a true missing-file case; ensure references to oldCRD and
loadCRDFromCommit are updated accordingly so both the early return and the log
message reflect the distinct states.
---
Nitpick comments:
In `@hack/tools/go.mod`:
- Line 18: The go.mod currently declares k8s.io/apiextensions-apiserver at
v0.34.3 while a replace directive forces v0.34.2, causing a version mismatch;
update one so both match—either change the declared module version
k8s.io/apiextensions-apiserver to v0.34.2 or (preferably) update the replace
directive to v0.34.3—ensure the declared module version and the replace entry
for k8s.io/apiextensions-apiserver use the identical semver string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 42394e2d-6833-4293-997a-b4d7e469f7f0
📒 Files selected for processing (2)
hack/tools/crd-schema-check/main.gohack/tools/go.mod
0b4378f to
c423f10
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
hack/tools/crd-schema-check/main.go (2)
138-140:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
.ymlfiles in the CRD scan.This filter silently skips CRDs committed as
.yml, which leaves a gap in the breaking-change check.Proposed fix
- if filepath.Ext(entry.Name()) != ".yaml" { + ext := strings.ToLower(filepath.Ext(entry.Name())) + if ext != ".yaml" && ext != ".yml" { continue }As per coding guidelines, "
**/*.{go,yaml,yml}: Provide API definitions that align with OpenShift and Kubernetes best practices".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/tools/crd-schema-check/main.go` around lines 138 - 140, The current file-extension filter in crd-schema-check only accepts ".yaml" and skips ".yml" files; update the check that uses filepath.Ext(entry.Name()) so it allows both ".yaml" and ".yml" (e.g., compute ext := filepath.Ext(entry.Name()) and continue only if ext is neither ".yaml" nor ".yml"), ensuring CRDs saved with .yml are included in the scan.
41-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
upstream/mainas the local fallback.Falling back to
maindiverges from the documentedapi-lintbehavior and can compare against a stale local branch—or fail on clones that only haveupstream/main—so local verification may report the wrong schema diff.Proposed fix
- comparisonBase := flag.String("comparison-base", envOrDefault("PULL_BASE_SHA", "main"), + comparisonBase := flag.String("comparison-base", envOrDefault("PULL_BASE_SHA", "upstream/main"), "git ref to compare CRD schemas against (branch, tag, or SHA)")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/tools/crd-schema-check/main.go` around lines 41 - 42, The flag default for comparisonBase uses envOrDefault(..., "main") which can point to a stale local branch; change the fallback default string to "upstream/main" in the flag.String call that defines comparisonBase so the CLI uses upstream/main as the local fallback (keep the envOrDefault and flag.String usage intact; update only the default value passed to comparisonBase).
🧹 Nitpick comments (1)
hack/tools/go.mod (1)
18-18: ⚡ Quick winAlign direct
requireversion with the pinnedreplaceversion.
k8s.io/apiextensions-apiserveris required atv0.34.3but force-replaced tov0.34.2(line 335). Keeping these aligned avoids misleading dependency declarations and tooling confusion.Suggested diff
- k8s.io/apiextensions-apiserver v0.34.3 + k8s.io/apiextensions-apiserver v0.34.2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/tools/go.mod` at line 18, The go.mod requires k8s.io/apiextensions-apiserver at v0.34.3 but the file contains a replace pin to v0.34.2; update the direct require entry for k8s.io/apiextensions-apiserver in hack/tools/go.mod to v0.34.2 so it matches the replace directive (ensure you edit the require line that mentions k8s.io/apiextensions-apiserver).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@hack/tools/crd-schema-check/main.go`:
- Around line 138-140: The current file-extension filter in crd-schema-check
only accepts ".yaml" and skips ".yml" files; update the check that uses
filepath.Ext(entry.Name()) so it allows both ".yaml" and ".yml" (e.g., compute
ext := filepath.Ext(entry.Name()) and continue only if ext is neither ".yaml"
nor ".yml"), ensuring CRDs saved with .yml are included in the scan.
- Around line 41-42: The flag default for comparisonBase uses envOrDefault(...,
"main") which can point to a stale local branch; change the fallback default
string to "upstream/main" in the flag.String call that defines comparisonBase so
the CLI uses upstream/main as the local fallback (keep the envOrDefault and
flag.String usage intact; update only the default value passed to
comparisonBase).
---
Nitpick comments:
In `@hack/tools/go.mod`:
- Line 18: The go.mod requires k8s.io/apiextensions-apiserver at v0.34.3 but the
file contains a replace pin to v0.34.2; update the direct require entry for
k8s.io/apiextensions-apiserver in hack/tools/go.mod to v0.34.2 so it matches the
replace directive (ensure you edit the require line that mentions
k8s.io/apiextensions-apiserver).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 055a5765-7bdf-4da1-98da-f89b538788f7
📒 Files selected for processing (2)
hack/tools/crd-schema-check/main.gohack/tools/go.mod
|
Re: #8535 (review) Thanks for the review suggestions — here's my assessment of each: 1. Include 2. Use 3. Align AI-assisted response via Claude Code |
|
Re: #8535 (review) Thanks for the review, @coderabbitai. Addressing the three suggestions: 1. Include 2. Use 3. Align go.mod require version with replace (go.mod line 18) ✅ Done. AI-assisted response via Claude Code |
|
If there is already a library at openshift/crd-schema-checker, why do we need the go files? Is this how that repo says to set it up? |
|
/address-review-comments |
|
Re: #8535 (comment) Good question! The custom Go tool in
This is how AI-assisted response via Claude Code |
|
Re: #8535 (comment) Good question. The The Go files in this PR use the library's packages to add HyperShift-specific integration:
This is the same pattern AI-assisted response via Claude Code |
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Staff Engineer Review
Overall the approach is sound — integrating openshift/crd-schema-checker aligns with openshift/api patterns and this is a valuable addition to CI. The tool is well-structured and follows existing hack/tools/ conventions. See inline comments for specific issues to address before merge.
Good:
- Correct KAL comparator exclusions matching
openshift/api - Proper nil CRD handling (upstream library comparators handle it correctly)
- Clean separation of concerns and testable design
- CodeRabbit feedback has been incorporated well
Blocking issues (3): featuregated CRD variants, PULL_BASE_SHA double-defaulting, exported unused function
Suggestions (4): gomega assertions, dead code, karpenter CRDs, recursive scan fragility
| .PHONY: verify-crd-schema | ||
| verify-crd-schema: $(CRD_SCHEMA_CHECK) ## Verify CRD schemas for breaking changes against base branch. | ||
| $(CRD_SCHEMA_CHECK) --comparison-base=$(PULL_BASE_SHA) \ | ||
| --crd-dir=cmd/install/assets/crds/hypershift-operator |
There was a problem hiding this comment.
[suggestion] Karpenter CRDs are not checked.
There is also a karpenter CRD at karpenter-operator/controllers/karpenter/assets/zz_generated.crd-manifests/openshiftec2nodeclasses.crd.yaml. If this is an intentional exclusion (karpenter CRDs may have a different lifecycle), add a comment explaining why. Otherwise, add another --crd-dir.
There was a problem hiding this comment.
Done. Added --crd-dir=karpenter-operator/controllers/karpenter/assets/zz_generated.crd-manifests to the Makefile target.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Added --crd-dir=karpenter-operator/controllers/karpenter/assets/zz_generated.crd-manifests to the verify-crd-schema Makefile target.
AI-assisted response via Claude Code
| return fallback | ||
| } | ||
|
|
||
| // CompareCRDs is the testable core of the comparison logic. |
There was a problem hiding this comment.
[blocking] CompareCRDs is exported but only used in tests.
The production code in checkCRDsInDir calls config.ComparatorRegistry.Compare() directly. This exported wrapper exists only for the test file. Per project conventions, unexported symbols should be preferred unless there is an explicit external consumer.
Fix: Either unexport it (compareCRDs) or remove it entirely and have tests call config.ComparatorRegistry.Compare() directly.
There was a problem hiding this comment.
Done. Unexported to compareCRDs — no external consumers, only used in tests.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Unexported to compareCRDs — no external consumers, only used in tests.
AI-assisted response via Claude Code
| for _, e := range allErrors { | ||
| fmt.Fprintf(os.Stderr, " ERROR: %s\n", e) | ||
| } | ||
| fmt.Fprintf(os.Stderr, "\nIf this is a pre-existing violation from a moved/renamed file, use Prow /override to bypass.\n") |
There was a problem hiding this comment.
[nit] The override guidance is vague:
If this is a pre-existing violation from a moved/renamed file, use Prow /override to bypass.
Consider adding the specific job name that would need to be overridden, or referencing a doc/process for intentional breaking changes. In openshift/api, the crd-schema-checker annotations provide a more granular exception mechanism.
There was a problem hiding this comment.
Done. Updated to reference crd-schema-checker annotations for intentional changes and added the specific verify-crd-schema job name for /override.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Updated to reference crd-schema-checker annotations for intentional changes and added the specific verify-crd-schema job name for /override.
AI-assisted response via Claude Code
|
/address-review-comments |
|
🤖 Addressing review comments: workflow run |
|
Re: #8535 (review) Thanks for the thorough review, @bryan-cox. All inline comments have been addressed: Blocking (3):
Nit (1):
Suggestions (4):
All tests pass locally. AI-assisted response via Claude Code |
|
Re: #8535 (review) @bryan-cox Thank you for the thorough review. All 3 blocking issues and 4 suggestions have been addressed: Blocking (3):
Suggestions (4):
AI-assisted response via Claude Code |
|
🤖 Addressing review comments: workflow run |
|
Re: #8535 (comment) Review Comments StatusAll review threads have been addressed. Here's the current state: ✅ Resolved in code (bryan-cox, jparrill reviews)
✅ Answered today (JoelSpeed)
⏳ Open question (needs human decision)
No code changes needed this roundNo new code changes were required — all feedback is either addressed in existing commits or awaiting a policy decision. AI-assisted response via Claude Code |
|
/restructure-commits |
|
🤖 Restructuring commits: workflow run |
6e86a17 to
2c40615
Compare
|
/restructure-commits |
|
🤖 Restructuring commits: workflow run |
- Add crd-schema-check tool using crdify library to detect breaking CRD schema changes between git revisions - Wire verify-crd-schema target into Makefile verify pipeline - Support configurable allowed changes via crdify-config.yaml - Include unit tests for enum addition validation Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
- Add sigs.k8s.io/crdify library for CRD schema validation - Add github.com/onsi/gomega for test assertions Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
2c40615 to
9155e6b
Compare
|
/approve |
1 similar comment
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, hypershift-jira-solve-ci[bot], JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/lgtm |
|
Scheduling tests matching the |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
I have all the evidence I need. The analysis is clear:
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 5 test failures are in the Root CauseTransient CI image registry unavailability at The
Both HostedClusters remained Proof of transient nature: The TestAutoscaling cluster successfully pulled the exact same image digest ( PR #8535 changes are entirely unrelated: Only 6 non-vendor files changed, all under Recommendations
Evidence
|
|
/test e2e-aks |
|
/verified by GHA verify test |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@hypershift-jira-solve-ci: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
What this PR does / why we need it:
Adds automated CRD schema breaking-change detection to HyperShift CI.
A new standalone Go tool (
hack/tools/crd-schema-check) uses thekubernetes-sigs/crdifylibrary to compare CRD YAML files between a base git commit and the current working tree, catching unintentional breaking changes before merge.What the checker validates (via crdify):
Featuregate variant handling:
CustomNoUpgradesuperset CRD variant is checked-Defaultand-TechPreviewNoUpgradevariants are skipped to avoid false positives when fields move between featuregate levelsA new
verify-crd-schemaMakefile target is wired intoverify-parallel, so it runs automatically as part ofmake verifyand CI presubmit checks. The target usesPULL_BASE_SHA(set by Prow in CI, defaults toupstream/mainlocally) as the comparison baseline, following the same pattern used byapi-lint. Both the HyperShift operator CRD directory and the Karpenter CRD directory are checked.Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-502
Special notes for your reviewer:
hack/tools/crd-schema-check/as a standalone binary, consistent with the project's pattern for CI tooling.kubernetes-sigs/crdify— the upstream successor toopenshift/crd-schema-checker— for CRD validation.Checklist: