-
Notifications
You must be signed in to change notification settings - Fork 422
Bug 2077332: pkg/cli/admin/upgrade: Use PATCH instead of POST for spec updates #1114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 2077332: pkg/cli/admin/upgrade: Use PATCH instead of POST for spec updates #1114
Conversation
…update targets When folks use --to-image and --allow-explicit-upgrade together, oc searches through recommended (and, with --allow-explicit-upgrade, also supported but not recommended) targets to see if it knew about the requested target image. Until this commit, it would throw out that information, create an Update that just pointed at the target release image, and warn the user about the risks of --allow-explicit-upgrade. With this commit, --allow-explicit-upgrade is consumed conditionally. If we need it, because the earlier image search was unable to turn up the requested release in ClusterVersion status, we'll still create an Update and warn about the risks of --allow-explicit-upgrade. But if the search found the requested image (update != nil), we just use that as if the user had requested it with --to, and we no longer pester the user with complaints about the --allow-explicit-upgrade that they used but, in this case, didn't need.
… together 6c8d935 (Adding the flag --allow-not-recommended to oc adm upgrade, 2021-12-01, openshift#986) taught --to and --to-image about --allow-not-recommended. This commit extends that coverage to --to-latest, allowing folks to say "I want the latest supported update, even if that update is not currently recommended". Probably not the best idea to do that sort of thing, but that's generally true of --allow-not-recommended. To accomplish this change, I've consolidated into a single "user requested an update" case, where we use one of the three options (--to-latest, --to, or --to-image) to hunt for a target update, and then centralized logic to actually apply that target.
ClusterVersion's spec can evolve over time, growing additional properties like capabilities [1]. With the old POST/Update(...) approach, that could lead to older oc (who didn't know about the new properties) naively clearing them when they washed the in-cluster resource through their local ClusterVersion Go type. With this commit, we transition to PATCH, so we can touch just spec.desiredUpdate, regardless of what else is going on in spec. [1]: openshift/api@5b82635
|
@openshift-cherrypick-robot: Bugzilla bug 2075647 has been cloned as Bugzilla bug 2077332. Retitling PR to link against new bug. 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 kubernetes/test-infra repository. |
|
@openshift-cherrypick-robot: This pull request references Bugzilla bug 2077332, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 6 validation(s) were run on this bug
Requesting review from QA contact: 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 kubernetes/test-infra repository. |
|
No need for /retitle Bug 2077332: pkg/cli/admin/upgrade: Use PATCH instead of POST for spec updates |
wking
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
8 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@openshift-cherrypick-robot: The following test 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/test-infra repository. I understand the commands that are listed here. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
soltysh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/label backport-risk-assessed
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: openshift-cherrypick-robot, soltysh, 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 |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
13 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/label cherry-pick-approved |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@openshift-cherrypick-robot: All pull requests linked via external trackers have merged: Bugzilla bug 2077332 has been moved to the MODIFIED state. 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 kubernetes/test-infra repository. |
This is an automated cherry-pick of #1111
/assign wking