remove some dead code and add generated oauth clients #15965
Conversation
openshift-merge-robot
added
size/XXL
|
Nothing stands out at me as wrong here. /lgtm |
| @@ -4,6 +4,9 @@ import ( | |||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |||
| ) | |||
|
|
|||
| // +genclient | |||
| // +genclient:nonNamespaced | |||
There was a problem hiding this comment.
@deads2k does OAuthAccessToken implement full CRUD? I see this in legacy:
Create(token *oauthapi.OAuthAccessToken) (*oauthapi.OAuthAccessToken, error)
Get(name string, options metav1.GetOptions) (*oauthapi.OAuthAccessToken, error)
List(opts metav1.ListOptions) (*oauthapi.OAuthAccessTokenList, error)
Delete(name string) error
No watch, no patch, no update :-) (genclient:onlyVerbs=create,get,list,delete) ?
There was a problem hiding this comment.
TIL about the generator, thanks
There was a problem hiding this comment.
But... why shouldn't full CRUD be supported?
There was a problem hiding this comment.
@ironcladlou in some resources methods were not implemented in rest (leading to 404 or worse)... maybe we should generate all verbs in client but it might be confusing to users (they use Patch() but always get 404 as it is not implemented...
There was a problem hiding this comment.
@ironcladlou also missing list will lead to broken informer/lister....
There was a problem hiding this comment.
Not sure I understand what's broken about list... also, what's not covered by the generic registry implementation? Patch is the only thing I haven't found so far
There was a problem hiding this comment.
i was wrong, oauth use generic storage as you noted and we got all verbs for free, so this is correct and the legacy client was left behind.
|
/lgtm cancel |
|
/lgtm (oauth is now using generic storage which gives us all verbs for free, the legacy client was never updated to have them, so this is good thing). |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, ironcladlou, mfojtik 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 |
|
/test extended_networking_minimal flake: #14385 |
|
/retest |
1 similar comment
|
/retest |
|
Automatic merge from submit-queue (batch tested with PRs 15904, 15962, 15838, 15965, 15963) |
remove some dead code and add generated oauth clients.
@mfojtik We need the new clients to divorce ourselves from direct etcd access through the API server layers.
@ironcladlou ought to be pretty easy. I just wanted the easy/clean bit before I start with the harder swizzling.