OCPBUGS-73945: Allow custom feature gates to be exempted from upgrade blocking #6139
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/severity-critical
|
@copejon: This pull request references Jira Issue OCPBUGS-73945, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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: copejon 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
copejon
force-pushed
the
ocpbugs-73945
branch
2 times, most recently
from
35dcb79 to
18ec209
Compare
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@copejon: This pull request references Jira Issue OCPBUGS-73945, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
copejon
force-pushed
the
ocpbugs-73945
branch
from
18ec209 to
716d195
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
copejon
force-pushed
the
ocpbugs-73945
branch
from
646fa3d to
9ddd7ea
Compare
|
/test e2e-aws-tests |
copejon
force-pushed
the
ocpbugs-73945
branch
from
df41a6a to
726499d
Compare
|
@copejon: 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. |
|
@coderabbitai perform full review |
|
✅ Actions performedFull review triggered. |
WalkthroughThe PR introduces a new feature gate Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/apparentlymart/go-cidr@v1.1.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/go-systemd@v0.0.0-20190321100706-95778dfbb74e: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/google/go-cmp@v0.7.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/miekg/dns@v1.1.63: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/openshift/api@v0.0.0-20251117165054-348370f055bf: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/openshift/build-machinery-go@v0.0.0-20251023084048-5d77c1a5e5af: is explicitly required in go.mod, but not marked as explicit ... [truncated 28336 characters] ... belet: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/metrics: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/mount-utils: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/pod-security-admission: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-cli-plugin: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-controller: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/admin/prerun/featuregate_lock_test.go (1)
131-136:⚠️ Potential issue | 🟡 MinorDuplicate error check — the same
erris checked and reported twice.Lines 131–133 and 134–136 are identical.
Proposed fix
err := configValidationChecksPass(featureGateLockFile{ FeatureSet: tt.fg.FeatureSet, CustomNoUpgrade: tt.fg.CustomNoUpgrade, }, &tt.fg) if err != nil { t.Errorf("featureValidationsPass() error = %v", err) } - if err != nil { - t.Errorf("featureValidationsPass() error = %v", err) - }pkg/config/apiserver.go (1)
142-157:⚠️ Potential issue | 🟠 MajorExtend
validateFeatureGatesto validateSpecialHandlingSupportExceptionRequired.The new
SpecialHandlingSupportExceptionRequiredfield is not validated (lines 187–229). This allows invalid configurations such as:
- Required feature gates listed in
SpecialHandlingSupportExceptionRequired.Disabled- The same gate in both enabled and disabled lists within
SpecialHandlingSupportExceptionRequired- Gates in
SpecialHandlingSupportExceptionRequiredthat don't exist inCustomNoUpgrade(which has no effect and may confuse users)Additionally, the
GoString()method (lines 176–177) omitsSpecialHandlingSupportExceptionRequiredfrom its output, making debugging incomplete.
🤖 Fix all issues with AI agents
In `@pkg/admin/prerun/featuregate_lock_test.go`:
- Around line 455-467: The test case labeled "minor version upgrade with special
handling support exception should succeed" uses getVersion(1, -21, 0) which
yields a major version change instead of a minor bump; update the test to use
getVersion(0, 1, 0) so currentVer represents the intended minor upgrade (e.g.,
4.21.0 -> 4.22.0). Locate the table entry in featuregate_lock_test.go (the
struct with fields name, lockFileVer, currentVer, wantErr, description,
customNoUpgrade, specialHandlingSupportException) and replace the currentVer
value for that case from getVersion(1, -21, 0) to getVersion(0, 1, 0).
🧹 Nitpick comments (1)
pkg/admin/prerun/featuregate_lock.go (1)
131-136: Minor: variable assignment between error-producing call and its error check.
lockedVersionassignment on line 133 sits between thegetExecutableVersion()call and theerrcheck. While correct, this is unusual Go style and could be slightly confusing.Proposed reorder
func upgradeChecksPass(lockFile featureGateLockFile, fgCfg *config.FeatureGates) error { currentExecutableVersion, err := getExecutableVersion() - lockedVersion := lockFile.Version if err != nil { return fmt.Errorf("failed to get current executable version: %w", err) } + lockedVersion := lockFile.Version
| { | ||
| name: "minor version upgrade with special handling support exception should succeed", | ||
| lockFileVer: getVersion(0, 0, 0), | ||
| currentVer: getVersion(1, -21, 0), | ||
| wantErr: false, | ||
| description: "minor version upgrade (4.21.0 -> 4.22.0) with special handling support exception should succeed", | ||
| customNoUpgrade: &config.EnableDisableFeatures{ | ||
| Enabled: []string{"FeatureA"}, | ||
| }, | ||
| specialHandlingSupportException: &config.EnableDisableFeatures{ | ||
| Enabled: []string{"FeatureA"}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Test case uses wrong version: tests major upgrade instead of minor upgrade.
The test name and description say "minor version upgrade (4.21.0 -> 4.22.0)" but getVersion(1, -21, 0) produces {5, 0, 0}, which is a major version change — identical to the test case above at line 442. This should use getVersion(0, 1, 0) to test an actual minor version upgrade to 4.22.0.
Proposed fix
{
name: "minor version upgrade with special handling support exception should succeed",
lockFileVer: getVersion(0, 0, 0),
- currentVer: getVersion(1, -21, 0),
+ currentVer: getVersion(0, 1, 0),
wantErr: false,
- description: "minor version upgrade (4.21.0 -> 5.0.0) with special handling support exception should succeed",
+ description: "minor version upgrade (4.21.0 -> 4.22.0) with special handling support exception should succeed",🤖 Prompt for AI Agents
In `@pkg/admin/prerun/featuregate_lock_test.go` around lines 455 - 467, The test
case labeled "minor version upgrade with special handling support exception
should succeed" uses getVersion(1, -21, 0) which yields a major version change
instead of a minor bump; update the test to use getVersion(0, 1, 0) so
currentVer represents the intended minor upgrade (e.g., 4.21.0 -> 4.22.0).
Locate the table entry in featuregate_lock_test.go (the struct with fields name,
lockFileVer, currentVer, wantErr, description, customNoUpgrade,
specialHandlingSupportException) and replace the currentVer value for that case
from getVersion(1, -21, 0) to getVersion(0, 1, 0).
3 participants
No description provided.