Open
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. |
|
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:
WalkthroughAdds a new Cobra subcommand to manage ClusterVersion.spec.desiredUpdate.acceptRisks, wires it behind a feature gate, propagates AcceptRisks into upgrade/cancel flows, adds unit and e2e tests, extends test CLI env var support, tweaks rollback visibility and describer exceptions, and bumps two dependencies. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User as "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 (tokens, --replace/--clear)
CLI->>CLI: parse flags, validate args
CLI->>Client: GET ClusterVersion
Client->>API: GET /clusterversions/{name}
API-->>Client: return ClusterVersion (includes spec.desiredUpdate.acceptRisks)
Client-->>CLI: return cv
CLI->>CLI: compute new acceptRisks (getAcceptRisks)
alt changes required
CLI->>Client: PATCH ClusterVersion (JSON Merge Patch)
Client->>API: PATCH /clusterversions/{name}
API-->>Client: patched cv
Client-->>CLI: success
CLI->>User: print updated acceptRisks
else no changes
CLI->>User: print "no changes"
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)
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
Contributor
|
[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
Member
Author
|
I will wait a bit on |
hongkailiu
force-pushed
the
accept-cmd
branch
6 times, most recently
from
87f10fc to
583aa51
Compare
Member
Author
|
Cluster bot: Testing results with 583aa51: So we showed that the oc/pkg/cli/admin/upgrade/upgrade.go Line 688 in 90d7ae6 |
wking
reviewed
Jan 30, 2026
wking
reviewed
Jan 30, 2026
wking
reviewed
Jan 30, 2026
wking
reviewed
Jan 30, 2026
wking
reviewed
Jan 30, 2026
wking
reviewed
Jan 30, 2026
|
Some invalid behavior need your confirm: |
Member
Author
You found a design flaw. We should use |
|
@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. |
Member
Author
$ git --no-pager log --pretty=oneline -1
ddb58895eb8d4ee9fc6edfa6a5b492139e51e0ec (HEAD -> accept-cmd) Use risk- for removal instead of -risk
$ CGO_CFLAGS="-I/opt/homebrew/opt/heimdal/include" make oc
$ OC_ENABLE_CMD_UPGRADE_ACCEPT_RISKS=true ./oc adm upgrade accept riskA,riskB
info: Accept risks are [riskA, riskB]
$ OC_ENABLE_CMD_UPGRADE_ACCEPT_RISKS=true ./oc adm upgrade accept --clear
info: Accept risks are []
$ OC_ENABLE_CMD_UPGRADE_ACCEPT_RISKS=true ./oc adm upgrade accept --clear
info: Accept risks are not changed
$ OC_ENABLE_CMD_UPGRADE_ACCEPT_RISKS=true ./oc adm upgrade accept riskA,riskB
info: Accept risks are [riskA, riskB]
$ OC_ENABLE_CMD_UPGRADE_ACCEPT_RISKS=true ./oc adm upgrade accept riskA,riskB,riskC
info: Accept risks are [riskA, riskB, riskC]
$ OC_ENABLE_CMD_UPGRADE_ACCEPT_RISKS=true ./oc adm upgrade accept riskA,riskB --replace
info: Accept risks are [riskA, riskB]
$ oc get clusterversion version -o yaml | yq -y .spec
channel: simple
clusterID: 250943dd-e3b5-4010-b554-bf67572256d4
desiredUpdate:
acceptRisks:
- name: riskA
- name: riskB
architecture: ''
force: false
image: quay.io/openshift-release-dev/ocp-release@sha256:00000000000000000000000000000000000000000000000000000000003d5ef1
version: 4.22.1
overrides:
- group: config.openshift.io
kind: ClusterImagePolicy
name: openshift
namespace: ''
unmanaged: true
upstream: https://fauxinnati-fauxinnati.apps.ota-stage.q2z4.p1.openshiftapps.com/api/upgrades_info/graph
$ OC_ENABLE_CMD_UPGRADE_ACCEPT_RISKS=true ./oc adm upgrade accept riskA,riskB-
info: Accept risks are [riskA]
$ oc get clusterversion version -o yaml | yq -y .spec
channel: simple
clusterID: 250943dd-e3b5-4010-b554-bf67572256d4
desiredUpdate:
acceptRisks:
- name: riskA
architecture: ''
force: false
image: quay.io/openshift-release-dev/ocp-release@sha256:00000000000000000000000000000000000000000000000000000000003d5ef1
version: 4.22.1
overrides:
- group: config.openshift.io
kind: ClusterImagePolicy
name: openshift
namespace: ''
unmanaged: true
upstream: https://fauxinnati-fauxinnati.apps.ota-stage.q2z4.p1.openshiftapps.com/api/upgrades_info/graph |
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
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/cli/admin/upgrade/accept/accept.go (2)
154-154:⚠️ Potential issue | 🟠 MajorPropagate
Run(ctx)context to the patch request.Line [154] uses
context.TODO(), so command cancellation/timeouts won’t reach the API patch 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 }#!/bin/bash # Verify there are no remaining patch calls that drop caller context in this file rg -nP 'patchDesiredUpdate\(context\.TODO\(' pkg/cli/admin/upgrade/accept/accept.go🤖 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 154, The patch call uses context.TODO() which drops the caller's Run(ctx) cancellation/timeout; change the call to pass the incoming context (use the Run(ctx) parameter) instead of context.TODO() so the API patch request inherits caller cancellation. Update the invocation of patchDesiredUpdate(...) in accept.go to use the ctx variable from Run(ctx) and ensure any helper wrappers (patchDesiredUpdate) accept and forward that context if not already.
49-49:⚠️ Potential issue | 🟡 MinorFix user-facing typo in help text.
Line [49] says “excepted risks”; this should be “accepted risks”.
✏️ Proposed fix
- Passing --clear removes all existing excepted risks. + Passing --clear removes all existing 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, Update the user-facing help text for the --clear flag: find the help/usage string that currently reads "Passing --clear removes all existing excepted risks." (the --clear flag's description in the accept command) and change "excepted risks" to "accepted risks" so it reads "Passing --clear removes all existing accepted risks." Ensure only the typo in the flag description is changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/cli/admin/upgrade/accept/accept.go`:
- Line 154: The patch call uses context.TODO() which drops the caller's Run(ctx)
cancellation/timeout; change the call to pass the incoming context (use the
Run(ctx) parameter) instead of context.TODO() so the API patch request inherits
caller cancellation. Update the invocation of patchDesiredUpdate(...) in
accept.go to use the ctx variable from Run(ctx) and ensure any helper wrappers
(patchDesiredUpdate) accept and forward that context if not already.
- Line 49: Update the user-facing help text for the --clear flag: find the
help/usage string that currently reads "Passing --clear removes all existing
excepted risks." (the --clear flag's description in the accept command) and
change "excepted risks" to "accepted risks" so it reads "Passing --clear removes
all existing accepted risks." Ensure only the typo in the flag description is
changed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7975d16a-f314-43b5-908f-12d70d662137
📒 Files selected for processing (2)
pkg/cli/admin/upgrade/accept/accept.gopkg/cli/admin/upgrade/rollback/rollback.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cli/admin/upgrade/rollback/rollback.go
hongkailiu
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@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.
♻️ Duplicate comments (3)
pkg/cli/admin/upgrade/upgrade.go (1)
269-271:⚠️ Potential issue | 🔴 CriticalGuard
cv.Spec.DesiredUpdatebefore readingAcceptRisks(panic risk).Both reads dereference
cv.Spec.DesiredUpdatewithout a nil check. Clusters with no desired update can hit a nil-pointer panic.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 reads cv.Spec.DesiredUpdate.AcceptRisks without guarding cv.Spec.DesiredUpdate which can cause a nil-pointer panic; update the logic around the assignment to first check that cv.Spec != nil and cv.Spec.DesiredUpdate != nil before reading AcceptRisks (e.g., only set update.AcceptRisks = cv.Spec.DesiredUpdate.AcceptRisks when cv.Spec.DesiredUpdate != nil), and apply the same nil-check pattern to the other occurrence that reads cv.Spec.DesiredUpdate.AcceptRisks (the second spot noted in the review).pkg/cli/admin/upgrade/accept/accept.go (2)
154-155:⚠️ Potential issue | 🟠 MajorUse
ctxin patch call instead ofcontext.TODO().
Run(ctx)receives caller context, but the patch call discards it. This prevents cancellation/timeout propagation.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 }#!/bin/bash # Verify whether patch call in accept.Run uses context.TODO instead of ctx. rg -n --type=go 'patchDesiredUpdate\(context\.TODO\(\)' pkg/cli/admin/upgrade/accept/accept.go rg -n --type=go 'func \(o \*options\) Run\(ctx context\.Context\)' pkg/cli/admin/upgrade/accept/accept.goExpected result:
- First command returns a match (current issue present).
- Second command confirms
Run(ctx)exists, soctxshould be threaded through.🤖 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 154 - 155, The call to patchDesiredUpdate is using context.TODO() which discards the caller context; change the call in the Run(ctx context.Context) method to pass the received ctx instead of context.TODO() so cancellation/timeouts propagate (update the call to patchDesiredUpdate(ctx, acceptRisks, o.Client.ConfigV1().ClusterVersions(), "version")). Locate the call to patchDesiredUpdate in the options.Run (or accept.Run) function and replace context.TODO() with the ctx parameter.
49-49:⚠️ Potential issue | 🟡 MinorTypo in user-facing help text.
“excepted risks” should be “accepted risks”.
Proposed fix
- Passing --clear removes all existing excepted risks. + Passing --clear removes all existing 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, There's a typo in the user-facing help text in accept.go: change the phrase "excepted risks" to "accepted risks" in the help/description string associated with the --clear flag (look for the string "Passing --clear removes all existing excepted risks" inside accept.go, likely in the command description or flag help for the Accept command or NewAcceptCmd); update that literal to "Passing --clear removes all existing accepted risks".
🧹 Nitpick comments (1)
pkg/helpers/describe/describer_test.go (1)
136-136: Please document why this group-version is exempted.Adding a new entry to
MissingDescriberGroupCoverageExceptionsbroadens the test allowlist. Add a short inline reason (or issue link) next to this entry so future reviewers know this is intentional.Proposed tweak
- {Group: "apiextensions.openshift.io", Version: "v1alpha1"}, + {Group: "apiextensions.openshift.io", Version: "v1alpha1"}, // intentional temporary exception; see <issue/link>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helpers/describe/describer_test.go` at line 136, The test allowlist entry {Group: "apiextensions.openshift.io", Version: "v1alpha1"} in MissingDescriberGroupCoverageExceptions needs an inline comment explaining why it's exempted; update the test by adding a brief justification (e.g., "exempt due to X reason" or an issue/PR link) directly next to that entry so future reviewers understand the intentional omission and can reassess it later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/cli/admin/upgrade/accept/accept.go`:
- Around line 154-155: The call to patchDesiredUpdate is using context.TODO()
which discards the caller context; change the call in the Run(ctx
context.Context) method to pass the received ctx instead of context.TODO() so
cancellation/timeouts propagate (update the call to patchDesiredUpdate(ctx,
acceptRisks, o.Client.ConfigV1().ClusterVersions(), "version")). Locate the call
to patchDesiredUpdate in the options.Run (or accept.Run) function and replace
context.TODO() with the ctx parameter.
- Line 49: There's a typo in the user-facing help text in accept.go: change the
phrase "excepted risks" to "accepted risks" in the help/description string
associated with the --clear flag (look for the string "Passing --clear removes
all existing excepted risks" inside accept.go, likely in the command description
or flag help for the Accept command or NewAcceptCmd); update that literal to
"Passing --clear removes all existing accepted risks".
In `@pkg/cli/admin/upgrade/upgrade.go`:
- Around line 269-271: The code reads cv.Spec.DesiredUpdate.AcceptRisks without
guarding cv.Spec.DesiredUpdate which can cause a nil-pointer panic; update the
logic around the assignment to first check that cv.Spec != nil and
cv.Spec.DesiredUpdate != nil before reading AcceptRisks (e.g., only set
update.AcceptRisks = cv.Spec.DesiredUpdate.AcceptRisks when
cv.Spec.DesiredUpdate != nil), and apply the same nil-check pattern to the other
occurrence that reads cv.Spec.DesiredUpdate.AcceptRisks (the second spot noted
in the review).
---
Nitpick comments:
In `@pkg/helpers/describe/describer_test.go`:
- Line 136: The test allowlist entry {Group: "apiextensions.openshift.io",
Version: "v1alpha1"} in MissingDescriberGroupCoverageExceptions needs an inline
comment explaining why it's exempted; update the test by adding a brief
justification (e.g., "exempt due to X reason" or an issue/PR link) directly next
to that entry so future reviewers understand the intentional omission and can
reassess it later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76cca185-dc7d-432e-bb5c-1d7c68b00c93
⛔ Files ignored due to path filters (105)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.ci-operator.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/.coderabbit.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/AGENTS.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Dockerfile.ocpis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_image_policy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_feature.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_image_policy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_node.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_scheduling.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_image_policy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_image_policy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/console/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types_machineconfignode.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_internalreleaseimage.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_osimagestream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_pinnedimageset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/awsplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/azureplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/baremetalplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/clusterimagepolicyspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gcpplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gcpserviceendpoint.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/imagepolicyfulciocawithrekorrootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/imagepolicypkirootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/imagepolicypublickeyrootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/imagepolicyspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/imagesigstoreverificationpolicy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/nutanixplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/openstackplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/ovirtplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/policyrootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vsphereplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clusterimagepolicyspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagepolicyfulciocawithrekorrootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagepolicypkirootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagepolicypublickeyrootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagepolicyspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/imagesigstoreverificationpolicy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/policyrootoftrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfignodestatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfignodestatusinternalreleaseimage.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfignodestatusinternalreleaseimageref.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/osimagestreamreference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/internalreleaseimage.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/internalreleaseimagebundlestatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/internalreleaseimageref.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/internalreleaseimagespec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/internalreleaseimagestatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/osimagestream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/osimagestreamset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/osimagestreamstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/pinnedimageref.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/clientset/versioned/typed/machineconfiguration/v1alpha1/generated_expansion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/clientset/versioned/typed/machineconfiguration/v1alpha1/internalreleaseimage.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/clientset/versioned/typed/machineconfiguration/v1alpha1/machineconfiguration_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/clientset/versioned/typed/machineconfiguration/v1alpha1/osimagestream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/ingresscontrollerspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/ingresscontrollertuningoptions.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 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
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- pkg/cli/admin/upgrade/accept/accept_test.go
JianLi-RH
reviewed
Mar 5, 2026
|
@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: 3
🧹 Nitpick comments (1)
test/e2e/util.go (1)
153-159: SnapshotaddEnvVarsinRun()to prevent mutation leaks across commands.
addEnvVarsis stored by reference inCLICommand. Later calls toEnvVar()on the CLI instance mutate the map, affecting all previously createdCLICommandinstances that share the reference.♻️ Proposed fix
+func copyEnvVars(in map[string]string) map[string]string { + if in == nil { + return nil + } + out := make(map[string]string, len(in)) + for k, v := range in { + out[k] = v + } + return out +} + func (c *CLI) Run(verb string) *CLICommand { return &CLICommand{ cli: c, verb: verb, args: []string{}, - addEnvVars: c.addEnvVars, + addEnvVars: copyEnvVars(c.addEnvVars), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/util.go` around lines 153 - 159, The Run method is storing CLI.addEnvVars by reference into the new CLICommand (causing later CLI.EnvVar mutations to leak into existing commands); update CLI.Run to snapshot/copy the addEnvVars map when creating the CLICommand so each command gets its own map copy (shallow copy of map[string]string) and continue using that copy in CLICommand; reference the CLI.Run function, the CLICommand struct and the addEnvVars/EnvVar symbols when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/accept.go`:
- Around line 25-47: BeforeEach/AfterEach currently force AcceptRisks to
empty/nil, which can overwrite pre-existing cluster state; change the setup to
capture and preserve the original state and restore it on teardown. In
BeforeEach (function BeforeEach) read cv via configClient.ClusterVersions().Get
and store a copy of cv.Spec.DesiredUpdate.AcceptRisks (e.g., originalAcceptRisks
or originalCV variable) including handling nil DesiredUpdate, then assert
without mutating the cluster. In AfterEach, fetch cv again, create a DeepCopy,
set backup.Spec.DesiredUpdate.AcceptRisks back to the saved originalAcceptRisks
(restoring nil vs non-nil exactly), and Update via
configClient.ClusterVersions().Update to restore the prior value instead of
always setting nil/empty. Ensure the preserved variable is accessible to both
BeforeEach and AfterEach and handle cases where DesiredUpdate is nil when
restoring.
- Line 49: The test uses g.It("can operate accept risks [Serial]", ...) but
lacks the g.Serial decorator, so it can run in parallel and interfere with the
BeforeEach/AfterEach that mutate ClusterVersion.Spec.DesiredUpdate.AcceptRisks;
update the g.It invocation to include g.Serial as a decorator (e.g., pass
g.Serial alongside g.Label("tech-preview")) so the test is flagged serial;
ensure the modified g.It still wraps the same func and that BeforeEach/AfterEach
and any references to ClusterVersion remain unchanged.
In `@test/e2e/util.go`:
- Around line 145-159: The environment construction for CLI commands is
inconsistent: CLICommand.Run initializes addEnvVars but Output() applies them
while Outputs() and Background() do not, and Output() can clobber the inherited
environment by appending to a nil execCmd.Env when kubeconfig is empty. Fix by
centralizing env building (e.g., a helper used by Output(), Outputs(), and
Background()) so addEnvVars is always applied; when building execCmd.Env, start
from os.Environ() (or existing execCmd.Env if non-nil) and then append the
addEnvVars entries (respecting kubeconfig handling) to avoid dropping parent
env. Ensure changes reference CLICommand, Run(), Output(), Outputs(),
Background(), and execCmd.Env/kubeconfig so reviewers can locate the updates.
---
Nitpick comments:
In `@test/e2e/util.go`:
- Around line 153-159: The Run method is storing CLI.addEnvVars by reference
into the new CLICommand (causing later CLI.EnvVar mutations to leak into
existing commands); update CLI.Run to snapshot/copy the addEnvVars map when
creating the CLICommand so each command gets its own map copy (shallow copy of
map[string]string) and continue using that copy in CLICommand; reference the
CLI.Run function, the CLICommand struct and the addEnvVars/EnvVar symbols when
making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9297b7a4-ba5f-4c19-9805-5baff6250208
📒 Files selected for processing (2)
test/e2e/accept.gotest/e2e/util.go
Contributor
|
@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. |
Member
|
#2219 is working through the Kube 1.35 and OCP-API vendor bump that's blocking us here :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
New Features
Improvements
Tests
Chores
Style