[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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new hidden Cobra subcommand is added to manage conditional update risks for clusters, supporting addition, removal, and replacement of accepted risk names via flags. The command is conditionally wired into the upgrade command tree via a feature gate, with no changes to existing public APIs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
hongkailiu
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu 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
![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
| for _, s := range strings.Split(args[0], ",") { | ||
| trimmed := strings.TrimSpace(s) | ||
| if strings.HasPrefix(trimmed, "-") { | ||
| o.minus.Insert(trimmed[1:]) | ||
| } else { | ||
| o.plus.Insert(trimmed) | ||
| } | ||
| } |
There was a problem hiding this comment.
Edge case: empty risk name when input is just "-".
If a user passes "-," or just "-" within the comma-separated list, trimmed[1:] yields an empty string that gets inserted into the minus set. Consider validating that risk names are non-empty after trimming.
🤖 Prompt for AI Agents
In pkg/cli/admin/upgrade/accept/accept.go around lines 92 to 99, the loop
inserts an empty string into o.minus when an item is "-" (or "-,") because
trimmed[1:] can be empty; change the logic to validate names are non-empty after
trimming (and after removing the leading '-') and skip or return an error when
an empty risk name is encountered: compute name := strings.TrimSpace(...) then
if strings.HasPrefix(name, "-") { name = name[1:]; } if name == "" { return
fmt.Errorf("empty risk name in '%s' input", args[0]) } then insert into the
appropriate set. Ensure you handle both plus and minus cases consistently and
propagate the error to the caller.
| // TODO remove this testing code | ||
| o.Client = fake.NewClientset(&configv1.ClusterVersion{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "version", | ||
| }, | ||
| }).ConfigV1().ClusterVersions() | ||
| return nil |
There was a problem hiding this comment.
Remove debug/test code before merging.
This fake client assignment completely overrides the real client created on lines 110-114, making the command non-functional against real clusters. The fake package import on line 20 should also be removed.
🤖 Prompt for AI Agents
In pkg/cli/admin/upgrade/accept/accept.go around lines 116 to 122, remove the
temporary testing code that assigns o.Client =
fake.NewClientset(...).ConfigV1().ClusterVersions() and the following return nil
so the real client created on lines 110-114 is used; also remove the unused
"fake" package import at line 20. Ensure no lingering TODO comment remains and
that error handling/return flow falls through to use the actual client created
earlier.
| if err != nil { | ||
| return fmt.Errorf("marshal ClusterVersion patch: %v", err) | ||
| } | ||
| patch := []byte(fmt.Sprintf(`{"spec":{"desiredUpdate.acceptRisks": %s}}`, acceptRisksJSON)) |
There was a problem hiding this comment.
Incorrect JSON merge patch path.
The patch creates a literal key "desiredUpdate.acceptRisks" instead of the nested path spec.desiredUpdate.acceptRisks. The ClusterVersion will not be updated correctly.
🔎 Proposed fix
- patch := []byte(fmt.Sprintf(`{"spec":{"desiredUpdate.acceptRisks": %s}}`, acceptRisksJSON))
+ patch := []byte(fmt.Sprintf(`{"spec":{"desiredUpdate":{"acceptRisks": %s}}}`, acceptRisksJSON))📝 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.
| patch := []byte(fmt.Sprintf(`{"spec":{"desiredUpdate.acceptRisks": %s}}`, acceptRisksJSON)) | |
| patch := []byte(fmt.Sprintf(`{"spec":{"desiredUpdate":{"acceptRisks": %s}}}`, acceptRisksJSON)) |
🤖 Prompt for AI Agents
In pkg/cli/admin/upgrade/accept/accept.go around line 165, the current patch
uses a dotted key which produces a literal "desiredUpdate.acceptRisks" instead
of nesting under spec.desiredUpdate; change the patch payload to nest objects so
the JSON is {"spec":{"desiredUpdate":{"acceptRisks": <acceptRisksJSON>}}}
(ensure acceptRisksJSON is inserted raw, not quoted), or build the patch using a
small map/struct and marshal it to JSON to produce the correct nested path.
42ccc9a to
b4dc31c
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
b4dc31c to
1efbc2c
Compare
|
@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. |
2 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