-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
remove usage of legacy client in oc describers #16041
remove usage of legacy client in oc describers #16041
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
8a4e7b0
to
be81c5e
Compare
The cited example happens to fall in our desired support window ("For example, a 3.4 oc will work against 3.3, 3.4, and 3.5 servers.") but we should update that table with |
if err != nil { | ||
glog.V(1).Info(err) | ||
} | ||
|
||
m := map[schema.GroupKind]kprinters.Describer{ | ||
buildapi.Kind("Build"): &BuildDescriber{c, kclient}, |
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.
reason to not drop the last 7 uses of c
and snip that client completely?
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.
reason to not drop the last 7 uses of c and snip that client completely?
I hadn't decided how granular I'd go with the kube client ones.
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.
leaving kclient as-is and getting rid of c
would let us eliminate /oapi calls here. Not a big deal, just wondered
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.
leaving kclient as-is and getting rid of c would let us eliminate /oapi calls here. Not a big deal, just wondered
I'll pick it up on the next pass.
LGTM |
be81c5e
to
2f31964
Compare
/retest |
@fabianofranz @juanvallejo I'm thinking about splitting all the describers by API group, so that they end up closer to the types. |
2f31964
to
cf47f92
Compare
/retest |
1 similar comment
/retest |
/retest |
3 similar comments
/retest |
/retest |
/retest |
Automatic merge from submit-queue |
This pull will switch describers in oc 3.7 from using /oapi to using the groupified API. This means that normal CRUD will work from oc3.7 to master3.5, but
oc describe
from oc3.7 to master3.5 will not.oc describe
from oc3.5 to master3.7 will work finekubectl
only supports one level of skew. Do we need to update ours to match: https://github.com/openshift/openshift-docs/blob/d5c99ae36b59436459219701c2c19d1bce9bf905/release_notes/index.adoc ?@fabianofranz @smarterclayton @liggitt