homogenize fake discovery #62069

yue9944882 · 2018-04-03T16:11:55Z

What this PR does / why we need it:

Cleans fake discovery and leaves the only one defined in client-go/discovery

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

Special notes for your reviewer:

Release note:

NONE

yue9944882 · 2018-04-03T16:12:52Z

/kind cleanup
/assign
/sig api-machinery

yue9944882 · 2018-04-08T06:33:36Z

/cc @liggitt

wenjiaswe · 2018-04-12T20:09:02Z

cc @yliaog

yliaog · 2018-04-13T21:26:11Z

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

-		Verb:     "get",
-		Resource: schema.GroupVersionResource{Resource: "resource"},
-	}
-	c.Invokes(action, nil)


what's the reason for the change here?

@yliaog I believe these codes are useless here, aren't they? Am I missing sth?

i tend to agree with you, however, i don't quite understand why the code for c.Invoke(...) was introduced in the first place, similarly for the other methods ServerResourcesForGroupVersion(), ServerGroups(), ServerVersion()

Yeah.. IDK either. But I'm pretty sure it should be cleaned up anyway.

Cleaning up for for other methods too :)

yue9944882 · 2018-04-17T15:56:33Z

/test pull-kubernetes-bazel-test

yliaog · 2018-04-17T16:39:46Z

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

@@ -37,11 +37,6 @@ type FakeDiscovery struct {
 }


could you update the comment above the type, that mentioned testing.Fake.Invoke

yliaog · 2018-04-17T17:11:42Z

/lgtm

yue9944882 · 2018-04-17T17:40:38Z

@yliaog I just rebased to fix the bazel build failure. Could you please take another look at this once it's all green in the CI? :)

yliaog · 2018-04-17T17:43:01Z

Sure

yue9944882 · 2018-04-18T09:53:49Z

@yliaog PTAL thx :)

yliaog · 2018-04-18T16:42:12Z

/lgtm

k8s-ci-robot · 2018-04-18T16:42:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yliaog, yue9944882
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: smarterclayton

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

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:

pkg/kubectl/OWNERS
staging/src/k8s.io/client-go/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

soltysh · 2018-04-19T09:32:34Z

Please squash your changes into a single commit then I'll approve the kubectl bits, you still need approval for the client-go, though.

k8s-ci-robot · 2018-04-19T09:48:45Z

New changes are detected. LGTM label has been removed.

yue9944882 · 2018-04-19T09:49:21Z

@soltysh Thanks, PTAL

yue9944882 · 2018-04-19T09:50:48Z

/cc @deads2k How do you think about approving this PR plz?

yue9944882 · 2018-04-19T10:19:19Z

/test pull-kubernetes-e2e-kops-aws

deads2k · 2018-04-19T13:03:16Z

We need to be able to track actions. There are two reasons for using a mock:

to get back a return value (you've solved this I think)
to make sure that your code is calling the correct client methods.

this pull eliminates the ability to do item 2.

deads2k · 2018-04-19T13:03:30Z

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

-		Verb:     "get",
-		Resource: schema.GroupVersionResource{Resource: "group"},
-	}
-	c.Invokes(action, nil)


I think we need this or something very much like it.

yue9944882 · 2018-04-19T14:45:59Z

to make sure that your code is calling the correct client methods.

@deads2k Thanks. But it somehow still looks confusing to me THO, because these code might only make a difference by panics when *testing.Fake is nil.

I'll revert the changes of this part.

cleaning up fake discovery impl update fake discovery comment rebase and fixes bazel compilation failure

yue9944882 · 2018-04-19T15:43:41Z

/test pull-kubernetes-kubemark-e2e-gce

yue9944882 · 2018-04-20T03:02:02Z

/test pull-kubernetes-kubemark-e2e-gce

yue9944882 · 2018-04-25T07:35:37Z

@deads2k Friendly ping for a review 😃

k8s-ci-robot · 2018-04-26T22:53:44Z

@yue9944882: PR needs rebase.

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.

fejta-bot · 2018-07-25T23:01:55Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2018-08-24T23:48:37Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2018-09-24T00:41:12Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2018-09-24T00:41:19Z

@fejta-bot: Closing this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 3, 2018

k8s-ci-robot requested review from soltysh and zmerlynn April 3, 2018 16:12

k8s-ci-robot assigned yue9944882 Apr 3, 2018

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Apr 3, 2018

yue9944882 force-pushed the homogenize-fake-discovery-defs branch 5 times, most recently from 213d849 to 1bcb804 Compare April 4, 2018 03:26

k8s-ci-robot requested a review from liggitt April 8, 2018 06:33

