⚠ Expose Client runtime.Scheme #1058
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[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 |
k8s-ci-robot
added
approved
|
/test pull-controller-runtime-test-master |
1 similar comment
|
/test pull-controller-runtime-test-master |
|
/milestone v0.7.x |
|
/retest |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
/retest |
k8s-ci-robot
removed
the
lgtm
Signed-off-by: Vince Prignano <vincepri@vmware.com>
vincepri
force-pushed
the
expose-client-scheme
branch
from
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.
Is there a way to have a fake RESTMapper?
There was a problem hiding this comment.
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.
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.
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.
Will do! I'll split this commit in another PR first :)
vincepri
force-pushed
the
expose-client-scheme
branch
from
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 |
k8s-ci-robot
removed
the
do-not-merge/hold
|
@alvaroaleman Should be good to go / be merged |
k8s-ci-robot
added
the
lgtm
Signed-off-by: Vince Prignano vincepri@vmware.com