NO-JIRA: Allow for risks only desired update#1335
NO-JIRA: Allow for risks only desired update#1335openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@hongkailiu: This pull request explicitly references no jira issue. 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:
WalkthroughValidateClusterVersion now accepts a second parameter Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/validation/validation_test.go (2)
55-55: Consider simplifying error list construction.The
append(make(field.ErrorList, 0), ...)pattern is verbose. A simpler approach:Proposed simplification
- expect: append(make(field.ErrorList, 0), field.Required(field.NewPath("spec", "desiredUpdate", "acceptRisks"), "acceptRisks must not be specified")), + expect: field.ErrorList{field.Required(field.NewPath("spec", "desiredUpdate", "acceptRisks"), "acceptRisks must not be specified")},Apply similarly to lines 66 and 76.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/validation/validation_test.go` at line 55, The test constructs expected validation errors using the verbose append(make(field.ErrorList, 0), ...) pattern; replace those with a literal composite initializer like field.ErrorList{ field.Required(field.NewPath("spec", "desiredUpdate", "acceptRisks"), "acceptRisks must not be specified") } to simplify; do the same replacement for the similar expectations at the other occurrences (the lines that create field.ErrorList for other cases) so all expected lists use field.ErrorList{ ... } instead of append(make(...), ...).
14-87: Consider adding additional test coverage.The current tests cover the basic acceptRisks scenarios well. Consider adding tests for:
- Combined fields: When
shouldReconcileAcceptRisks=trueand bothAcceptRisksANDImage/Versionare provided (should this be allowed?).- Nil DesiredUpdate: Verify no panic when
config.Spec.DesiredUpdateis nil.Would you like me to generate additional test cases?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/validation/validation_test.go` around lines 14 - 87, Add two unit tests inside TestValidateClusterVersion to cover the missing cases: (1) a case where shouldReconcileAcceptRisks is true and config.Spec.DesiredUpdate includes both AcceptRisks and other fields (e.g. Image and Version) — call ValidateClusterVersion and assert the returned field.ErrorList matches the current validator’s behavior (expect no error if combined fields are allowed by ValidateClusterVersion); (2) a case where config.Spec.DesiredUpdate is nil — call ValidateClusterVersion(nil DesiredUpdate) and assert it does not panic and returns the expected field.ErrorList (empty or the current validator’s expected error). Reference TestValidateClusterVersion and the ValidateClusterVersion function when adding these cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/validation/validation_test.go`:
- Around line 57-77: Update the two table-driven test cases in
validation_test.go that assert the error message for empty Spec.DesiredUpdate:
keep the first case (where shouldReconcileAcceptRisks is true) expecting
field.Required(..., "must specify architecture, version, image, acceptRisks"),
but change the second case (where shouldReconcileAcceptRisks is false) to expect
field.Required(..., "must specify architecture, version, or image") to match the
updated validation behavior; locate the expectations constructed with
field.Required(field.NewPath("spec", "desiredUpdate"), ...) and adjust the
message string accordingly.
In `@lib/validation/validation.go`:
- Around line 35-36: The validation currently appends a field.Required error
when acceptRisks is present but shouldReconcileAcceptRisks is false; change this
to append a field.Forbidden error instead so the error type correctly indicates
the field must not be specified—update the call in the same block that
references shouldReconcileAcceptRisks and u.AcceptRisks to use
field.Forbidden(field.NewPath("spec","desiredUpdate","acceptRisks"),
"acceptRisks must not be specified").
- Around line 37-40: The switch branch is producing unreachable code and an
incorrect error message; replace the two overlapping cases by first checking if
u.Architecture, u.Version, and u.Image are all empty, then inside that branch
inspect shouldReconcileAcceptRisks: if shouldReconcileAcceptRisks is true
require AcceptRisks too (append
field.Required(field.NewPath("spec","desiredUpdate"), "must specify
architecture, version, image, or acceptRisks") when u.AcceptRisks is empty),
otherwise append the message that does not mention acceptRisks (e.g., "must
specify architecture, version, or image"); remove the unreachable case and keep
references to u.Architecture, u.Version, u.Image, u.AcceptRisks,
shouldReconcileAcceptRisks, errs and field.NewPath("spec","desiredUpdate") to
locate and update the logic.
---
Nitpick comments:
In `@lib/validation/validation_test.go`:
- Line 55: The test constructs expected validation errors using the verbose
append(make(field.ErrorList, 0), ...) pattern; replace those with a literal
composite initializer like field.ErrorList{ field.Required(field.NewPath("spec",
"desiredUpdate", "acceptRisks"), "acceptRisks must not be specified") } to
simplify; do the same replacement for the similar expectations at the other
occurrences (the lines that create field.ErrorList for other cases) so all
expected lists use field.ErrorList{ ... } instead of append(make(...), ...).
- Around line 14-87: Add two unit tests inside TestValidateClusterVersion to
cover the missing cases: (1) a case where shouldReconcileAcceptRisks is true and
config.Spec.DesiredUpdate includes both AcceptRisks and other fields (e.g. Image
and Version) — call ValidateClusterVersion and assert the returned
field.ErrorList matches the current validator’s behavior (expect no error if
combined fields are allowed by ValidateClusterVersion); (2) a case where
config.Spec.DesiredUpdate is nil — call ValidateClusterVersion(nil
DesiredUpdate) and assert it does not panic and returns the expected
field.ErrorList (empty or the current validator’s expected error). Reference
TestValidateClusterVersion and the ValidateClusterVersion function when adding
these cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bdf57353-952c-4da9-8294-604feb973ea6
📒 Files selected for processing (4)
lib/validation/validation.golib/validation/validation_test.gopkg/cvo/cvo.gopkg/cvo/cvo_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (2)
lib/validation/validation.go (2)
35-36:⚠️ Potential issue | 🟡 MinorIncorrect error type: use
field.Forbiddeninstead offield.Required.
field.Requiredsemantically indicates a missing required field. For a field that must not be specified,field.Forbiddenis the correct choice.Proposed fix
case !shouldReconcileAcceptRisks && len(u.AcceptRisks) > 0: - errs = append(errs, field.Required(field.NewPath("spec", "desiredUpdate", "acceptRisks"), "acceptRisks must not be specified")) + errs = append(errs, field.Forbidden(field.NewPath("spec", "desiredUpdate", "acceptRisks"), "acceptRisks is not supported when the feature is disabled"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/validation/validation.go` around lines 35 - 36, Replace the incorrect usage of field.Required with field.Forbidden for the acceptance-case check: when shouldReconcileAcceptRisks is false but u.AcceptRisks is non-empty, update the append call that adds to errs (the one referencing field.NewPath("spec","desiredUpdate","acceptRisks")) to use field.Forbidden instead of field.Required so the error correctly represents a forbidden/undeclared field; the condition and the variables (shouldReconcileAcceptRisks, u.AcceptRisks, errs) should remain unchanged.
37-40:⚠️ Potential issue | 🟠 MajorCase at lines 39-40 is unreachable dead code; Case 2 message may mislead.
When
!shouldReconcileAcceptRisksis true andarch/version/imageare all empty:
- If
AcceptRisksis empty → Case 2 (line 37) matches first- If
AcceptRisksis non-empty → Case 1 (line 35) matches firstCase 3 can never execute. Additionally, when
shouldReconcileAcceptRisks=falseand all fields are empty, Case 2 matches with an error message mentioning "acceptRisks" as a valid option, which is misleading since the feature is disabled.Proposed fix
case !shouldReconcileAcceptRisks && len(u.AcceptRisks) > 0: - errs = append(errs, field.Required(field.NewPath("spec", "desiredUpdate", "acceptRisks"), "acceptRisks must not be specified")) - case len(u.Architecture) == 0 && len(u.Version) == 0 && len(u.Image) == 0 && len(u.AcceptRisks) == 0: - errs = append(errs, field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, image, or acceptRisks")) - case len(u.Architecture) == 0 && len(u.Version) == 0 && len(u.Image) == 0 && !shouldReconcileAcceptRisks: - errs = append(errs, field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, or image")) + errs = append(errs, field.Forbidden(field.NewPath("spec", "desiredUpdate", "acceptRisks"), "acceptRisks is not supported when the feature is disabled")) + case len(u.Architecture) == 0 && len(u.Version) == 0 && len(u.Image) == 0: + if len(u.AcceptRisks) == 0 { + if shouldReconcileAcceptRisks { + errs = append(errs, field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, image, or acceptRisks")) + } else { + errs = append(errs, field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, or image")) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/validation/validation.go` around lines 37 - 40, The two case branches checking u.Architecture/u.Version/u.Image emptiness and acceptRisks are redundant and make one branch unreachable; consolidate into a single conditional that checks if len(u.Architecture)==0 && len(u.Version)==0 && len(u.Image)==0 and then branch on shouldReconcileAcceptRisks: if true append field.Required(field.NewPath("spec","desiredUpdate"), "must specify architecture, version, image, or acceptRisks"), otherwise append field.Required(field.NewPath("spec","desiredUpdate"), "must specify architecture, version, or image"); remove the unreachable case and ensure you still reference u.AcceptRisks only for messaging, keeping errs and field.Required/field.NewPath calls as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/validation/validation.go`:
- Around line 35-36: Replace the incorrect usage of field.Required with
field.Forbidden for the acceptance-case check: when shouldReconcileAcceptRisks
is false but u.AcceptRisks is non-empty, update the append call that adds to
errs (the one referencing field.NewPath("spec","desiredUpdate","acceptRisks"))
to use field.Forbidden instead of field.Required so the error correctly
represents a forbidden/undeclared field; the condition and the variables
(shouldReconcileAcceptRisks, u.AcceptRisks, errs) should remain unchanged.
- Around line 37-40: The two case branches checking
u.Architecture/u.Version/u.Image emptiness and acceptRisks are redundant and
make one branch unreachable; consolidate into a single conditional that checks
if len(u.Architecture)==0 && len(u.Version)==0 && len(u.Image)==0 and then
branch on shouldReconcileAcceptRisks: if true append
field.Required(field.NewPath("spec","desiredUpdate"), "must specify
architecture, version, image, or acceptRisks"), otherwise append
field.Required(field.NewPath("spec","desiredUpdate"), "must specify
architecture, version, or image"); remove the unreachable case and ensure you
still reference u.AcceptRisks only for messaging, keeping errs and
field.Required/field.NewPath calls as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a738f1c2-bbd8-41f3-929b-c5c31e70767b
📒 Files selected for processing (1)
lib/validation/validation.go
hongkailiu
force-pushed
the
allow-accept-risks
branch
from
ce1a27d to
2a66574
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/validation/validation.go (1)
37-38:⚠️ Potential issue | 🟡 MinorFeature-disabled validation message still advertises
acceptRisksas valid.When
shouldReconcileAcceptRisksis false andspec.desiredUpdateis empty, this message still saysacceptRisksis an allowed option, which conflicts with Line 35-36 behavior.Proposed fix
- case len(u.Architecture) == 0 && len(u.Version) == 0 && len(u.Image) == 0 && len(u.AcceptRisks) == 0: - errs = append(errs, field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, image, or acceptRisks")) + case len(u.Architecture) == 0 && len(u.Version) == 0 && len(u.Image) == 0 && len(u.AcceptRisks) == 0: + if shouldReconcileAcceptRisks { + errs = append(errs, field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, image, or acceptRisks")) + } else { + errs = append(errs, field.Required(field.NewPath("spec", "desiredUpdate"), "must specify architecture, version, or image")) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/validation/validation.go` around lines 37 - 38, The required-field error message for the spec.desiredUpdate check still mentions "acceptRisks" even when the feature gate shouldReconcileAcceptRisks is false; update the logic around the case that builds errs (the branch with len(u.Architecture) == 0 && len(u.Version) == 0 && len(u.Image) == 0 && len(u.AcceptRisks) == 0) to choose the error text based on shouldReconcileAcceptRisks (or split into two cases): when shouldReconcileAcceptRisks is true keep "must specify architecture, version, image, or acceptRisks", otherwise use "must specify architecture, version, or image" and append the result via the field.Required call for field.NewPath("spec","desiredUpdate").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/validation/validation.go`:
- Around line 37-38: The required-field error message for the spec.desiredUpdate
check still mentions "acceptRisks" even when the feature gate
shouldReconcileAcceptRisks is false; update the logic around the case that
builds errs (the branch with len(u.Architecture) == 0 && len(u.Version) == 0 &&
len(u.Image) == 0 && len(u.AcceptRisks) == 0) to choose the error text based on
shouldReconcileAcceptRisks (or split into two cases): when
shouldReconcileAcceptRisks is true keep "must specify architecture, version,
image, or acceptRisks", otherwise use "must specify architecture, version, or
image" and append the result via the field.Required call for
field.NewPath("spec","desiredUpdate").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 226d134f-5c56-4180-9205-288567a18a78
📒 Files selected for processing (4)
lib/validation/validation.golib/validation/validation_test.gopkg/cvo/cvo.gopkg/cvo/cvo_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/validation/validation_test.go
hongkailiu
force-pushed
the
allow-accept-risks
branch
2 times, most recently
from
f35e2cf to
72c0f04
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/validation/validation.go`:
- Around line 37-38: The error string in the validation branch that checks
len(u.Architecture)==0 && len(u.Version)==0 && len(u.Image)==0 &&
!shouldReconcileAcceptRisks is wrong: update the message passed to
field.Required (the call that appends to errs using
field.NewPath("spec","desiredUpdate")) to remove any reference to "acceptRisks"
and include "image" so it correctly reads that the user must specify
architecture, version, or image when shouldReconcileAcceptRisks is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8679795f-8eb3-4236-b8d5-a4de5a34f468
📒 Files selected for processing (4)
lib/validation/validation.golib/validation/validation_test.gopkg/cvo/cvo.gopkg/cvo/cvo_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/validation/validation_test.go
- pkg/cvo/cvo.go
This is to address the issue from [1]. We need to allow for n desired update with accept risks and without image, version and arch. [1]. openshift#1318 (comment)
hongkailiu
force-pushed
the
allow-accept-risks
branch
from
72c0f04 to
d36ceeb
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu, wking 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 |
|
We're continuing to iterate on this, and there are several more pulls open for the CVO polishing accepted-risks stuff. I am fine with CI passing, and no further manual verification here. /verified by @wking |
openshift-ci-robot
added
the
verified
|
@wking: 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. |
|
/test e2e-agnostic-ovn-techpreview-serial |
|
@hongkailiu: 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. |
3 participants
This is to address the issue from [1].
We need to allow for n desired update with
accept risks and without image, version and
arch.
[1]. #1318 (comment)
Summary by CodeRabbit
Bug Fixes
Tests