refact kubectl Factory make it interface #33180

adohe-zz · 2016-09-21T15:34:12Z

refactor kubectl to make Factory interface. @kubernetes/kubectl

This change is

deads2k · 2016-09-21T15:46:51Z

@fabianofranz This ripples downstream.

deads2k · 2016-09-21T15:47:32Z

I'm in favor of the change, but wow that's big :)

MHBauer · 2016-09-21T19:52:29Z

related to #31702 ?

deads2k · 2016-09-21T19:54:11Z

related to #31702 ?

Looks like it.

smarterclayton · 2016-09-21T22:05:40Z

Please split the *cmdutil.Factory change in each command into its own commit - that will make this much easier to review.

adohe-zz · 2016-09-22T00:31:39Z

yes, sure, I will split this into separate commits.

adohe-zz · 2016-09-22T00:37:54Z

@fabianofranz As we have discussed on email, we will propose a new SIG for kubectl, which gives us a place to discuss the CLI architecture, like this one. So here I want to make sure this is the correct way. I will get this merged asap. @pwittrock ptal.

soltysh · 2016-09-23T09:54:35Z

pkg/kubectl/cmd/cmd_test.go

@@ -209,7 +213,12 @@ type testFactory struct {
 	Err          error
 }

-func NewTestFactory() (*cmdutil.Factory, *testFactory, runtime.Codec, runtime.NegotiatedSerializer) {
+type fakeFactory struct {
+	tf    *testFactory


Would it make sense to have a general fakeFactory for tests, that will have all the methods implemented, with empty bodies, of course. Then in commands you'd be embedding it and just providing an implementation to whichever method you need. This should simplify testing, imho.

@soltysh a general fakeFactory with all methods implemented with empty bodies? what if several commands want to use a same implementation of some methods?

That shouldn't be a problem for several commands re-using those.

k8s-ci-robot · 2016-09-25T08:00:33Z

Jenkins GCI Kubemark GCE e2e failed for commit 8918223. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark gci e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k82cn · 2016-09-25T16:17:17Z

pkg/kubectl/cmd/cmd_test.go

+	return nil
+}
+
+func (f *fakeFactory) PrinterForMapping(cmd *cobra.Command, mapping *meta.RESTMapping, withNamespace bool) (kubectl.ResourcePrinter, error) {


Should we return t.tf.Printer ?

Should we return t.tf.Printer

I think we should keep this pull targeted. Switch them all to methods on the Factory interface and then change signatures later. This pull is already massive.

k82cn · 2016-09-25T16:23:16Z

pkg/kubectl/cmd/cmd_test.go

+}
+
+func (f *fakeFactory) NewBuilder() *resource.Builder {
+	return nil


The DrainOptions.SetupDrain in cmd/drain.go will use NewBuidler of factory, and the default implementation of NewFactory will take Factory::Object's return value as parameters. That's what I mean in slack.

But we're luck, it seems DrainOptions.SetupDrain is not used by anyone :).

we still need to be careful about this.

deads2k · 2016-09-26T12:20:06Z

pkg/kubectl/cmd/util/factory.go

-	Object func() (meta.RESTMapper, runtime.ObjectTyper)
+type Factory interface {
+	// Returns the inner flag set
+	Flags() *pflag.FlagSet


This used to be private, right? Can we keep it private?

deads2k · 2016-09-26T12:20:48Z

pkg/kubectl/cmd/util/factory.go

+	// Command will stringify and return all environment arguments ie. a command run by a client
+	// using the factory.
+	// TODO: We need to filter out stuff like secrets.
+	Command() string


Why have these been added?

@deads2k originally Factory struct has these methods, and they are exported, so other packages can use. If we make Factory interface, and not add these methods to the interface definition, other packages which use this interface can not use theses methods anymore.

deads2k · 2016-09-26T12:21:22Z

pkg/kubectl/cmd/util/factory.go

+	// TODO: We need to filter out stuff like secrets.
+	Command() string
+	// BindFlags adds any flags that are common to all kubectl sub commands.
+	BindFlags(flags *pflag.FlagSet)


If these weren't exposed before, lets keep them hidden. I don't think we want to encourage access patterns like this.

@deads2k actually these are methods of Factory struct before, and they are exported. The problem is why we add these methods as struct methods, and others as struct fields.

deads2k · 2016-09-26T12:22:46Z

pkg/kubectl/cmd/util/factory.go

+	// BindExternalFlags adds any flags defined by external projects (not part of pflags)
+	BindExternalFlags(flags *pflag.FlagSet)
+	// PrintObject prints an api object given command line flags to modify the output format
+	PrintObject(cmd *cobra.Command, mapper meta.RESTMapper, obj runtime.Object, out io.Writer) error


please remove/hide. We don't want to the factory accepting the command. We want specific structs passing the information we need and all of these look be net-new

deads2k · 2016-09-26T12:26:09Z

Mind squashing into

update factory.
create test factory
mechanical command updates? - looks like these were mostly just removing the *.

I'd like to see the interface slimmed down a little bit. If that turns out to require rippling changes, just make follow-up issues. This is a nice improvement over where we are now.

deads2k · 2016-09-26T12:27:23Z

After squash and green, I'd merge this as-is and we can iterate.

I bumped the priority, once its green, we want this to beat any other kubectl changes.

adohe-zz · 2016-09-28T15:24:02Z

3. mechanical command updates? - looks like these were mostly just removing the *.

yes, so I would like to merge these into one commit, and one commit one command would leave so many commits tests failed.

adohe-zz · 2016-10-13T00:25:25Z

@deads2k @fabianofranz @pwittrock I just rebased and fixed the failed tests, please take a look, and give your feedback, I would hope we can get this merged asap. This is just a beginning of Factory refactor, eventually we will make Factory a collection of interfaces, and finally we will implement something we discussed in #31173 #7311

0xmichalis · 2016-10-13T09:35:23Z

pkg/kubectl/cmd/annotate.go

@@ -54,7 +54,7 @@ type AnnotateOptions struct {
 	newAnnotations    map[string]string
 	removeAnnotations []string

-	// Common shared fields


shared is correct here

0xmichalis · 2016-10-13T09:36:13Z

pkg/kubectl/cmd/drain.go

@@ -419,6 +419,7 @@ func (o *DrainOptions) RunCordonOrUncordon(desired bool) error {
 			unsched.SetBool(desired)
 			_, err := helper.Replace(cmdNamespace, o.nodeInfo.Name, true, o.nodeInfo.Object)
 			if err != nil {
+				fmt.Println("get error here")


remove this?

some stupid error, :(

0xmichalis · 2016-10-13T09:37:19Z

pkg/kubectl/cmd/util/factory.go

+	// Requires that printer flags have been added to cmd (see AddPrinterFlags).
+	PrinterForMapping(cmd *cobra.Command, mapping *meta.RESTMapping, withNamespace bool) (kubectl.ResourcePrinter, error)
+	// One stop shopping for a Builder
+	NewBuilder() *resource.Builder


adohe-zz · 2016-10-13T13:03:29Z

@deads2k @fabianofranz @pwittrock @Kargakis rebased and update, ptal.

deads2k · 2016-10-13T13:22:14Z

@deads2k @fabianofranz @pwittrock @Kargakis rebased and update, ptal.

I don't see an obvious, "this doesn't work", so I'm tagging this as-is. This breaks the back of the problem and I'd like to merge it and take everything else as a follow-up unless someone finds a "this breaks kubectl" kind of bug.

0xmichalis · 2016-10-13T13:32:57Z

Agreed.

On Thu, Oct 13, 2016 at 3:22 PM, David Eads notifications@github.com
wrote:

@deads2k https://github.com/deads2k @fabianofranz
https://github.com/fabianofranz @pwittrock
https://github.com/pwittrock @Kargakis https://github.com/kargakis
rebased and update, ptal.

I don't see an obvious, "this doesn't work", so I'm tagging this as-is.
This breaks the back of the problem and I'd like to merge it and take
everything else as a follow-up unless someone finds a "this breaks kubectl"
kind of bug.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#33180 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADuFf5hZTN1ioeKIpO6xKH8LZn1jWTHYks5qzjCigaJpZM4KC8-K
.

k8s-github-robot · 2016-10-13T14:05:17Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

adohe-zz · 2016-10-13T14:28:34Z

wow~ so glad to see this thanks @deads2k @Kargakis @fabianofranz @pwittrock

k8s-github-robot · 2016-10-13T14:44:05Z

Automatic merge from submit-queue

googlebot added the cla: yes label Sep 21, 2016

k8s-github-robot assigned deads2k Sep 21, 2016

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Sep 21, 2016

deads2k assigned fabianofranz and unassigned deads2k Sep 21, 2016

soltysh reviewed Sep 23, 2016

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2016

adohe-zz force-pushed the refactory_interface branch from edb7b7e to 8918223 Compare September 25, 2016 07:55

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2016

adohe-zz changed the title ~~[WIP] refact kubectl Factory make it interface~~ refact kubectl Factory make it interface Sep 25, 2016

k8s-github-robot mentioned this pull request Sep 25, 2016

[k8s.io] Downward API volume should update labels on modification [Conformance] {E2eNode Suite} #33423

Closed

k82cn suggested changes Sep 25, 2016

View reviewed changes

deads2k reviewed Sep 26, 2016

View reviewed changes

deads2k added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Sep 26, 2016

deads2k added this to the v1.5 milestone Sep 26, 2016

adohe-zz force-pushed the refactory_interface branch from 29e19ac to 284dbbc Compare October 13, 2016 00:20

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2016

0xmichalis reviewed Oct 13, 2016

View reviewed changes

deads2k mentioned this pull request Oct 13, 2016

Move the common test functions from cmd_test.go to cmd/testing/fake.go #34683

Merged

adohe added 3 commits October 13, 2016 20:57

refactor Factory make it interface

593d8e0

add fake factories for test

dfae1a2

update various commands to adapt the new Factory interface

b2280a6

adohe-zz force-pushed the refactory_interface branch from 284dbbc to b2280a6 Compare October 13, 2016 13:02

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2016

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 13, 2016

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Oct 13, 2016

deads2k added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note-label-needed labels Oct 13, 2016

k8s-github-robot merged commit 9b3ca2f into kubernetes:master Oct 13, 2016

adohe-zz deleted the refactory_interface branch October 13, 2016 15:13

k82cn mentioned this pull request Oct 17, 2016

Refactor cmdutil.Factory to be an Interface. #31793

Closed

This was referenced Oct 17, 2016

"Unable to register third party resources" in kubectl help #34977

Closed

Implement Factory interface and mock for test. openshift/origin#11290

Closed

idealhack mentioned this pull request Jul 2, 2018

Make kubectl exec support "pod/" prefix in the pod name (closes #65670) #65673

Closed

refact kubectl Factory make it interface #33180

refact kubectl Factory make it interface #33180

Conversation

adohe-zz commented Sep 21, 2016 • edited by k8s-oncall Loading

deads2k commented Sep 21, 2016

deads2k commented Sep 21, 2016

MHBauer commented Sep 21, 2016

deads2k commented Sep 21, 2016

smarterclayton commented Sep 21, 2016

adohe-zz commented Sep 22, 2016

adohe-zz commented Sep 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Sep 26, 2016

deads2k commented Sep 26, 2016

adohe-zz commented Sep 28, 2016

adohe-zz commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adohe-zz commented Oct 13, 2016

deads2k commented Oct 13, 2016

0xmichalis commented Oct 13, 2016

k8s-github-robot commented Oct 13, 2016

adohe-zz commented Oct 13, 2016

k8s-github-robot commented Oct 13, 2016

adohe-zz commented Sep 21, 2016 •

edited by k8s-oncall

Loading