-
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
🌱 Un-deprecate NewFakeClient #1101
🌱 Un-deprecate NewFakeClient #1101
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 |
54a43e5
to
b9449f4
Compare
pkg/client/fake/client.go
Outdated
// You can choose to initialize it with a slice of runtime.Object. | ||
func NewFakeClientWithScheme(clientScheme *runtime.Scheme, initObjs ...runtime.Object) client.Client { | ||
func NewFakeClient(clientScheme *runtime.Scheme, initObjs ...runtime.Object) client.Client { |
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.
Should we remove initObjs
support as well here? It creates object without a resourceVersion which is technically incorrect
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.
I would like to keep it, its super convenient. If anything, we can extend this to set the resourceVersion
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, there was another PR out to do that, although that breaks lots of tests
I don't really understand why we need to enforce passing in a scheme. IMHO, using the kubernetes one by default and allowing an override is totally reasonable. |
That is probably more of a question for @DirectXMan12 as he was driving the whole "enforce scheme everywhere" |
It might be error-prone, folks can always pass scheme.Scheme explicitly this way |
When is this error prone? I've literally never used the |
We have a lot of tests using custom schemes (in cluster api), because that's how they were setup. I'm not opposed to say that folks should use |
I want to discuss this at the next meeting. IMHO we should undeprecate it. It makes total sense to allow passing a Scheme but it does not make sense to me to enforce this. I've a long history of building controllers and I've never used anything other than the default client-go scheme. To me its not clear what the problem with that is. |
/hold |
We discussed this in the meeting on Aug 27th and decided that we can undeprecate it. The main reason for deprecating it was that ppl used it with CRDs without adding them to their scheme, resulting in errors. We can instead better handle this error and return an error that clarifies what needs to be done to add the types to the scheme. |
Gonna rework the PR later this week |
Signed-off-by: Vince Prignano <vincepri@vmware.com>
b9449f4
to
994c1b3
Compare
@alvaroaleman Reworked to un-deprecate |
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
/hold cancel
Thanks!
Signed-off-by: Vince Prignano vincepri@vmware.com
This PR removes the long deprecated function from the fake client, and replaces it with the one that requires a scheme
/milestone v0.7.x