yliaog reviewed Apr 13, 2018

View reviewed changes

yliaog reviewed Apr 17, 2018

View reviewed changes

k8s-ci-robot assigned yliaog Apr 17, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2018

yue9944882 force-pushed the homogenize-fake-discovery-defs branch from ba4ef7c to d35a278 Compare April 17, 2018 17:39

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 18, 2018

yue9944882 force-pushed the homogenize-fake-discovery-defs branch from d35a278 to bb728a5 Compare April 19, 2018 09:48

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2018

deads2k reviewed Apr 19, 2018

View reviewed changes

yue9944882 force-pushed the homogenize-fake-discovery-defs branch from bb728a5 to 69c7b04 Compare April 19, 2018 14:50

homogenize fake discovery

6b3ab65

cleaning up fake discovery impl update fake discovery comment rebase and fixes bazel compilation failure

yue9944882 force-pushed the homogenize-fake-discovery-defs branch from 69c7b04 to 6b3ab65 Compare April 19, 2018 14:51

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2018

juanvallejo mentioned this pull request May 8, 2018

move cached_discovery to client-go/discovery #63550

Merged

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 25, 2018

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 24, 2018

k8s-ci-robot closed this Sep 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

homogenize fake discovery #62069

homogenize fake discovery #62069

yue9944882 commented Apr 3, 2018 •

edited

Loading

yue9944882 commented Apr 3, 2018

yue9944882 commented Apr 8, 2018

wenjiaswe commented Apr 12, 2018

yliaog Apr 13, 2018

yue9944882 Apr 14, 2018

yliaog Apr 16, 2018

yue9944882 Apr 17, 2018

yue9944882 Apr 17, 2018

yue9944882 commented Apr 17, 2018

yliaog Apr 17, 2018

yliaog commented Apr 17, 2018

yue9944882 commented Apr 17, 2018

yliaog commented Apr 17, 2018

yue9944882 commented Apr 18, 2018

yliaog commented Apr 18, 2018

k8s-ci-robot commented Apr 18, 2018

soltysh commented Apr 19, 2018

k8s-ci-robot commented Apr 19, 2018

yue9944882 commented Apr 19, 2018

yue9944882 commented Apr 19, 2018

yue9944882 commented Apr 19, 2018

deads2k commented Apr 19, 2018

deads2k Apr 19, 2018

yue9944882 commented Apr 19, 2018

yue9944882 commented Apr 19, 2018

yue9944882 commented Apr 20, 2018

yue9944882 commented Apr 25, 2018

k8s-ci-robot commented Apr 26, 2018

fejta-bot commented Jul 25, 2018

fejta-bot commented Aug 24, 2018

fejta-bot commented Sep 24, 2018

k8s-ci-robot commented Sep 24, 2018

homogenize fake discovery #62069

homogenize fake discovery #62069

Conversation

yue9944882 commented Apr 3, 2018 • edited Loading

yue9944882 commented Apr 3, 2018

yue9944882 commented Apr 8, 2018

wenjiaswe commented Apr 12, 2018

yliaog Apr 13, 2018

Choose a reason for hiding this comment

yue9944882 Apr 14, 2018

Choose a reason for hiding this comment

yliaog Apr 16, 2018

Choose a reason for hiding this comment

yue9944882 Apr 17, 2018

Choose a reason for hiding this comment

yue9944882 Apr 17, 2018

Choose a reason for hiding this comment

yue9944882 commented Apr 17, 2018

yliaog Apr 17, 2018

Choose a reason for hiding this comment

yliaog commented Apr 17, 2018

yue9944882 commented Apr 17, 2018

yliaog commented Apr 17, 2018

yue9944882 commented Apr 18, 2018

yliaog commented Apr 18, 2018

k8s-ci-robot commented Apr 18, 2018

soltysh commented Apr 19, 2018

k8s-ci-robot commented Apr 19, 2018

yue9944882 commented Apr 19, 2018

yue9944882 commented Apr 19, 2018

yue9944882 commented Apr 19, 2018

deads2k commented Apr 19, 2018

deads2k Apr 19, 2018

Choose a reason for hiding this comment

yue9944882 commented Apr 19, 2018

yue9944882 commented Apr 19, 2018

yue9944882 commented Apr 20, 2018

yue9944882 commented Apr 25, 2018

k8s-ci-robot commented Apr 26, 2018

fejta-bot commented Jul 25, 2018

fejta-bot commented Aug 24, 2018

fejta-bot commented Sep 24, 2018

k8s-ci-robot commented Sep 24, 2018

yue9944882 commented Apr 3, 2018 •

edited

Loading