Bug 2075647: pkg/cli/admin/upgrade: Use PATCH instead of POST for spec updates #1111
Conversation
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
openshift-ci
bot
added
the
approved
…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.
wking
force-pushed
the
patch-cluster-version-updates
branch
2 times, most recently
from
c8df16e to
f2f68e3
Compare
wking
changed the title
openshift-ci
bot
added
the
bugzilla/severity-unspecified
Contributor
|
@wking: This pull request references Bugzilla bug 2075647, which is invalid:
Comment 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-ci
bot
added
the
bugzilla/invalid-bug
Member
Author
|
/bugzilla refresh |
openshift-ci
bot
added
bugzilla/severity-medium
Contributor
|
@wking: This pull request references Bugzilla bug 2075647, 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. 3 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. |
… 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
wking
force-pushed
the
patch-cluster-version-updates
branch
from
f2f68e3 to
123fa58
Compare
Member
Author
|
|
Contributor
|
/lgtm |
Contributor
|
/retest |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jottofar, 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 |
Member
Author
|
|
Contributor
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
Contributor
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
Contributor
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
Contributor
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
4 similar comments
Contributor
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
Contributor
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
Contributor
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
Contributor
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
Contributor
|
@wking: 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. |
Contributor
|
@wking: All pull requests linked via external trackers have merged: Bugzilla bug 2075647 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. |
Member
Author
|
/cherrypick release-4.10 |
|
@wking: new pull request created: #1114 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. |
wking
added a commit
to wking/oc
that referenced
this pull request
Feb 20, 2023
…ates Like 123fa58 (pkg/cli/admin/upgrade: Use PATCH instead of POST for spec updates, 2022-04-14, openshift#1111), but for the 'channel' subcommand. 123fa58 described the general risk of using POST/Update when the local 'oc' client may not be aware of some new spec properties. For the 'channel' subcommand specifically, it was added in 4.9: $ git ls-tree -r origin/release-4.8 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob e2418c1 pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go $ git ls-tree -r origin/release-4.9 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob b427176 pkg/cli/admin/upgrade/channel/channel.go 100644 blob 2bbf9da pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go And between 4.9 and the present, ClusterVersion spec grep the 'capabilities' property: $ git diff origin/release-4.9..origin/master -- vendor/github.com/openshift/api/config/v1/types_cluster_version.go | grep -v // | grep -A10 Spec @@ -45,8 +45,17 @@ type ClusterVersionSpec struct { @@ -68,6 +77,12 @@ type ClusterVersionSpec struct { Channel string `json:"channel,omitempty"` + Capabilities *ClusterVersionCapabilitiesSpec `json:"capabilities,omitempty"` + @@ -113,6 +128,9 @@ type ClusterVersionStatus struct { ... That property landed in 4.11: $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.10 vendor/github.com/openshift/api/config/v1/types_cluster_version.go ...no hits... $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.11 vendor/github.com/openshift/api/config/v1/types_cluster_version.go origin/release-4.11:vendor/github.com/openshift/api/config/v1/types_cluster_version.go:3 So I suspect 4.9 and 4.10 oc calls to 'oc adm upgrade channel ...' for 4.11+ clusters would clear spec.capabilities. Not all that many clusters try to restrict capabilities, but folks will need to bump their channel for at least every other minor (if their using EUS channels), and while we recommend folks use an oc from the 4.y they're heading towards, we don't have anything in place to enforce that.
wking
added a commit
to wking/oc
that referenced
this pull request
Feb 20, 2023
…ates Like 123fa58 (pkg/cli/admin/upgrade: Use PATCH instead of POST for spec updates, 2022-04-14, openshift#1111), but for the 'channel' subcommand. 123fa58 described the general risk of using POST/Update when the local 'oc' client may not be aware of some new spec properties. For the 'channel' subcommand specifically, it was added in 4.9: $ git ls-tree -r origin/release-4.8 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob e2418c1 pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go $ git ls-tree -r origin/release-4.9 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob b427176 pkg/cli/admin/upgrade/channel/channel.go 100644 blob 2bbf9da pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go And between 4.9 and the present, ClusterVersion spec grew the 'capabilities' property: $ git diff origin/release-4.9..origin/master -- vendor/github.com/openshift/api/config/v1/types_cluster_version.go | grep -v // | grep -A10 Spec @@ -45,8 +45,17 @@ type ClusterVersionSpec struct { @@ -68,6 +77,12 @@ type ClusterVersionSpec struct { Channel string `json:"channel,omitempty"` + Capabilities *ClusterVersionCapabilitiesSpec `json:"capabilities,omitempty"` + @@ -113,6 +128,9 @@ type ClusterVersionStatus struct { ... That property landed in 4.11: $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.10 vendor/github.com/openshift/api/config/v1/types_cluster_version.go ...no hits... $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.11 vendor/github.com/openshift/api/config/v1/types_cluster_version.go origin/release-4.11:vendor/github.com/openshift/api/config/v1/types_cluster_version.go:3 So I suspect 4.9 and 4.10 oc calls to 'oc adm upgrade channel ...' for 4.11+ clusters would clear spec.capabilities. Not all that many clusters try to restrict capabilities, but folks will need to bump their channel for at least every other minor (if their using EUS channels), and while we recommend folks use an oc from the 4.y they're heading towards, we don't have anything in place to enforce that.
wking
added a commit
to wking/oc
that referenced
this pull request
Feb 20, 2023
…ates Like 123fa58 (pkg/cli/admin/upgrade: Use PATCH instead of POST for spec updates, 2022-04-14, openshift#1111), but for the 'channel' subcommand. 123fa58 described the general risk of using POST/Update when the local 'oc' client may not be aware of some new spec properties. For the 'channel' subcommand specifically, it was added in 4.9: $ git ls-tree -r origin/release-4.8 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob e2418c1 pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go $ git ls-tree -r origin/release-4.9 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob b427176 pkg/cli/admin/upgrade/channel/channel.go 100644 blob 2bbf9da pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go And between 4.9 and the present, ClusterVersion spec grew the 'capabilities' property: $ git diff origin/release-4.9..origin/master -- vendor/github.com/openshift/api/config/v1/types_cluster_version.go | grep -v // | grep -A10 Spec @@ -45,8 +45,17 @@ type ClusterVersionSpec struct { @@ -68,6 +77,12 @@ type ClusterVersionSpec struct { Channel string `json:"channel,omitempty"` + Capabilities *ClusterVersionCapabilitiesSpec `json:"capabilities,omitempty"` + @@ -113,6 +128,9 @@ type ClusterVersionStatus struct { ... That property landed in 4.11: $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.10 vendor/github.com/openshift/api/config/v1/types_cluster_version.go ...no hits... $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.11 vendor/github.com/openshift/api/config/v1/types_cluster_version.go origin/release-4.11:vendor/github.com/openshift/api/config/v1/types_cluster_version.go:3 So I suspect 4.9 and 4.10 oc calls to 'oc adm upgrade channel ...' for 4.11+ clusters would clear spec.capabilities. Not all that many clusters try to restrict capabilities, but folks will need to bump their channel for at least every other minor (if their using EUS channels), and while we recommend folks use an oc from the 4.y they're heading towards, we don't have anything in place to enforce that.
wking
added a commit
to wking/oc
that referenced
this pull request
Feb 20, 2023
…ates Like 123fa58 (pkg/cli/admin/upgrade: Use PATCH instead of POST for spec updates, 2022-04-14, openshift#1111), but for the 'channel' subcommand. 123fa58 described the general risk of using POST/Update when the local 'oc' client may not be aware of some new spec properties. For the 'channel' subcommand specifically, it was added in 4.9: $ git ls-tree -r origin/release-4.8 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob e2418c1 pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go $ git ls-tree -r origin/release-4.9 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob b427176 pkg/cli/admin/upgrade/channel/channel.go 100644 blob 2bbf9da pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go And between 4.9 and the present, ClusterVersion spec grew the 'capabilities' property: $ git diff origin/release-4.9..origin/master -- vendor/github.com/openshift/api/config/v1/types_cluster_version.go | grep -v // | grep -A10 Spec @@ -45,8 +45,17 @@ type ClusterVersionSpec struct { @@ -68,6 +77,12 @@ type ClusterVersionSpec struct { Channel string `json:"channel,omitempty"` + Capabilities *ClusterVersionCapabilitiesSpec `json:"capabilities,omitempty"` + @@ -113,6 +128,9 @@ type ClusterVersionStatus struct { ... That property landed in 4.11: $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.10 vendor/github.com/openshift/api/config/v1/types_cluster_version.go ...no hits... $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.11 vendor/github.com/openshift/api/config/v1/types_cluster_version.go origin/release-4.11:vendor/github.com/openshift/api/config/v1/types_cluster_version.go:3 So I suspect 4.9 and 4.10 oc calls to 'oc adm upgrade channel ...' for 4.11+ clusters would clear spec.capabilities. Not all that many clusters try to restrict capabilities, but folks will need to bump their channel for at least every other minor (if their using EUS channels), and while we recommend folks use an oc from the 4.y they're heading towards, we don't have anything in place to enforce that.
wking
added a commit
to wking/oc
that referenced
this pull request
Feb 20, 2023
…ates Like 123fa58 (pkg/cli/admin/upgrade: Use PATCH instead of POST for spec updates, 2022-04-14, openshift#1111), but for the 'channel' subcommand. 123fa58 described the general risk of using POST/Update when the local 'oc' client may not be aware of some new spec properties. For the 'channel' subcommand specifically, it was added in 4.9: $ git ls-tree -r origin/release-4.8 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob e2418c1 pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go $ git ls-tree -r origin/release-4.9 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob b427176 pkg/cli/admin/upgrade/channel/channel.go 100644 blob 2bbf9da pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go And between 4.9 and the present, ClusterVersion spec grew the 'capabilities' property: $ git diff origin/release-4.9..origin/master -- vendor/github.com/openshift/api/config/v1/types_cluster_version.go | grep -v // | grep -A10 Spec @@ -45,8 +45,17 @@ type ClusterVersionSpec struct { @@ -68,6 +77,12 @@ type ClusterVersionSpec struct { Channel string `json:"channel,omitempty"` + Capabilities *ClusterVersionCapabilitiesSpec `json:"capabilities,omitempty"` + @@ -113,6 +128,9 @@ type ClusterVersionStatus struct { ... That property landed in 4.11: $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.10 vendor/github.com/openshift/api/config/v1/types_cluster_version.go ...no hits... $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.11 vendor/github.com/openshift/api/config/v1/types_cluster_version.go origin/release-4.11:vendor/github.com/openshift/api/config/v1/types_cluster_version.go:3 So I suspect 4.9 and 4.10 oc calls to 'oc adm upgrade channel ...' for 4.11+ clusters would clear spec.capabilities. Not all that many clusters try to restrict capabilities, but folks will need to bump their channel for at least every other minor (if their using EUS channels), and while we recommend folks use an oc from the 4.y they're heading towards, we don't have anything in place to enforce that.
wking
added a commit
to wking/oc
that referenced
this pull request
Feb 20, 2023
…ates Like 123fa58 (pkg/cli/admin/upgrade: Use PATCH instead of POST for spec updates, 2022-04-14, openshift#1111), but for the 'channel' subcommand. 123fa58 described the general risk of using POST/Update when the local 'oc' client may not be aware of some new spec properties. For the 'channel' subcommand specifically, it was added in 4.9: $ git ls-tree -r origin/release-4.8 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob e2418c1 pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go $ git ls-tree -r origin/release-4.9 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob b427176 pkg/cli/admin/upgrade/channel/channel.go 100644 blob 2bbf9da pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go And between 4.9 and the present, ClusterVersion spec grew the 'capabilities' property: $ git diff origin/release-4.9..origin/master -- vendor/github.com/openshift/api/config/v1/types_cluster_version.go | grep -v // | grep -A10 Spec @@ -45,8 +45,17 @@ type ClusterVersionSpec struct { @@ -68,6 +77,12 @@ type ClusterVersionSpec struct { Channel string `json:"channel,omitempty"` + Capabilities *ClusterVersionCapabilitiesSpec `json:"capabilities,omitempty"` + @@ -113,6 +128,9 @@ type ClusterVersionStatus struct { ... That property landed in 4.11: $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.10 vendor/github.com/openshift/api/config/v1/types_cluster_version.go ...no hits... $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.11 vendor/github.com/openshift/api/config/v1/types_cluster_version.go origin/release-4.11:vendor/github.com/openshift/api/config/v1/types_cluster_version.go:3 So I suspect 4.9 and 4.10 oc calls to 'oc adm upgrade channel ...' for 4.11+ clusters would clear spec.capabilities. Not all that many clusters try to restrict capabilities, but folks will need to bump their channel for at least every other minor (if their using EUS channels), and while we recommend folks use an oc from the 4.y they're heading towards, we don't have anything in place to enforce that.
openshift-cherrypick-robot
pushed a commit
to openshift-cherrypick-robot/oc
that referenced
this pull request
Feb 24, 2023
…ates Like 123fa58 (pkg/cli/admin/upgrade: Use PATCH instead of POST for spec updates, 2022-04-14, openshift#1111), but for the 'channel' subcommand. 123fa58 described the general risk of using POST/Update when the local 'oc' client may not be aware of some new spec properties. For the 'channel' subcommand specifically, it was added in 4.9: $ git ls-tree -r origin/release-4.8 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob e2418c1 pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go $ git ls-tree -r origin/release-4.9 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob b427176 pkg/cli/admin/upgrade/channel/channel.go 100644 blob 2bbf9da pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go And between 4.9 and the present, ClusterVersion spec grew the 'capabilities' property: $ git diff origin/release-4.9..origin/master -- vendor/github.com/openshift/api/config/v1/types_cluster_version.go | grep -v // | grep -A10 Spec @@ -45,8 +45,17 @@ type ClusterVersionSpec struct { @@ -68,6 +77,12 @@ type ClusterVersionSpec struct { Channel string `json:"channel,omitempty"` + Capabilities *ClusterVersionCapabilitiesSpec `json:"capabilities,omitempty"` + @@ -113,6 +128,9 @@ type ClusterVersionStatus struct { ... That property landed in 4.11: $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.10 vendor/github.com/openshift/api/config/v1/types_cluster_version.go ...no hits... $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.11 vendor/github.com/openshift/api/config/v1/types_cluster_version.go origin/release-4.11:vendor/github.com/openshift/api/config/v1/types_cluster_version.go:3 So I suspect 4.9 and 4.10 oc calls to 'oc adm upgrade channel ...' for 4.11+ clusters would clear spec.capabilities. Not all that many clusters try to restrict capabilities, but folks will need to bump their channel for at least every other minor (if their using EUS channels), and while we recommend folks use an oc from the 4.y they're heading towards, we don't have anything in place to enforce that.
Jamstah
pushed a commit
to Jamstah/oc
that referenced
this pull request
Feb 27, 2023
…ates Like 123fa58 (pkg/cli/admin/upgrade: Use PATCH instead of POST for spec updates, 2022-04-14, openshift#1111), but for the 'channel' subcommand. 123fa58 described the general risk of using POST/Update when the local 'oc' client may not be aware of some new spec properties. For the 'channel' subcommand specifically, it was added in 4.9: $ git ls-tree -r origin/release-4.8 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob e2418c1 pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go $ git ls-tree -r origin/release-4.9 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob b427176 pkg/cli/admin/upgrade/channel/channel.go 100644 blob 2bbf9da pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go And between 4.9 and the present, ClusterVersion spec grew the 'capabilities' property: $ git diff origin/release-4.9..origin/master -- vendor/github.com/openshift/api/config/v1/types_cluster_version.go | grep -v // | grep -A10 Spec @@ -45,8 +45,17 @@ type ClusterVersionSpec struct { @@ -68,6 +77,12 @@ type ClusterVersionSpec struct { Channel string `json:"channel,omitempty"` + Capabilities *ClusterVersionCapabilitiesSpec `json:"capabilities,omitempty"` + @@ -113,6 +128,9 @@ type ClusterVersionStatus struct { ... That property landed in 4.11: $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.10 vendor/github.com/openshift/api/config/v1/types_cluster_version.go ...no hits... $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.11 vendor/github.com/openshift/api/config/v1/types_cluster_version.go origin/release-4.11:vendor/github.com/openshift/api/config/v1/types_cluster_version.go:3 So I suspect 4.9 and 4.10 oc calls to 'oc adm upgrade channel ...' for 4.11+ clusters would clear spec.capabilities. Not all that many clusters try to restrict capabilities, but folks will need to bump their channel for at least every other minor (if their using EUS channels), and while we recommend folks use an oc from the 4.y they're heading towards, we don't have anything in place to enforce that.
openshift-cherrypick-robot
pushed a commit
to openshift-cherrypick-robot/oc
that referenced
this pull request
Mar 2, 2023
…ates Like 123fa58 (pkg/cli/admin/upgrade: Use PATCH instead of POST for spec updates, 2022-04-14, openshift#1111), but for the 'channel' subcommand. 123fa58 described the general risk of using POST/Update when the local 'oc' client may not be aware of some new spec properties. For the 'channel' subcommand specifically, it was added in 4.9: $ git ls-tree -r origin/release-4.8 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob e2418c1 pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go $ git ls-tree -r origin/release-4.9 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob b427176 pkg/cli/admin/upgrade/channel/channel.go 100644 blob 2bbf9da pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go And between 4.9 and the present, ClusterVersion spec grew the 'capabilities' property: $ git diff origin/release-4.9..origin/master -- vendor/github.com/openshift/api/config/v1/types_cluster_version.go | grep -v // | grep -A10 Spec @@ -45,8 +45,17 @@ type ClusterVersionSpec struct { @@ -68,6 +77,12 @@ type ClusterVersionSpec struct { Channel string `json:"channel,omitempty"` + Capabilities *ClusterVersionCapabilitiesSpec `json:"capabilities,omitempty"` + @@ -113,6 +128,9 @@ type ClusterVersionStatus struct { ... That property landed in 4.11: $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.10 vendor/github.com/openshift/api/config/v1/types_cluster_version.go ...no hits... $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.11 vendor/github.com/openshift/api/config/v1/types_cluster_version.go origin/release-4.11:vendor/github.com/openshift/api/config/v1/types_cluster_version.go:3 So I suspect 4.9 and 4.10 oc calls to 'oc adm upgrade channel ...' for 4.11+ clusters would clear spec.capabilities. Not all that many clusters try to restrict capabilities, but folks will need to bump their channel for at least every other minor (if their using EUS channels), and while we recommend folks use an oc from the 4.y they're heading towards, we don't have anything in place to enforce that.
openshift-cherrypick-robot
pushed a commit
to openshift-cherrypick-robot/oc
that referenced
this pull request
Mar 23, 2023
…ates Like 123fa58 (pkg/cli/admin/upgrade: Use PATCH instead of POST for spec updates, 2022-04-14, openshift#1111), but for the 'channel' subcommand. 123fa58 described the general risk of using POST/Update when the local 'oc' client may not be aware of some new spec properties. For the 'channel' subcommand specifically, it was added in 4.9: $ git ls-tree -r origin/release-4.8 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob e2418c1 pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go $ git ls-tree -r origin/release-4.9 -- pkg/cli/admin/upgrade 100644 blob 578fa80 pkg/cli/admin/upgrade/OWNERS 100644 blob b427176 pkg/cli/admin/upgrade/channel/channel.go 100644 blob 2bbf9da pkg/cli/admin/upgrade/upgrade.go 100644 blob 581af58 pkg/cli/admin/upgrade/upgrade_test.go And between 4.9 and the present, ClusterVersion spec grew the 'capabilities' property: $ git diff origin/release-4.9..origin/master -- vendor/github.com/openshift/api/config/v1/types_cluster_version.go | grep -v // | grep -A10 Spec @@ -45,8 +45,17 @@ type ClusterVersionSpec struct { @@ -68,6 +77,12 @@ type ClusterVersionSpec struct { Channel string `json:"channel,omitempty"` + Capabilities *ClusterVersionCapabilitiesSpec `json:"capabilities,omitempty"` + @@ -113,6 +128,9 @@ type ClusterVersionStatus struct { ... That property landed in 4.11: $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.10 vendor/github.com/openshift/api/config/v1/types_cluster_version.go ...no hits... $ git --no-pager grep -c ClusterVersionCapabilitiesSpec origin/release-4.11 vendor/github.com/openshift/api/config/v1/types_cluster_version.go origin/release-4.11:vendor/github.com/openshift/api/config/v1/types_cluster_version.go:3 So I suspect 4.9 and 4.10 oc calls to 'oc adm upgrade channel ...' for 4.11+ clusters would clear spec.capabilities. Not all that many clusters try to restrict capabilities, but folks will need to bump their channel for at least every other minor (if their using EUS channels), and while we recommend folks use an oc from the 4.y they're heading towards, we don't have anything in place to enforce that.
4 tasks
wking
added a commit
to wking/oc
that referenced
this pull request
Aug 29, 2025
…mmits Originally, the change-log generation focused on merge commits, which are common in many repositories. For example, in this 'oc' repository: $ git log --graph --oneline -10 * 559c67e (HEAD -> main, origin/main) Merge pull request openshift#2083 from wking/oc-adm-upgrade-recommend-certificate-authority |\ | * b98b2e1 pkg/cli/admin/inspectalerts: Pass through --certificate-authority, etc. * | e7900fe Merge pull request openshift#2082 from ardaguclu/code-rabbit-test |\ \ | |/ |/| | * ffe752e Remove mentioning about IRC channel from README * | f114b17 OTA-1581: Promote `oc adm upgrade status` to general availability (openshift#2063) * | 1c54ec0 Merge pull request openshift#2077 from hongkailiu/OTA-1601 |\ \ | * | 5f1dbb8 Move not-lines-related table.Write out of the loop | * | a352662 Truncate long reasons | * | 11f9f47 Reproduce the long reason issue | * | 970f883 status: improve line break handling for CO Message 4299014 (Add squash-merge support into oc adm release info, 2022-04-26, openshift#1116), and mentioned the insights operator: $ git clone --depth 10 --branch master https://github.com/openshift/insights-operator $ cd insights-operator $ git log --graph --oneline -5 * da45333 (HEAD -> master, origin/master, origin/HEAD) OCPBUGS-60611: virt launcher logs gatherer (openshift#1110) * 737b59b fix: incorrect anonymization of domains (openshift#1111) * b5185da feat: add permissions to gather clusterrole (openshift#1106) * 3099eb0 feat: Allow on-demand gathering during initial periodic run (openshift#1109) * 7f8b40d RFE-4152: Add missing readonlyRootFilesystem (openshift#1104) But some repositories also use a mixture of real merges and squash or rebase merges: $ git clone --depth 20 --branch main https://github.com/openshift/operator-framework-operator-controller $ cd operator-framework-operator-controller $ git log --graph --oneline -11 * 9319f22a (HEAD -> main, origin/main, origin/HEAD) UPSTREAM: <carry>: Ensure unique name for bad-catalog tests * 9b50498a UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. * 2ed1a5c8 UPSTREAM: <carry>: Migrate single/own namespace tests * 0c4f4303 UPSTREAM: <carry>: [OTE] add catalog tests from openshift/origin * 28299f38 UPSTREAM: <carry>: remove obsolete owners * 5aa7897f UPSTREAM: <carry>: [OTE] - Readme:Add info to help use payload-aggregate with new tests * 0bb19537 UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. * e9e3220f UPSTREAM: <carry>: [OTE] Add webhook to validate openshift-service-ca certificate rotation * a7d35272 Merge pull request openshift#453 from openshift-bot/synchronize-upstream |\ | * d93f9d16 UPSTREAM: <drop>: configure the commit-checker | * 9a69e8ed UPSTREAM: <drop>: remove upstream GitHub configuration Before this commit, the logic would find the merge commits, assume that all content came in via merge commits, and not mention the other changes: $ git log --first-parent --merges --oneline a7d3527 Merge pull request openshift#453 from openshift-bot/synchronize-upstream 4cae159 Merge pull request openshift#449 from openshift-bot/synchronize-upstream 82f63dc Merge pull request openshift#444 from openshift-bot/synchronize-upstream 6f91d84 Merge pull request openshift#435 from openshift-bot/synchronize-upstream With this commit, we'll continue to include a line for each of those merges. But we'll also include the count of any elided, non-merge, first-parent commits: $ git log --first-parent --no-merges --oneline | wc -l 16 Folks interested in what exactly was elided will have to click through to 'Full changelog' or otherwise go digging. But at least they'll get the count of elided commits to know that there is more information there to see behind that click.
wking
added a commit
to wking/oc
that referenced
this pull request
Aug 29, 2025
…mmits Originally, the change-log generation focused on merge commits, which are common in many repositories. For example, in this 'oc' repository: $ git log --graph --oneline -10 * 559c67e (HEAD -> main, origin/main) Merge pull request openshift#2083 from wking/oc-adm-upgrade-recommend-certificate-authority |\ | * b98b2e1 pkg/cli/admin/inspectalerts: Pass through --certificate-authority, etc. * | e7900fe Merge pull request openshift#2082 from ardaguclu/code-rabbit-test |\ \ | |/ |/| | * ffe752e Remove mentioning about IRC channel from README * | f114b17 OTA-1581: Promote `oc adm upgrade status` to general availability (openshift#2063) * | 1c54ec0 Merge pull request openshift#2077 from hongkailiu/OTA-1601 |\ \ | * | 5f1dbb8 Move not-lines-related table.Write out of the loop | * | a352662 Truncate long reasons | * | 11f9f47 Reproduce the long reason issue | * | 970f883 status: improve line break handling for CO Message 4299014 (Add squash-merge support into oc adm release info, 2022-04-26, openshift#1116), and mentioned the insights operator: $ git clone --depth 10 --branch master https://github.com/openshift/insights-operator $ cd insights-operator $ git log --graph --oneline -5 * da45333 (HEAD -> master, origin/master, origin/HEAD) OCPBUGS-60611: virt launcher logs gatherer (openshift#1110) * 737b59b fix: incorrect anonymization of domains (openshift#1111) * b5185da feat: add permissions to gather clusterrole (openshift#1106) * 3099eb0 feat: Allow on-demand gathering during initial periodic run (openshift#1109) * 7f8b40d RFE-4152: Add missing readonlyRootFilesystem (openshift#1104) But some repositories also use a mixture of real merges and squash or rebase merges: $ git clone --depth 20 --branch main https://github.com/openshift/operator-framework-operator-controller $ cd operator-framework-operator-controller $ git log --graph --oneline -11 * 9319f22a (HEAD -> main, origin/main, origin/HEAD) UPSTREAM: <carry>: Ensure unique name for bad-catalog tests * 9b50498a UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. * 2ed1a5c8 UPSTREAM: <carry>: Migrate single/own namespace tests * 0c4f4303 UPSTREAM: <carry>: [OTE] add catalog tests from openshift/origin * 28299f38 UPSTREAM: <carry>: remove obsolete owners * 5aa7897f UPSTREAM: <carry>: [OTE] - Readme:Add info to help use payload-aggregate with new tests * 0bb19537 UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. * e9e3220f UPSTREAM: <carry>: [OTE] Add webhook to validate openshift-service-ca certificate rotation * a7d35272 Merge pull request openshift#453 from openshift-bot/synchronize-upstream |\ | * d93f9d16 UPSTREAM: <drop>: configure the commit-checker | * 9a69e8ed UPSTREAM: <drop>: remove upstream GitHub configuration Before this commit, the logic would find the merge commits, assume that all content came in via merge commits, and not mention the other changes: $ git log --first-parent --merges --oneline a7d3527 Merge pull request openshift#453 from openshift-bot/synchronize-upstream 4cae159 Merge pull request openshift#449 from openshift-bot/synchronize-upstream 82f63dc Merge pull request openshift#444 from openshift-bot/synchronize-upstream 6f91d84 Merge pull request openshift#435 from openshift-bot/synchronize-upstream With this commit, we'll continue to include a line for each of those merges. But we'll also include the count of any elided, non-merge, first-parent commits: $ git log --first-parent --no-merges --oneline | wc -l 16 Folks interested in what exactly was elided will have to click through to 'Full changelog' or otherwise go digging. But at least they'll get the count of elided commits to know that there is more information there to see behind that click.
wking
added a commit
to wking/oc
that referenced
this pull request
Aug 29, 2025
…mmits Originally, the change-log generation focused on merge commits, which are common in many repositories. For example, in this 'oc' repository: $ git log --graph --oneline -10 * 559c67e (HEAD -> main, origin/main) Merge pull request openshift#2083 from wking/oc-adm-upgrade-recommend-certificate-authority |\ | * b98b2e1 pkg/cli/admin/inspectalerts: Pass through --certificate-authority, etc. * | e7900fe Merge pull request openshift#2082 from ardaguclu/code-rabbit-test |\ \ | |/ |/| | * ffe752e Remove mentioning about IRC channel from README * | f114b17 OTA-1581: Promote `oc adm upgrade status` to general availability (openshift#2063) * | 1c54ec0 Merge pull request openshift#2077 from hongkailiu/OTA-1601 |\ \ | * | 5f1dbb8 Move not-lines-related table.Write out of the loop | * | a352662 Truncate long reasons | * | 11f9f47 Reproduce the long reason issue | * | 970f883 status: improve line break handling for CO Message 4299014 (Add squash-merge support into oc adm release info, 2022-04-26, openshift#1116), and mentioned the insights operator: $ git clone --depth 10 --branch master https://github.com/openshift/insights-operator $ cd insights-operator $ git log --graph --oneline -5 * da45333 (HEAD -> master, origin/master, origin/HEAD) OCPBUGS-60611: virt launcher logs gatherer (openshift#1110) * 737b59b fix: incorrect anonymization of domains (openshift#1111) * b5185da feat: add permissions to gather clusterrole (openshift#1106) * 3099eb0 feat: Allow on-demand gathering during initial periodic run (openshift#1109) * 7f8b40d RFE-4152: Add missing readonlyRootFilesystem (openshift#1104) But some repositories also use a mixture of real merges and squash or rebase merges: $ git clone --depth 20 --branch main https://github.com/openshift/operator-framework-operator-controller $ cd operator-framework-operator-controller $ git log --graph --oneline -11 * 9319f22a (HEAD -> main, origin/main, origin/HEAD) UPSTREAM: <carry>: Ensure unique name for bad-catalog tests * 9b50498a UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. * 2ed1a5c8 UPSTREAM: <carry>: Migrate single/own namespace tests * 0c4f4303 UPSTREAM: <carry>: [OTE] add catalog tests from openshift/origin * 28299f38 UPSTREAM: <carry>: remove obsolete owners * 5aa7897f UPSTREAM: <carry>: [OTE] - Readme:Add info to help use payload-aggregate with new tests * 0bb19537 UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. * e9e3220f UPSTREAM: <carry>: [OTE] Add webhook to validate openshift-service-ca certificate rotation * a7d35272 Merge pull request openshift#453 from openshift-bot/synchronize-upstream |\ | * d93f9d16 UPSTREAM: <drop>: configure the commit-checker | * 9a69e8ed UPSTREAM: <drop>: remove upstream GitHub configuration Before this commit, the logic would find the merge commits, assume that all content came in via merge commits, and not mention the other changes: $ git log --first-parent --merges --oneline a7d3527 Merge pull request openshift#453 from openshift-bot/synchronize-upstream 4cae159 Merge pull request openshift#449 from openshift-bot/synchronize-upstream 82f63dc Merge pull request openshift#444 from openshift-bot/synchronize-upstream 6f91d84 Merge pull request openshift#435 from openshift-bot/synchronize-upstream With this commit, we'll continue to include a line for each of those merges. But we'll also include the count of any elided, non-merge, first-parent commits: $ git log --first-parent --no-merges --oneline | wc -l 16 Folks interested in what exactly was elided will have to click through to 'Full changelog' or otherwise go digging. But at least they'll get the count of elided commits to know that there is more information there to see behind that click.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
ClusterVersion's spec can evolve over time, growing additional properties like capabilities. With the old
POST/Update(...)approach, that could lead to olderoc(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 toPATCH, so we can touch justspec.desiredUpdate, regardless of what else is going on in spec.Also, while I'm touching this code:
--allow-not-recommendedworks with--to-latestas well. This also consolidated so there was only a singleUpdateto convert to aPatch.--allow-explicit-upgrade --to-image ...logging to only complain about--allow-explicit-upgradewhen we needed to use it, and to not log complaints when the requested image was known to ClusterVersionstatus.Details about those two in their commit messages.