-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
⚠ Expose Client runtime.Scheme #1058
⚠ Expose Client runtime.Scheme #1058
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test pull-controller-runtime-test-master |
1 similar comment
/test pull-controller-runtime-test-master |
/milestone v0.7.x |
/retest |
/retest |
Signed-off-by: Vince Prignano <vincepri@vmware.com>
39bed6b
to
f1885ed
Compare
@vincepri I am all in for the restmapper change, but lets please make it a distinct PR so it gets its own release note :) |
I separated the commits because I knew you'd say that :P, wanted to get some eyes on it, given all the PRs all blocked for now |
pkg/client/fake/client.go
Outdated
} | ||
|
||
func (c *fakeClient) RESTMapper() meta.RESTMapper { | ||
return nil |
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.
Is there a way to have a fake RESTMapper?
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.
from a quick glance it seems there is no exported fake for this, only a private one: https://github.com/kubernetes/kubernetes/blob/0051d65f9f30db724dfb88256f70c23e37f6b257/staging/src/k8s.io/client-go/restmapper/shortcut_test.go#L197
At the end of the day we will need to hand-maintain a list of cluster-scoped objects to get this working for unit tests though. I think that should be okay though, because kube doesn't have that many cluster-scoped objects. We can also allow ppl to add additional types and their scope information so this can be used with cluster-scoped crds.
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.
Would you be ok with having it nil for now and ask for some contributor to fill this in? I don't find the fake client particularly useful in many cases, but that's just me probably :D
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.
yeah sgtm. Can you create an issue though, maybe with "good first issue" as its mostly:
- Copy the other implementation
- add a list of cluster-scoped objects
- Return namespace-scoped by default and cluster-scoped if in list?
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.
Will do! I'll split this commit in another PR first :)
f1885ed
to
e263d2c
Compare
@vincepri: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
/hold cancel |
@alvaroaleman Should be good to go / be merged |
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
Signed-off-by: Vince Prignano vincepri@vmware.com