[wip]OTA-1548: set up accepted risks#2170
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@hongkailiu: This pull request references OTA-1548 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 "4.22.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. |
WalkthroughAdds a new hidden Cobra subcommand to manage ClusterVersion accepted update risks, wires it behind a feature gate, propagates AcceptRisks handling into upgrade logic, updates dependencies, exposes unit tests for risk computation, and makes minor CLI and describer test adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "oc upgrade accept\n(Cobra cmd)"
participant Client as "ClusterVersion client"
participant API as "Kubernetes API / ClusterVersion"
User->>CLI: invoke command (+/-/risk tokens, --replace/--clear)
CLI->>CLI: parse flags, validate args (mutual exclusivity, tokens)
CLI->>Client: GET ClusterVersion
Client->>API: Fetch cv
API-->>Client: cv with spec.desiredUpdate.acceptRisks
Client-->>CLI: return cv
CLI->>CLI: compute new acceptRisks (getAcceptRisks)
alt changes needed
CLI->>Client: PATCH ClusterVersion (JSON Merge Patch)
Client->>API: apply patch
API-->>Client: patched cv
Client-->>CLI: success
CLI->>User: print new acceptRisks summary
else no changes
CLI->>User: print "no changes" message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
hongkailiu
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
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 (1)
pkg/cli/admin/upgrade/accept/accept.go (1)
134-136: Placeholder logic pending API update.The hardcoded fake risks bypass actual ClusterVersion data. Ensure this is tracked for completion once the
o/apidependency is updated.Would you like me to open an issue to track this TODO?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/cli/admin/upgrade/accept/accept.gopkg/cli/admin/upgrade/upgrade.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cli/admin/upgrade/accept/accept.gopkg/cli/admin/upgrade/upgrade.go
🧬 Code graph analysis (2)
pkg/cli/admin/upgrade/accept/accept.go (1)
pkg/cli/admin/upgrade/upgrade.go (1)
New(56-132)
pkg/cli/admin/upgrade/upgrade.go (1)
pkg/cli/admin/upgrade/accept/accept.go (1)
New(29-57)
🔇 Additional comments (1)
pkg/cli/admin/upgrade/upgrade.go (1)
28-28: LGTM!The import and feature gate wiring follow the established pattern used for the
statusandrollbacksubcommands.Also applies to: 126-128
hongkailiu
force-pushed
the
accept-cmd
branch
2 times, most recently
from
b4dc31c to
1efbc2c
Compare
hongkailiu
force-pushed
the
accept-cmd
branch
from
1efbc2c to
7e52894
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hongkailiu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
bot
removed
the
approved
hongkailiu
force-pushed
the
accept-cmd
branch
from
7e52894 to
62e2d9a
Compare
|
I will wait a bit on |
hongkailiu
force-pushed
the
accept-cmd
branch
5 times, most recently
from
0a916fe to
87f10fc
Compare
With OC_ENABLE_CMD_UPGRADE_ACCEPT_RISKS=true, a new command `oc adm upgrade accept` is enabled. It accepts comma-separated risks exposed to an OpenShift release [1]. The risks are stored in `clusterversion/version`'s `.specs.desiredUpdate.acceptRisks`. [1]. https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html-single/updating_clusters/index#understanding-clusterversion-conditiontypes_understanding-openshift-updates
hongkailiu
force-pushed
the
accept-cmd
branch
from
87f10fc to
583aa51
Compare
|
Cluster bot: Testing results with 583aa51: So we showed that the oc/pkg/cli/admin/upgrade/upgrade.go Line 688 in 90d7ae6 |
|
We also need to provide users with help information but it seems there are no any info: |
Try this? |
|
Tested with 254493c |
|
I just noticed an issue that we do not show help info for Right now there are descriptions for |
|
@hongkailiu I mean we need a command help info, like what we have for |
|
@hongkailiu: This pull request references OTA-1548 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 "4.22.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. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/cli/admin/upgrade/upgrade.go (1)
236-236: Use existing request context for the clear patch call.At Line [236], using
context.TODO()bypasses thectxalready established in this method. Prefer passingctxfor cancellation/deadline propagation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/admin/upgrade/upgrade.go` at line 236, The Patch call using context.TODO() should use the existing request context for cancellation/deadline propagation: replace the context.TODO() argument in o.Client.ConfigV1().ClusterVersions().Patch(...) with the method's ctx variable so the Patch operation honors the caller's context.pkg/cli/admin/upgrade/accept/accept.go (1)
57-57: Remove commented-out command config.At Line [57], stale commented code (
//Hidden: true,) is confusing now that visibility is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/admin/upgrade/accept/accept.go` at line 57, Remove the stale commented-out command configuration line "//Hidden: true," from the accept command definition to avoid confusion; locate the accept command (e.g., the acceptCmd or NewAcceptCmd command/constructor in accept.go) and delete that commented line so the command configuration contains only active fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cli/admin/upgrade/accept/accept_test.go`:
- Around line 55-56: The test's diff argument order is inverted: change the
cmp.Diff call in the accept_test.go assertion from cmp.Diff(actual,
testCase.expected) to cmp.Diff(testCase.expected, actual) so the produced diff
matches the "(-want +got)" label; this affects the assertion around
getAcceptRisks() where variables actual and testCase.expected are compared using
cmp.Diff.
In `@pkg/cli/admin/upgrade/accept/accept.go`:
- Line 49: The long help text for the accept command contains a typo: change the
phrase "excepted risks" to "accepted risks" in the long description string (the
Long/LongHelp text associated with the accept command, e.g., in the acceptCmd
declaration or its help variable in accept.go) so the user-facing help displays
"accepted risks".
- Around line 155-156: The call to patchDesiredUpdate currently uses
context.TODO(), which prevents Run(ctx) cancellation/timeouts from propagating;
change the call to pass the incoming context (ctx) instead. Locate the
invocation of patchDesiredUpdate in the accept flow (the call using
patchDesiredUpdate(context.TODO(), acceptRisks,
o.Client.ConfigV1().ClusterVersions(), "version")) and replace the placeholder
context with the Run(ctx) parameter so patchDesiredUpdate receives the caller's
context; ensure any surrounding functions (e.g., Run(ctx)) keep ctx in scope and
thread it through to this call.
In `@pkg/cli/admin/upgrade/upgrade.go`:
- Around line 269-271: The code dereferences cv.Spec.DesiredUpdate.AcceptRisks
without guarding for nil; update the logic around the assignment to
update.AcceptRisks so you first check that cv.Spec != nil and
cv.Spec.DesiredUpdate != nil before reading AcceptRisks (and fall back to a safe
default, e.g. false, when DesiredUpdate is nil). Apply the same nil-check fix at
the other occurrence referencing cv.Spec.DesiredUpdate.AcceptRisks (the second
instance noted) so both reads are protected and cannot panic.
---
Nitpick comments:
In `@pkg/cli/admin/upgrade/accept/accept.go`:
- Line 57: Remove the stale commented-out command configuration line "//Hidden:
true," from the accept command definition to avoid confusion; locate the accept
command (e.g., the acceptCmd or NewAcceptCmd command/constructor in accept.go)
and delete that commented line so the command configuration contains only active
fields.
In `@pkg/cli/admin/upgrade/upgrade.go`:
- Line 236: The Patch call using context.TODO() should use the existing request
context for cancellation/deadline propagation: replace the context.TODO()
argument in o.Client.ConfigV1().ClusterVersions().Patch(...) with the method's
ctx variable so the Patch operation honors the caller's context.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.modpkg/cli/admin/upgrade/accept/accept.gopkg/cli/admin/upgrade/accept/accept_test.gopkg/cli/admin/upgrade/rollback/rollback.gopkg/cli/admin/upgrade/upgrade.gopkg/helpers/describe/describer_test.go
| if diff := cmp.Diff(actual, testCase.expected); diff != "" { | ||
| t.Errorf("getAcceptRisks() mismatch (-want +got):\n%s", diff) |
There was a problem hiding this comment.
Fix cmp.Diff argument order in failure output.
At Line [55], cmp.Diff(actual, testCase.expected) conflicts with the (-want +got) label and inverts diff semantics.
Proposed fix
- if diff := cmp.Diff(actual, testCase.expected); diff != "" {
+ if diff := cmp.Diff(testCase.expected, actual); diff != "" {
t.Errorf("getAcceptRisks() mismatch (-want +got):\n%s", diff)
}📝 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 diff := cmp.Diff(actual, testCase.expected); diff != "" { | |
| t.Errorf("getAcceptRisks() mismatch (-want +got):\n%s", diff) | |
| if diff := cmp.Diff(testCase.expected, actual); diff != "" { | |
| t.Errorf("getAcceptRisks() mismatch (-want +got):\n%s", diff) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cli/admin/upgrade/accept/accept_test.go` around lines 55 - 56, The test's
diff argument order is inverted: change the cmp.Diff call in the accept_test.go
assertion from cmp.Diff(actual, testCase.expected) to
cmp.Diff(testCase.expected, actual) so the produced diff matches the "(-want
+got)" label; this affects the assertion around getAcceptRisks() where variables
actual and testCase.expected are compared using cmp.Diff.
| removal if it exists and no-ops otherwise. If --replace is specified, the prefix "-" on the risks | ||
| is not allowed. | ||
|
|
||
| Passing --clear removes all existing excepted risks. |
There was a problem hiding this comment.
User-facing typo in long help text.
At Line [49], “excepted risks” should be “accepted risks”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cli/admin/upgrade/accept/accept.go` at line 49, The long help text for
the accept command contains a typo: change the phrase "excepted risks" to
"accepted risks" in the long description string (the Long/LongHelp text
associated with the accept command, e.g., in the acceptCmd declaration or its
help variable in accept.go) so the user-facing help displays "accepted risks".
| if err := patchDesiredUpdate(context.TODO(), acceptRisks, o.Client.ConfigV1().ClusterVersions(), "version"); err != nil { | ||
| return err |
There was a problem hiding this comment.
Use the passed context for patch calls.
At Line [155], context.TODO() ignores the caller context from Run(ctx), so cancellation/timeouts won’t propagate to the API call.
Proposed fix
- if err := patchDesiredUpdate(context.TODO(), acceptRisks, o.Client.ConfigV1().ClusterVersions(), "version"); err != nil {
+ if err := patchDesiredUpdate(ctx, acceptRisks, o.Client.ConfigV1().ClusterVersions(), "version"); err != nil {
return err
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cli/admin/upgrade/accept/accept.go` around lines 155 - 156, The call to
patchDesiredUpdate currently uses context.TODO(), which prevents Run(ctx)
cancellation/timeouts from propagating; change the call to pass the incoming
context (ctx) instead. Locate the invocation of patchDesiredUpdate in the accept
flow (the call using patchDesiredUpdate(context.TODO(), acceptRisks,
o.Client.ConfigV1().ClusterVersions(), "version")) and replace the placeholder
context with the Run(ctx) parameter so patchDesiredUpdate receives the caller's
context; ensure any surrounding functions (e.g., Run(ctx)) keep ctx in scope and
thread it through to this call.
| if update != nil { | ||
| update.AcceptRisks = cv.Spec.DesiredUpdate.AcceptRisks | ||
| } |
There was a problem hiding this comment.
Guard cv.Spec.DesiredUpdate before reading AcceptRisks.
At Line [270] and Line [411], dereferencing cv.Spec.DesiredUpdate.AcceptRisks can panic when cv.Spec.DesiredUpdate == nil (valid state for clusters with no desired update set).
Proposed fix
- if update != nil {
- update.AcceptRisks = cv.Spec.DesiredUpdate.AcceptRisks
- }
+ if cv.Spec.DesiredUpdate != nil {
+ update.AcceptRisks = append([]configv1.AcceptRisk(nil), cv.Spec.DesiredUpdate.AcceptRisks...)
+ }
...
- update.AcceptRisks = cv.Spec.DesiredUpdate.AcceptRisks
+ if cv.Spec.DesiredUpdate != nil {
+ update.AcceptRisks = append([]configv1.AcceptRisk(nil), cv.Spec.DesiredUpdate.AcceptRisks...)
+ }Also applies to: 411-411
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cli/admin/upgrade/upgrade.go` around lines 269 - 271, The code
dereferences cv.Spec.DesiredUpdate.AcceptRisks without guarding for nil; update
the logic around the assignment to update.AcceptRisks so you first check that
cv.Spec != nil and cv.Spec.DesiredUpdate != nil before reading AcceptRisks (and
fall back to a safe default, e.g. false, when DesiredUpdate is nil). Apply the
same nil-check fix at the other occurrence referencing
cv.Spec.DesiredUpdate.AcceptRisks (the second instance noted) so both reads are
protected and cannot panic.
$ git --no-pager log --pretty=oneline -1
506aaa41ab10afc01415cf65a4d5e1250953d4c8 (HEAD -> accept-cmd) Stop hiding rollback cmd
$ OC_ENABLE_CMD_UPGRADE_ACCEPT_RISKS=true OC_ENABLE_CMD_UPGRADE_ROLLBACK=true ./oc adm upgrade -h | grep 'Available Commands:' -A 7
Available Commands:
accept Accept risks exposed to conditional updates.
channel Set or clear the update channel
recommend Displays cluster update recommendations.
rollback Rollback the cluster to the previous release.
status Display the status of the current cluster version update or multi-arch migration
Options:
$ OC_ENABLE_CMD_UPGRADE_ROLLBACK=true ./oc adm upgrade -h | grep 'Available Commands:' -A 7
Available Commands:
channel Set or clear the update channel
recommend Displays cluster update recommendations.
rollback Rollback the cluster to the previous release.
status Display the status of the current cluster version update or multi-arch migration
Options:
--allow-explicit-upgrade=false:
$ OC_ENABLE_CMD_UPGRADE_ACCEPT_RISKS=true ./oc adm upgrade -h | grep 'Available Commands:' -A 7
Available Commands:
accept Accept risks exposed to conditional updates.
channel Set or clear the update channel
recommend Displays cluster update recommendations.
status Display the status of the current cluster version update or multi-arch migration
Options:
--allow-explicit-upgrade=false:
$ ./oc adm upgrade -h | grep 'Available Commands:' -A 7
Available Commands:
channel Set or clear the update channel
recommend Displays cluster update recommendations.
status Display the status of the current cluster version update or multi-arch migration
Options:
--allow-explicit-upgrade=false:
Upgrade even if the upgrade target is not listed in the available versions list.
Also did a search: Probable they are $ rg 'Hidden:' -g '!vendor/'
pkg/cli/serviceaccounts/newtoken.go
83: Hidden: true,
pkg/cli/serviceaccounts/create_kubeconfig.go
68: Hidden: true,
pkg/cli/serviceaccounts/subcommand.go
26: Hidden: true,
pkg/cli/serviceaccounts/gettoken.go
63: Hidden: true,
|
|
@hongkailiu: 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. |
4 participants
With OC_ENABLE_CMD_UPGRADE_ACCEPT_RISKS=true, a new command
oc adm upgrade acceptis enabled. It accepts comma-separated risks exposed to an OpenShift release [1].The risks are stored in
clusterversion/version's.specs.desiredUpdate.acceptRisks.[1]. https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html-single/updating_clusters/index#understanding-clusterversion-conditiontypes_understanding-openshift-updates
Summary by CodeRabbit
Release Notes
New Features
Improvements
Chores