-
Notifications
You must be signed in to change notification settings - Fork 39k
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
Make the fake command factory return the clientset with appropriate rest clients for all the API groups. #35866
Make the fake command factory return the clientset with appropriate rest clients for all the API groups. #35866
Conversation
clientset.RbacClient.RESTClient.Client = fakeClient.Client | ||
clientset.StorageClient.RESTClient.Client = fakeClient.Client | ||
clientset.AppsClient.RESTClient.Client = fakeClient.Client | ||
clientset.PolicyClient.RESTClient.Client = fakeClient.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.
DiscoveryClient
wasn't covered. Is it intentional?
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.
If one adds more clients in Clientset
, how do we make sure one swap the fake REST client here?
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.
DiscoveryClient wasn't covered. Is it intentional?
Oops. That wasn't intentional. Copy-paste fail. Added it now.
If one adds more clients in Clientset, how do we make sure one swap the fake REST client here?
Unfortunately, yes.
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 think you'll need to rebase because RESTClient isn't there anymore :'(
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.
@janetkuo yeah. This entire chain of PRs need a rebase because of that. I will take care of the rebases, please ignore it for now.
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.
This LGTM then.
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.
@janetkuo thanks!
Jenkins verification failed for commit 68de01d409ff6cb61f50486d60dfa6de85522172. Full PR test history. The magic incantation to run this job again is |
Jenkins GCI GKE smoke e2e failed for commit 68de01d409ff6cb61f50486d60dfa6de85522172. Full PR test history. The magic incantation to run this job again is |
Jenkins GKE smoke e2e failed for commit 68de01d409ff6cb61f50486d60dfa6de85522172. Full PR test history. The magic incantation to run this job again is |
Jenkins GCE Node e2e failed for commit 68de01d409ff6cb61f50486d60dfa6de85522172. Full PR test history. The magic incantation to run this job again is |
Jenkins Kubemark GCE e2e failed for commit 68de01d409ff6cb61f50486d60dfa6de85522172. Full PR test history. The magic incantation to run this job again is |
Jenkins GCI GCE e2e failed for commit 68de01d409ff6cb61f50486d60dfa6de85522172. Full PR test history. The magic incantation to run this job again is |
Jenkins GCE e2e failed for commit 68de01d409ff6cb61f50486d60dfa6de85522172. Full PR test history. The magic incantation to run this job again is |
Jenkins GCE etcd3 e2e failed for commit 68de01d409ff6cb61f50486d60dfa6de85522172. Full PR test history. The magic incantation to run this job again is |
…est clients for all the API groups. Calling `internalclientset.New()` with a rest client as an argument simply copies that rest client to all the API group clients irrespective of the configured GroupVersion or versionedAPIPath in the client. So only one API group client gets the client configured correctly for that API group. All the other API group clients get misconfigured rest clients. On the other hand, `internalclientset.NewForConfigOrDie()` does the right thing by reconfiguring the passed configs for each API group and initializes an appropriate rest client for that group. Now that we are relying on the `NewForConfigOrDie()` method to initialize the rest clients, we need to swap the underlying http clients in each of these rest clients with a fake one for testing.
68de01d
to
e69475d
Compare
Rebased the PR and squashed the commits. Adding LGTM label. |
Jenkins unit/integration failed for commit e69475d. Full PR test history. The magic incantation to run this job again is |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue |
Please review only the last commit here. This is based on PR #35865 which will be reviewed independently.
Design Doc: PR #34484
cc @kubernetes/sig-cluster-federation @nikhiljindal
This change is![Reviewable](https://camo.githubusercontent.com/2d899f4291d07d3cd2fa4aaae1e3b243f164c23fce87d30a589ace0d496a444c/68747470733a2f2f72657669657761626c652e6b756265726e657465732e696f2f7265766965775f627574746f6e2e737667)