Make pkg/kubectl/cmd/util/factor.go:Factory to be interface #31702

k82cn · 2016-08-30T15:20:20Z

Refactor pkg/kubectl/cmd/util/factor.go:Factory to be a interface. Currently, the util.Factory is defined by function pointer:

type Factory struct {
...
    Object func(thirdPartyDiscovery bool) (meta.RESTMapper, runtime.ObjectTyper)
    // Returns interfaces for dealing with arbitrary
    // runtime.Unstructured. This performs API calls to discover types.
    UnstructuredObject func() (meta.RESTMapper, runtime.ObjectTyper, error)
...

In Go, it's better to make Factory to be an struct; it'll also reduce the code line of NewFactory() which is more than 100 line of code :).

cc @adohe / @pwittrock

The text was updated successfully, but these errors were encountered:

ncdc · 2016-08-30T15:27:26Z

cc @deads2k

pwittrock · 2016-08-30T15:40:01Z

@adohe Does this make sense to you? If so, lets have @k82cn pick it up

deads2k · 2016-08-30T16:40:27Z

Refactor pkg/kubectl/cmd/util/factor.go:Factory to be a struct. Currently, the util.Factory is defined by function pointer:

Factory already is a struct. I could support making Factory into an interface, promoting its various function into typed functions, and then having first class methods for each function. I think this is the sort of thing that @fabianofranz, @Kargakis, and @kubernetes/kubectl are likely to have opinions on.

I think that you could start by creating first class methods for the anonymous ones currently included.

deads2k · 2016-08-30T16:41:11Z

Also, starting with a single method as an example is probably worthwhile so we can argue over principle there instead of in a larger pull.

smarterclayton · 2016-08-30T16:58:38Z

And really it should be multiple interfaces and each command should take the interfaces it requires. That argument is partially why we have deferred this for so long.

k82cn · 2016-08-30T23:35:19Z

@deads2k , agree :). I'm going to refactor function pointer into functions firstly (one by one); and then refactor Factory to be an interface.

For multiple interfaces as @smarterclayton , we can discuss it after those patches.

Any comments?

adohe-zz · 2016-08-30T23:51:13Z

@k82cn @deads2k @smarterclayton actually we have discussed a lot about kubectl refactor in #7311 #31173, generally we want decouple command framework from command logical, we will try to create logical groups. I agree that we could promote those functions to typed functions, this could be a good start.

0xmichalis · 2016-08-31T08:17:07Z

Factory already is a struct. I could support making Factory into an interface, promoting its various function into typed functions, and then having first class methods for each function.

A bunch of the functions already conform to specific kubectl interfaces so it seems that this should end up being an interface composed of other interfaces.

Remaining business logic that exists in the Factory should be factored out to interfaces in pkg/kubectl
Example that conforms to the above already:

kubernetes/pkg/kubectl/cmd/util/factory.go

Lines 690 to 698 in ebe733e

    
           HistoryViewer: func(mapping *meta.RESTMapping) (kubectl.HistoryViewer, error) { 
        
           	mappingVersion := mapping.GroupVersionKind.GroupVersion() 
        
           	client, err := clients.ClientForVersion(&mappingVersion) 
        
           	clientset := clientset.FromUnversionedClient(client) 
        
           	if err != nil { 
        
           		return nil, err 
        
           	} 
        
           	return kubectl.HistoryViewerFor(mapping.GroupVersionKind.GroupKind(), clientset) 
        
           },

kubernetes/pkg/kubectl/history.go

Line 39 in ebe733e

