[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 Use the checkbox below for a quick retry:
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
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
| 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.
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 |
|
@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. |
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