Skip to content
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.

Sign up for GitHub

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

Jump to bottom

🌱 Un-deprecate NewFakeClient #1101

Merged
merged 1 commit into from Sep 28, 2020
Merged

🌱 Un-deprecate NewFakeClient #1101

merged 1 commit into from Sep 28, 2020

Conversation

vincepri
Copy link
Member

@vincepri vincepri commented Aug 4, 2020

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

@k8s-ci-robot k8s-ci-robot added this to the v0.7.x milestone Aug 4, 2020
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 4, 2020
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 4, 2020
@vincepri vincepri force-pushed the remove-deprecated-newfakeclient branch from 54a43e5 to b9449f4 Compare August 4, 2020 16:18
vincepri
vincepri commented Aug 4, 2020
View reviewed changes
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 {
Copy link
Member Author

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

Copy link
Member

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

Copy link
Member Author

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

@alvaroaleman
Copy link
Member

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.

@alvaroaleman
Copy link
Member

That is probably more of a question for @DirectXMan12 as he was driving the whole "enforce scheme everywhere"

@vincepri
Copy link
Member Author

vincepri commented Aug 4, 2020

It might be error-prone, folks can always pass scheme.Scheme explicitly this way

@alvaroaleman
Copy link
Member

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 WithScheme and the only thing I will do different if we merge this (and I presume most ppl that use the fake client) is construct a downstream helper that passes in the kubernetes scheme as the first arg. Can we maybe discuss this at the next meeting to clarify what risks exactly this is preventing, because thats not clear to me?

@vincepri
Copy link
Member Author

vincepri commented Aug 4, 2020

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 scheme.Scheme, although might be good to keep both and remove the deprecation notice if we'd like to keep the other constructor

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2020
@alvaroaleman
Copy link
Member

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.

@alvaroaleman
Copy link
Member

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2020
@alvaroaleman
Copy link
Member

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.

@vincepri
Copy link
Member Author

Gonna rework the PR later this week

@vincepri
🌱 Un-deprecate NewFakeClient
994c1b3 
Signed-off-by: Vince Prignano <vincepri@vmware.com>
@vincepri vincepri force-pushed the remove-deprecated-newfakeclient branch from b9449f4 to 994c1b3 Compare September 28, 2020 17:17
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 28, 2020
@vincepri vincepri changed the title ⚠️ Remove deprecated NewFakeClient function, replace with scheme ⚠️ Un-deprecate NewFakeClient Sep 28, 2020
@vincepri vincepri changed the title ⚠️ Un-deprecate NewFakeClient 🌱 Un-deprecate NewFakeClient Sep 28, 2020
@vincepri
Copy link
Member Author

@alvaroaleman Reworked to un-deprecate

alvaroaleman
alvaroaleman approved these changes Sep 28, 2020
View reviewed changes
Copy link
Member

@alvaroaleman alvaroaleman left a 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!

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2020
@k8s-ci-robot k8s-ci-robot merged commit ae48371 into kubernetes-sigs:master Sep 28, 2020
@drpaneas drpaneas mentioned this pull request Apr 16, 2021
Closed
@alvaroaleman alvaroaleman mentioned this pull request Apr 26, 2021
Closed
@tomasaschan tomasaschan mentioned this pull request Nov 23, 2021
Merged
@matteoolivi matteoolivi mentioned this pull request Jul 14, 2022
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvaroaleman alvaroaleman alvaroaleman approved these changes

@alenkacz alenkacz Awaiting requested review from alenkacz

@pwittrock pwittrock Awaiting requested review from pwittrock

Assignees

@alvaroaleman alvaroaleman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v0.7.x
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vincepri @k8s-ci-robot @alvaroaleman