Add the possibility to set return values for the FakeDiscovery implementation #51130

luxas · 2017-08-22T17:39:57Z

What this PR does / why we need it:

As an user of the fake clientset (with the fake discovery), I want to be able to set the fake server's version on demand like this for example:

func TestFakingServerVersion(t *testing.T) {
	client := fakeclientset.NewSimpleClientset()
	fakeDiscovery, ok := client.Discovery().(*fakediscovery.FakeDiscovery)
	if !ok {
		t.Fatalf("couldn't convert Discovery() to *FakeDiscovery")
	}

	testGitCommit := "v1.0.0"
	fakeDiscovery.FakedServerVersion = &version.Info{
		GitCommit: testGitCommit,
	}

	sv, err := client.Discovery().ServerVersion()
	if err != nil {
		t.Fatalf("unexpected error: %v", err)
	}
	if sv.GitCommit != testGitCommit {
		t.Fatalf("unexpected faked discovery return value: %q", sv.GitCommit)
	}
}

This PR makes that possible, in wait for a more sophisticated FakeDiscovery implementation generally.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

NONE

@kubernetes/sig-api-machinery-pr-reviews

deads2k · 2017-08-23T15:45:41Z

staging/src/k8s.io/client-go/discovery/fake/discovery.go

@@ -67,7 +80,11 @@ func (c *FakeDiscovery) ServerPreferredNamespacedResources() ([]*metav1.APIResou
 }

 func (c *FakeDiscovery) ServerGroups() (*metav1.APIGroupList, error) {
-	return nil, nil
+	return c.serverGroups, nil


This appears to be building the layer the wrong location. It seems like you want a simple skin on the *testing.Fake which setups up the "proper" handling for the various Invokes calls by the PrependReactor methods. That way you can use the normal check of Actions if you need to.

I'm fine with making it easier to set those, but I'd like to see these calls making their proper remote calls. That may require refactoring the DiscoveryClient itself to separate the accessors from the helpers, but I think that is more productive than doing it like this.

@luxas without looking into details, what @deads2k suggests was what you tried?

caesarxuchao · 2017-08-24T20:45:07Z

/assign @caesarxuchao

luxas · 2017-08-24T20:57:18Z

@caesarxuchao @sttts @deads2k Current proposal:
Usage (passing):

/*
Copyright 2017 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

    http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package foo

import (
	"testing"

	fakediscovery "k8s.io/client-go/discovery/fake"
	fakeclientset "k8s.io/client-go/kubernetes/fake"
	apimachineryversion "k8s.io/apimachinery/pkg/version"
)

func TestSettingFakeServerVersion(t *testing.T) {
	client := fakeclientset.NewSimpleClientset()
	fakeDiscovery, ok := client.Discovery().(*fakediscovery.FakeDiscovery)
	if !ok {
		t.Fatalf("couldn't convert")
	}

	testGitCommit := "v2.0.0"
	fakeDiscovery.FakedServerVersion = &apimachineryversion.Info{
		GitCommit: testGitCommit,
	}
	sv, err := client.Discovery().ServerVersion()
	if err != nil {
		t.Fatalf("unexpected err: %v", err)
	}
	if sv.GitCommit != testGitCommit {
		t.Fatalf("unexpected gitcommit: %q, %v", sv.GitCommit, *sv)
	}
}

Changes:

diff --git a/staging/src/k8s.io/client-go/discovery/fake/discovery.go b/staging/src/k8s.io/client-go/discovery/fake/discovery.go
index 02e77cfe71..aae63da3e3 100644
--- a/staging/src/k8s.io/client-go/discovery/fake/discovery.go
+++ b/staging/src/k8s.io/client-go/discovery/fake/discovery.go
@@ -33,6 +33,7 @@ import (
 
 type FakeDiscovery struct {
        *testing.Fake
+       FakedServerVersion *version.Info
 }
 
 func (c *FakeDiscovery) ServerResourcesForGroupVersion(groupVersion string) (*metav1.APIResourceList, error) {
@@ -76,6 +77,11 @@ func (c *FakeDiscovery) ServerVersion() (*version.Info, error) {
        action.Resource = schema.GroupVersionResource{Resource: "version"}
 
        c.Invokes(action, nil)
+
+       if c.FakedServerVersion != nil {
+               return c.FakedServerVersion, nil
+       }
+
        versionInfo := kubeversion.Get()
        return &versionInfo, nil
 }
diff --git a/staging/src/k8s.io/client-go/kubernetes/fake/clientset_generated.go b/staging/src/k8s.io/client-go/kubernetes/fake/clientset_generated.go
index 5569e92b7d..22937ccecd 100644
--- a/staging/src/k8s.io/client-go/kubernetes/fake/clientset_generated.go
+++ b/staging/src/k8s.io/client-go/kubernetes/fake/clientset_generated.go
@@ -87,10 +87,9 @@ func NewSimpleClientset(objects ...runtime.Object) *Clientset {
 
        fakePtr := testing.Fake{}
        fakePtr.AddReactor("*", "*", testing.ObjectReaction(o))
-
        fakePtr.AddWatchReactor("*", testing.DefaultWatchReactor(watch.NewFake(), nil))
 
-       return &Clientset{fakePtr}
+       return &Clientset{fakePtr, &fakediscovery.FakeDiscovery{Fake: &fakePtr}}
 }
 
 // Clientset implements clientset.Interface. Meant to be embedded into a
@@ -98,10 +97,11 @@ func NewSimpleClientset(objects ...runtime.Object) *Clientset {
 // you want to test easier.
 type Clientset struct {
        testing.Fake
+       discovery *fakediscovery.FakeDiscovery
 }
 
 func (c *Clientset) Discovery() discovery.DiscoveryInterface {
-       return &fakediscovery.FakeDiscovery{Fake: &c.Fake}
+       return c.discovery
 }
 
 var _ clientset.Interface = &Clientset{}

WDYT? Can implement for the other methods as well...

caesarxuchao · 2017-08-24T21:11:48Z

The apiserver fixture uses GroupVersionResource as keys, but our discovery API aren't mapping to resources. @luxas's proposal is making up non-existing resources to represent the discovery request, e.g., action.Resource = schema.GroupVersionResource{Resource: "version"}.

How do you plan to represent request sent to "/apis", "apis/extension", or "apis/extensions/v1beta1"?

sttts · 2017-08-25T06:16:35Z

staging/src/k8s.io/client-go/discovery/fake/discovery.go

+	discovery.DiscoveryInterface
+	SetServerGroups(*metav1.APIGroupList)
+	SetServerVersion(*version.Info)
+	SetRESTClient(restclient.Interface)


why the rest client?

luxas · 2017-08-25T13:50:50Z

@deads2k @caesarxuchao @sttts PTAL at the latest version. WDYT?

sttts · 2017-08-25T13:56:47Z

staging/src/k8s.io/client-go/kubernetes/fake/clientset_generated.go

 	fakePtr.AddWatchReactor("*", testing.DefaultWatchReactor(watch.NewFake(), nil))

-	return &Clientset{fakePtr}
+	return &Clientset{fakePtr, &fakediscovery.FakeDiscovery{Fake: &fakePtr}}


this must go into the generator

What generator are you talking about?

deads2k · 2017-08-25T13:59:08Z

staging/src/k8s.io/client-go/discovery/fake/discovery.go

 type FakeDiscovery struct {
 	*testing.Fake
+	FakedServerVersion *version.Info
+	FakedRESTClient restclient.Interface


I didn't notice this one in the comment in your diff. Can we avoid it?

Before this; RESTClient() always returned nil. With this, the user can set this field if they want to make client.Discovery().RESTClient() for a faked clientset to work, but it will still default to nil.
WDYT?

deads2k · 2017-08-25T13:59:54Z

One comment on a field that I think is unused right now, then @sttts comment and this lgtm.

caesarxuchao · 2017-08-25T22:12:49Z

lgtm

caesarxuchao · 2017-08-25T22:19:11Z

Both @deads2k and @sttts 's comments are addressed. Adding the label.

/lgtm

k8s-github-robot · 2017-08-25T22:19:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: caesarxuchao, luxas
We suggest the following additional approver: lavalamp

Assign the PR to them by writing /assign @lavalamp in a comment when ready.

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

luxas · 2017-08-25T22:42:48Z

@caesarxuchao just updated bazel as I noticed I didn't do that before.

luxas · 2017-08-25T22:43:51Z

I think this needs @smarterclayton's or @lavalamp's approval though.
Please look at this, should be pretty straightforward
(needed for --dry-run in #48899 to work correctly)

…mentation

luxas · 2017-08-27T15:35:17Z

Should hopefully fix small verification failure...

luxas · 2017-08-27T16:01:41Z

/retest

caesarxuchao · 2017-08-30T22:20:43Z

/assign @lavalamp

for approval.

caesarxuchao · 2017-08-30T22:20:49Z

/retest

caesarxuchao · 2017-09-01T19:50:59Z

/retest

caesarxuchao · 2017-09-01T19:52:57Z

I'm going to manually approve this one. It's lgtm'ed long ago, I don't want it to slip the code freeze.

luxas · 2017-09-01T21:42:30Z

/retest

Thank you @caesarxuchao, yeah, this is pretty trivial in any case so it's much more about review bandwidth than someone not wanting to approve this PR.

luxas · 2017-09-02T11:26:32Z

/retest

…

On 02 Sep 2017, at 13:38, k8s-ci-robot ***@***.***> wrote: @luxas: The following tests failed, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-verify e1cff67 link /test pull-kubernetes-verify pull-kubernetes-kubemark-e2e-gce e1cff67 link /test pull-kubernetes-kubemark-e2e-gce 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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

CaoShuFeng · 2017-09-02T14:30:19Z

/test pull-kubernetes-verify

luxas · 2017-09-02T19:57:43Z

/retest

luxas · 2017-09-02T20:46:09Z

/retest

k8s-ci-robot · 2017-09-02T22:21:21Z

@luxas: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`e1cff67`	link	`/test pull-kubernetes-e2e-kops-aws`

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.

k8s-github-robot · 2017-09-03T01:18:49Z

Automatic merge from submit-queue (batch tested with PRs 51335, 51364, 51130, 48075, 50920)

luxas assigned ncdc and sttts Aug 22, 2017

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 22, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Aug 22, 2017

deads2k reviewed Aug 23, 2017

View reviewed changes

k8s-ci-robot assigned caesarxuchao Aug 24, 2017

sttts reviewed Aug 25, 2017

View reviewed changes

luxas force-pushed the fake_discovery branch from 14912ff to a0a36dc Compare August 25, 2017 13:50

sttts reviewed Aug 25, 2017

View reviewed changes

deads2k reviewed Aug 25, 2017

View reviewed changes

luxas added this to the v1.8 milestone Aug 25, 2017

luxas force-pushed the fake_discovery branch from a0a36dc to 48aec46 Compare August 25, 2017 21:59

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 25, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2017

luxas force-pushed the fake_discovery branch from 48aec46 to 6092e69 Compare August 25, 2017 22:41

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2017

luxas added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2017

luxas mentioned this pull request Aug 25, 2017

Implement the kubeadm upgrade command #48899

Merged

luxas added 2 commits August 27, 2017 18:31

Make it possible to fake the ServerVersion in the FakeDiscovery imple…

9b060fa

…mentation

autogenerated code

e1cff67

luxas force-pushed the fake_discovery branch from 6092e69 to e1cff67 Compare August 27, 2017 15:32

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2017

luxas added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2017

k8s-ci-robot assigned lavalamp Aug 30, 2017

caesarxuchao added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2017

k8s-github-robot merged commit 94d9457 into kubernetes:master Sep 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the possibility to set return values for the FakeDiscovery implementation #51130

Add the possibility to set return values for the FakeDiscovery implementation #51130

luxas commented Aug 22, 2017 •

edited

deads2k Aug 23, 2017

sttts Aug 24, 2017

caesarxuchao commented Aug 24, 2017

luxas commented Aug 24, 2017

caesarxuchao commented Aug 24, 2017

sttts Aug 25, 2017

luxas commented Aug 25, 2017

sttts Aug 25, 2017

luxas Aug 25, 2017

deads2k Aug 25, 2017

luxas Aug 25, 2017

deads2k commented Aug 25, 2017

caesarxuchao commented Aug 25, 2017

caesarxuchao commented Aug 25, 2017

k8s-github-robot commented Aug 25, 2017

luxas commented Aug 25, 2017

luxas commented Aug 25, 2017 •

edited

luxas commented Aug 27, 2017

luxas commented Aug 27, 2017

caesarxuchao commented Aug 30, 2017

caesarxuchao commented Aug 30, 2017

caesarxuchao commented Sep 1, 2017

caesarxuchao commented Sep 1, 2017

luxas commented Sep 1, 2017

luxas commented Sep 2, 2017 via email

CaoShuFeng commented Sep 2, 2017

luxas commented Sep 2, 2017

luxas commented Sep 2, 2017

k8s-ci-robot commented Sep 2, 2017

k8s-github-robot commented Sep 3, 2017

Add the possibility to set return values for the FakeDiscovery implementation #51130

Add the possibility to set return values for the FakeDiscovery implementation #51130

Conversation

luxas commented Aug 22, 2017 • edited

deads2k Aug 23, 2017

Choose a reason for hiding this comment

sttts Aug 24, 2017

Choose a reason for hiding this comment

caesarxuchao commented Aug 24, 2017

luxas commented Aug 24, 2017

caesarxuchao commented Aug 24, 2017

sttts Aug 25, 2017

Choose a reason for hiding this comment

luxas commented Aug 25, 2017

sttts Aug 25, 2017

Choose a reason for hiding this comment

luxas Aug 25, 2017

Choose a reason for hiding this comment

deads2k Aug 25, 2017

Choose a reason for hiding this comment

luxas Aug 25, 2017

Choose a reason for hiding this comment

deads2k commented Aug 25, 2017

caesarxuchao commented Aug 25, 2017

caesarxuchao commented Aug 25, 2017

k8s-github-robot commented Aug 25, 2017

luxas commented Aug 25, 2017

luxas commented Aug 25, 2017 • edited

luxas commented Aug 27, 2017

luxas commented Aug 27, 2017

caesarxuchao commented Aug 30, 2017

caesarxuchao commented Aug 30, 2017

caesarxuchao commented Sep 1, 2017

caesarxuchao commented Sep 1, 2017

luxas commented Sep 1, 2017

luxas commented Sep 2, 2017 via email

CaoShuFeng commented Sep 2, 2017

luxas commented Sep 2, 2017

luxas commented Sep 2, 2017

k8s-ci-robot commented Sep 2, 2017

k8s-github-robot commented Sep 3, 2017

luxas commented Aug 22, 2017 •

edited

luxas commented Aug 25, 2017 •

edited