OCPBUGS-73945: Allow custom feature gates to be exempted from upgrade blocking#6139
OCPBUGS-73945: Allow custom feature gates to be exempted from upgrade blocking#6139copejon wants to merge 10 commits intoopenshift:mainfrom
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
|
@coderabbitai perform full review |
|
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds a new feature-gate group Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
|
@coderabbitai update PR description |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 944
OverviewThis PR implements support for exempting custom feature gates from blocking upgrades in MicroShift. It introduces a new configuration field that allows administrators to specify which custom feature gates should be permitted during version upgrades, addressing scenarios where certain non-default feature gates need to remain enabled across upgrades without blocking the process. ChangesConfiguration Schema Updates
Upgrade Logic Enhancements
Documentation & Configuration
Testing
Related IssueFixes: OCPBUGS-73945 Testing Performed
You can update the PR description by clicking the "Edit" button next to the PR title on the GitHub web interface and pasting the above content. |
|
/cherry-pick release-4.21 |
|
@copejon: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
@copejon: This pull request references Jira Issue OCPBUGS-73945, which is valid. 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. |
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 (1)
pkg/admin/prerun/featuregate_lock_test.go (1)
127-138:⚠️ Potential issue | 🟡 MinorDuplicate error check and unused
wantfield.Lines 131–133 and 134–136 are identical checks. Also,
want boolis declared in the test struct (line 82) but never asserted against — so this test doesn't actually verify the expected outcome for each case.Proposed fix
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { 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) + if (err != nil) != !tt.want { + t.Errorf("configValidationChecksPass() error = %v, wantConfigured = %v", err, tt.want) } }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/admin/prerun/featuregate_lock_test.go` around lines 127 - 138, The test contains a duplicated error check and never asserts the declared expectation: remove the duplicate if err != nil { t.Errorf(...) } block and replace the remaining error assertion with an assertion that checks the test case's expected outcome (tt.want) by comparing whether err is nil to tt.want (e.g., got := err == nil; if got != tt.want { t.Errorf(... ) }). Update the invocation around configValidationChecksPass and featureGateLockFile to use this single check so the test actually verifies the want bool from the test struct.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/admin/prerun/featuregate_lock_test.go`:
- Around line 491-496: The test currently dereferences tt.customNoUpgrade
unconditionally when constructing the featureGateLockFile
(FeatureSet/CustomNoUpgrade/Version), creating a nil-pointer panic risk if a
future case omits customNoUpgrade; change the test to guard the dereference
(e.g., only set CustomNoUpgrade when tt.customNoUpgrade != nil) or populate a
safe default before use, or alternatively change
featureGateLockFile.CustomNoUpgrade to a non-pointer value; update the
constructor in featuregate_lock_test.go (the code that builds lockFile) to check
tt.customNoUpgrade != nil and assign safely to
featureGateLockFile.CustomNoUpgrade to avoid panics.
- Around line 127-138: The test contains a duplicated error check and never
asserts the declared expectation: remove the duplicate if err != nil {
t.Errorf(...) } block and replace the remaining error assertion with an
assertion that checks the test case's expected outcome (tt.want) by comparing
whether err is nil to tt.want (e.g., got := err == nil; if got != tt.want {
t.Errorf(... ) }). Update the invocation around configValidationChecksPass and
featureGateLockFile to use this single check so the test actually verifies the
want bool from the test struct.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@pkg/admin/prerun/featuregate_lock_test.go`: - Around line 491-496: The test currently dereferences tt.customNoUpgrade unconditionally when constructing the featureGateLockFile (FeatureSet/CustomNoUpgrade/Version), creating a nil-pointer panic risk if a future case omits customNoUpgrade; change the test to guard the dereference (e.g., only set CustomNoUpgrade when tt.customNoUpgrade != nil) or populate a safe default before use, or alternatively change featureGateLockFile.CustomNoUpgrade to a non-pointer value; update the constructor in featuregate_lock_test.go (the code that builds lockFile) to check tt.customNoUpgrade != nil and assign safely to featureGateLockFile.CustomNoUpgrade to avoid panics.pkg/admin/prerun/featuregate_lock_test.go (1)
491-496: Nil-pointer dereference risk if a future test case omitscustomNoUpgrade.All current cases set
customNoUpgrade, but the field is a pointer that's unconditionally dereferenced at line 494. A guard or switching to a value type would prevent a future panicking test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/admin/prerun/featuregate_lock_test.go` around lines 491 - 496, The test currently dereferences tt.customNoUpgrade unconditionally when constructing the featureGateLockFile (FeatureSet/CustomNoUpgrade/Version), creating a nil-pointer panic risk if a future case omits customNoUpgrade; change the test to guard the dereference (e.g., only set CustomNoUpgrade when tt.customNoUpgrade != nil) or populate a safe default before use, or alternatively change featureGateLockFile.CustomNoUpgrade to a non-pointer value; update the constructor in featuregate_lock_test.go (the code that builds lockFile) to check tt.customNoUpgrade != nil and assign safely to featureGateLockFile.CustomNoUpgrade to avoid panics.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/admin/prerun/featuregate_lock_test.go (1)
440-465: Special handling exception test cases look good — consider adding a partial-exemption case.Both new cases test full exemption (all custom gates appear in
specialHandlingSupportException). A case where only some custom gates are exempted (e.g.,customNoUpgrade: {Enabled: ["FeatureA", "FeatureB"]}withspecialHandlingSupportException: {Enabled: ["FeatureA"]}) would exercise the set-difference logic and confirm the upgrade is still blocked.Similarly, there's no coverage for
Disabledfeature lists in the exemption path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/admin/prerun/featuregate_lock_test.go` around lines 440 - 465, Add at least one test case to featuregate_lock_test.go that covers partial exemptions and one that covers Disabled lists: create a case where customNoUpgrade has Enabled: ["FeatureA","FeatureB"] but specialHandlingSupportException has Enabled: ["FeatureA"] to ensure the set-difference blocks the upgrade, and another case where customNoUpgrade/ specialHandlingSupportException use the Disabled slice (e.g., customNoUpgrade.Disabled: ["FeatureC","FeatureD"] with specialHandlingSupportException.Disabled: ["FeatureC"]) to exercise the exemption logic for Disabled entries; use the same test struct fields (name, lockFileVer, currentVer, wantErr, description, customNoUpgrade, specialHandlingSupportException) and follow the existing pattern for getVersion to add these scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/admin/prerun/featuregate_lock_test.go`:
- Around line 440-465: Add at least one test case to featuregate_lock_test.go
that covers partial exemptions and one that covers Disabled lists: create a case
where customNoUpgrade has Enabled: ["FeatureA","FeatureB"] but
specialHandlingSupportException has Enabled: ["FeatureA"] to ensure the
set-difference blocks the upgrade, and another case where customNoUpgrade/
specialHandlingSupportException use the Disabled slice (e.g.,
customNoUpgrade.Disabled: ["FeatureC","FeatureD"] with
specialHandlingSupportException.Disabled: ["FeatureC"]) to exercise the
exemption logic for Disabled entries; use the same test struct fields (name,
lockFileVer, currentVer, wantErr, description, customNoUpgrade,
specialHandlingSupportException) and follow the existing pattern for getVersion
to add these scenarios.
|
@copejon: The following tests failed, say
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. |
|
/test ocp-full-conformance-rhel-eus |
4 participants
Summary by CodeRabbit
New Features
Documentation
Chores