type HistoryViewer interface {

Example that doesn't:

kubernetes/pkg/kubectl/cmd/util/factory.go

Lines 571 to 629 in ebe733e

    
           LogsForObject: func(object, options runtime.Object) (*restclient.Request, error) { 
        
           	c, err := clients.ClientForVersion(nil) 
        
           	if err != nil { 
        
           		return nil, err 
        
           	} 
        
           	switch t := object.(type) { 
        
           	case *api.Pod: 
        
           		opts, ok := options.(*api.PodLogOptions) 
        
           		if !ok { 
        
           			return nil, errors.New("provided options object is not a PodLogOptions") 
        
           		} 
        
           		return c.Pods(t.Namespace).GetLogs(t.Name, opts), nil 
        
           	case *api.ReplicationController: 
        
           		opts, ok := options.(*api.PodLogOptions) 
        
           		if !ok { 
        
           			return nil, errors.New("provided options object is not a PodLogOptions") 
        
           		} 
        
           		selector := labels.SelectorFromSet(t.Spec.Selector) 
        
           		sortBy := func(pods []*api.Pod) sort.Interface { return controller.ByLogging(pods) } 
        
           		pod, numPods, err := GetFirstPod(c, t.Namespace, selector, 20*time.Second, sortBy) 
        
           		if err != nil { 
        
           			return nil, err 
        
           		} 
        
           		if numPods > 1 { 
        
           			fmt.Fprintf(os.Stderr, "Found %v pods, using pod/%v\n", numPods, pod.Name) 
        
           		} 
        
           		return c.Pods(pod.Namespace).GetLogs(pod.Name, opts), nil 
        
           	case *extensions.ReplicaSet: 
        
           		opts, ok := options.(*api.PodLogOptions) 
        
           		if !ok { 
        
           			return nil, errors.New("provided options object is not a PodLogOptions") 
        
           		} 
        
           		selector, err := unversioned.LabelSelectorAsSelector(t.Spec.Selector) 
        
           		if err != nil { 
        
           			return nil, fmt.Errorf("invalid label selector: %v", err) 
        
           		} 
        
           		sortBy := func(pods []*api.Pod) sort.Interface { return controller.ByLogging(pods) } 
        
           		pod, numPods, err := GetFirstPod(c, t.Namespace, selector, 20*time.Second, sortBy) 
        
           		if err != nil { 
        
           			return nil, err 
        
           		} 
        
           		if numPods > 1 { 
        
           			fmt.Fprintf(os.Stderr, "Found %v pods, using pod/%v\n", numPods, pod.Name) 
        
           		} 
        
           		return c.Pods(pod.Namespace).GetLogs(pod.Name, opts), nil 
        
           	default: 
        
           		gvks, _, err := api.Scheme.ObjectKinds(object) 
        
           		if err != nil { 
        
           			return nil, err 
        
           		} 
        
           		return nil, fmt.Errorf("cannot get the logs from %v", gvks[0]) 
        
           	} 
        
           },

smarterclayton · 2016-08-31T13:16:52Z

Refactoring into functions would break valid uses in OpenShift (we
implement those). So a partial refactor that leaves us without an
interface isn't an option. I'd recommend going the other way, create an
interface, then turn those methods into it one by one.

On Tue, Aug 30, 2016 at 7:35 PM, Klaus Ma notifications@github.com wrote:

@deads2k https://github.com/deads2k , agree :). I'm going to refactor
function pointer into functions firstly (one by one); and then refactor
Factory to be an interface.

For multiple interfaces as @smarterclayton
https://github.com/smarterclayton , we can discuss it after those
patches.

Any comments?

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

k82cn · 2016-08-31T14:39:45Z

@smarterclayton , do you mean OpenShift implements some additional cmd?

Try to refactor Factory.Object today: if moving it out of struct as function, the cmd_test.go should be also updated accordingly; but no idea for now on following test cases, maybe we need to create interface firstly, any suggestion?

func NewAPIFactory() (*cmdutil.Factory, *testFactory, runtime.Codec, runtime.NegotiatedSerializer) {
    t := &testFactory{
        Validator: validation.NullSchema{},
    }

    f := &cmdutil.Factory{
        Object: func(discovery bool) (meta.RESTMapper, runtime.ObjectTyper) {
            return testapi.Default.RESTMapper(), api.Scheme
        },
        UnstructuredObject: func() (meta.RESTMapper, runtime.ObjectTyper, error) {
            groupResources := testDynamicResources()
....

BTW, it seems there's no test case in facotry_test.go for Factory.Object; I'll add some test cases for it.

adohe-zz · 2016-09-01T02:36:38Z

A bunch of the functions already conform to specific kubectl interfaces so it seems that this should end up being an interface composed of other interfaces.

I would support this way.

k82cn · 2016-09-04T02:31:46Z

Hi team,

After updating the patch, some unit test are failed; so I'm working to fix it. But any suggestion on how to debug it?

I use the following command to make check WHAT=pkg/kubectl/cmd KUBE_TEST_ARGS="-c" to generate a binary to debug, but I got the following message :(. Any suggestions?

Klauss-MacBook-Pro:kubernetes klaus$ lldb cmd.test
(lldb) target create "cmd.test"
Current executable set to 'cmd.test' (x86_64).
(lldb) r  -test.v=true -test.run="TestCordon"
Process 41181 launched: '/Users/klaus/Workspace/kubernetes/cmd.test' (x86_64)
=== RUN   TestCordon
--- FAIL: TestCordon (0.00s)
        drain_test.go:195: node/node syntax: unexpected error
FAIL
Process 41181 exited with status = 1 (0x00000001)
(lldb) b SetupDrain
Breakpoint 1: where = cmd.test`k8s.io/kubernetes/pkg/kubectl/cmd.(*DrainOptions).SetupDrain + 47 at drain.go:176, address = 0x0000000000094b9f
(lldb) r  -test.v=true -test.run="TestCordon"
Process 41193 launched: '/Users/klaus/Workspace/kubernetes/cmd.test' (x86_64)
=== RUN   TestCordon
Process 41193 stopped
* thread #1: tid = 0x17db68, 0x0000000000094b9f libsystem_c.dylib`_DefaultRuneXLocale + 279, stop reason = breakpoint 1.1
    frame #0: 0x0000000000094b9f libsystem_c.dylib`_DefaultRuneXLocale + 279
libsystem_c.dylib`_DefaultRuneXLocale:
->  0x94b9f <+279>: movq   0xd0(%rsp), %rax
    0x94ba7 <+287>: cmpq   $0x1, %rax
    0x94bab <+291>: je     0x94cb1                   ; <+553>
    0x94bb1 <+297>: movq   $0x0, 0x98(%rsp)
(lldb)

k8s-github-robot added area/kubectl sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Aug 30, 2016

pwittrock added good-for-contributor-onboarding and removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Aug 30, 2016

k8s-github-robot added team/ux and removed team/ux labels Aug 30, 2016

k82cn changed the title ~~Make pkg/kubectl/cmd/util/factor.go:Factory to be struct~~ Make pkg/kubectl/cmd/util/factor.go:Factory to be interface Aug 30, 2016

k82cn mentioned this issue Aug 31, 2016

Refactor cmdutil.Factory to be an Interface. #31793

Closed

k82cn mentioned this issue Sep 6, 2016

Test Factory.PrintObject by fake command #32133

Closed

janetkuo assigned k82cn Sep 8, 2016

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

MHBauer mentioned this issue Sep 21, 2016

refact kubectl Factory make it interface #33180

Merged

guilhermebr mentioned this issue Oct 10, 2016

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

Closed

pwittrock closed this as completed Oct 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make pkg/kubectl/cmd/util/factor.go:Factory to be interface #31702

Make pkg/kubectl/cmd/util/factor.go:Factory to be interface #31702

k82cn commented Aug 30, 2016 •

edited

Loading

ncdc commented Aug 30, 2016

pwittrock commented Aug 30, 2016

deads2k commented Aug 30, 2016

deads2k commented Aug 30, 2016

smarterclayton commented Aug 30, 2016

k82cn commented Aug 30, 2016

adohe-zz commented Aug 30, 2016

0xmichalis commented Aug 31, 2016

smarterclayton commented Aug 31, 2016

k82cn commented Aug 31, 2016

adohe-zz commented Sep 1, 2016

k82cn commented Sep 4, 2016

Make pkg/kubectl/cmd/util/factor.go:Factory to be interface #31702

Make pkg/kubectl/cmd/util/factor.go:Factory to be interface #31702

Comments

k82cn commented Aug 30, 2016 • edited Loading

ncdc commented Aug 30, 2016

pwittrock commented Aug 30, 2016

deads2k commented Aug 30, 2016

deads2k commented Aug 30, 2016

smarterclayton commented Aug 30, 2016

k82cn commented Aug 30, 2016

adohe-zz commented Aug 30, 2016

0xmichalis commented Aug 31, 2016

smarterclayton commented Aug 31, 2016

k82cn commented Aug 31, 2016

adohe-zz commented Sep 1, 2016

k82cn commented Sep 4, 2016

k82cn commented Aug 30, 2016 •

edited

Loading