Revert: Fix APIs for openshift.io must have stable versions check - #31468
Conversation
After an API has stabilized to version v1, then o/api wants the earlier v1alpha1 version to be removed irrespective of the version's `served` value. So, reverting back to the original way this test was implemented.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughThe CRD stability test now skips unserved versions before it validates version stability. ChangesCRD stability validation
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/extended/operators/crd_must_be_stable.go`:
- Around line 92-95: Remove the versionSpec.Served guard in the stability
validation loop so unserved versions are still checked once the API includes v1.
Add regression coverage pairing an unserved v1alpha1 with v1, then run make
verify and make check.
🪄 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: 069ee03c-d4a0-445a-af3c-bd2b841afeae
📒 Files selected for processing (1)
test/extended/operators/crd_must_be_stable.go
| if !versionSpec.Served { | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- file outline ---'
ast-grep outline test/extended/operators/crd_must_be_stable.go --view expanded
printf '%s\n' '--- relevant implementation ---'
sed -n '1,180p' test/extended/operators/crd_must_be_stable.go
printf '%s\n' '--- related symbols and tests ---'
rg -n --glob '*.go' 'legacyCRDSsWithUnstableVersions|stableVersions|crd_must_be_stable|MustBeStable|Served' test/extended
printf '%s\n' '--- tracked files in the area ---'
git ls-files test/extended/operators | sed -n '1,120p'
printf '%s\n' '--- concise diff summary ---'
git diff --statRepository: openshift/origin
Length of output: 8215
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
p = Path("test/extended/operators/crd_must_be_stable.go")
text = p.read_text()
print("--- focused control-flow excerpt ---")
for i, line in enumerate(text.splitlines(), 1):
if 70 <= i <= 125:
print(f"{i:4}: {line}")
print("--- all version checks ---")
for i, line in enumerate(text.splitlines(), 1):
if "versionSpec" in line or "stableVersions" in line:
print(f"{i:4}: {line}")
PY
printf '%s\n' '--- test package registration and nearby tests ---'
sed -n '1,220p' test/extended/operators/operators.go 2>/dev/null || true
rg -n -C 8 --glob '*.go' 'CRD|CustomResourceDefinition|v1alpha1|v1beta1' test/extended/operators test/extended/include.goRepository: openshift/origin
Length of output: 35566
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target file ---'
cat -n test/extended/operators/crd_must_be_stable.go | sed -n '1,150p'
printf '%s\n' '--- relevant repository references ---'
rg -n -C 6 --glob '*.go' 'legacyCRDSsWithUnstableVersions|stableVersions|versionSpec\.Served|crd_must_be_stable' .
printf '%s\n' '--- operator test files ---'
git ls-files 'test/extended/operators/**' | sed -n '1,160p'Repository: openshift/origin
Length of output: 12353
🏁 Script executed:
#!/bin/bash
set -e
cat -n test/extended/operators/crd_must_be_stable.go | sed -n '1,150p'
rg -n -C 6 --glob '*.go' 'legacyCRDSsWithUnstableVersions|stableVersions|versionSpec\.Served|crd_must_be_stable' .
git ls-files 'test/extended/operators/**' | sed -n '1,160p'Repository: openshift/origin
Length of output: 12266
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- repository state and commit context ---'
git status --short
git branch --show-current
git log -5 --oneline -- test/extended/operators/crd_must_be_stable.go
printf '%s\n' '--- CRD version declarations in source/manifests ---'
rg -n --glob '*.{yaml,yml,json,go}' 'served:\s*(true|false)|name:\s*v1(alpha|beta)' . | sed -n '1,240p'
printf '%s\n' '--- include registration ---'
rg -n 'operators' test/extended/include.go test/extended --glob 'include.go'
printf '%s\n' '--- standalone behavior probe ---'
python3 - <<'PY'
stable = {"v1", "v2", "v3", "v4", "v5"}
legacy = {("example.openshift.io", "v1alpha1")}
cases = [
("new.openshift.io", "v1alpha1", False),
("new.openshift.io", "v1alpha1", True),
("new.openshift.io", "v1", False),
("example.openshift.io", "v1alpha1", False),
]
for name, version, served in cases:
skipped_legacy = (name, version) in legacy
old_result = "failure" if (not skipped_legacy and served and version not in stable) else "pass"
proposed_without_served_guard = "failure" if (not skipped_legacy and version not in stable) else "pass"
print(f"{name} {version} served={served}: current={old_result}, without_guard={proposed_without_served_guard}")
PYRepository: openshift/origin
Length of output: 22135
🏁 Script executed:
#!/bin/bash
set -e
git status --short
git branch --show-current
git log -5 --oneline -- test/extended/operators/crd_must_be_stable.go
rg -n --glob '*.{yaml,yml,json,go}' 'served:\s*(true|false)|name:\s*v1(alpha|beta)' . | sed -n '1,240p'
rg -n 'operators' test/extended/include.go test/extended --glob 'include.go'
python3 - <<'PY'
stable = {"v1", "v2", "v3", "v4", "v5"}
legacy = {("example.openshift.io", "v1alpha1")}
for name, version, served in [
("new.openshift.io", "v1alpha1", False),
("new.openshift.io", "v1alpha1", True),
("new.openshift.io", "v1", False),
("example.openshift.io", "v1alpha1", False),
]:
exempt = (name, version) in legacy
current = "failure" if not exempt and served and version not in stable else "pass"
without_guard = "failure" if not exempt and version not in stable else "pass"
print(f"{name} {version} served={served}: current={current}, without_guard={without_guard}")
PYRepository: openshift/origin
Length of output: 21975
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- revert commit ---'
git show --format=fuller --stat 1761bfa5c9
git show --format= --unified=25 1761bfa5c9 -- test/extended/operators/crd_must_be_stable.go
printf '%s\n' '--- preceding implementation change ---'
git show --format=fuller --stat 5b828df7f1
git show --format= --unified=20 5b828df7f1 -- test/extended/operators/crd_must_be_stable.go
printf '%s\n' '--- commit ancestry for the target file ---'
git log --oneline --follow -- test/extended/operators/crd_must_be_stable.go | sed -n '1,30p'Repository: openshift/origin
Length of output: 8005
🏁 Script executed:
#!/bin/bash
set -e
git show --format=fuller --stat 1761bfa5c9
git show --format= --unified=25 1761bfa5c9 -- test/extended/operators/crd_must_be_stable.go
git show --format=fuller --stat 5b828df7f1
git show --format= --unified=20 5b828df7f1 -- test/extended/operators/crd_must_be_stable.go
git log --oneline --follow -- test/extended/operators/crd_must_be_stable.go | sed -n '1,30p'Repository: openshift/origin
Length of output: 7899
Do not skip unserved unstable versions.
The stability requirement applies after an API reaches v1, regardless of served. Remove this guard. Add regression coverage for an unserved v1alpha1 alongside v1.
Proposed fix
- if !versionSpec.Served {
- continue
- }
-Run make verify and make check after the change.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !versionSpec.Served { | |
| continue | |
| } |
🤖 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 `@test/extended/operators/crd_must_be_stable.go` around lines 92 - 95, Remove
the versionSpec.Served guard in the stability validation loop so unserved
versions are still checked once the API includes v1. Add regression coverage
pairing an unserved v1alpha1 with v1, then run make verify and make check.
Source: Coding guidelines
|
Scheduling required tests: |
|
/lgtm |
openshift-ci-robot
added
the
verified
|
@sdodson: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sadasu, sdodson 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
3 participants
After an API has stabilized to version v1, then o/api wants the earlier v1alpha1 version to be removed irrespective of the version's
servedvalue.So, reverting back to the original way this test was implemented.
Reverting change introduced by #31458
Summary by CodeRabbit