Refactor the main package to use Cobra #15

xmudrii · 2018-06-08T12:11:10Z

This PR refactors the main package to move logic to helper functions and to utilize Cobra for CLI.
PR #16 updates documentation to reflect changes made in this PR.

Closes #5
Closes #7
Closes #12

sttts · 2018-06-13T15:17:41Z

pkg/cmd/controller/config/config.go

+// Config is configuration struct for EtcdProxyController. It's used to wire CLI and the controller.
+type EtcdProxyController struct {
+	// EtcdProxy contains information about the controller, such as its namespace and etcd proxy image name.
+	EtcdProxy *options.EtcdProxyOptions


You need a copy of this struct here.

sttts · 2018-06-13T18:39:19Z

pkg/cmd/controller/config/config.go

+}
+
+// EtcdProxyOptions type is used to pass information from cli to the controller.
+type EtcdProxyOptions struct {


call them config here instead of options

Fixed and moved to EtcdProxyController package as proposed by @deads2k.

deads2k · 2018-06-13T18:50:54Z

pkg/cmd/controller/config/config.go

@@ -0,0 +1,51 @@
+/*


followup: seems like you should remove this header from the files.

I will remove all license headers from files in a follow-up PR.

Created issue #17.

deads2k · 2018-06-13T18:51:13Z

pkg/cmd/controller/config/config.go

+	restclient "k8s.io/client-go/rest"
+)
+
+// Config is configuration struct for EtcdProxyController. It's used to wire CLI and the controller.


godoc format please

deads2k · 2018-06-13T18:52:14Z

pkg/cmd/controller/config/config.go

+
+// CoreEtcdOptions type is used to wire the core etcd information used by controller to create ReplicaSets.
+type CoreEtcdOptions struct {
+	URL             string


think of these like an API. please doc each field.

deads2k · 2018-06-13T18:53:05Z

pkg/cmd/controller/config/config.go

+	ControllerNamespace string
+
+	// KubeconfigPath is used to obtain path to kubeconfig, used to create kubeclients.
+	KubeconfigPath string


This looks odd. Why would you have this and the restclient.Config ?

I wanted to have KubeconfigPath passed from CLI and then once config is built, to get restclient.Config.

I have not done the transition between Options and Config well. I'll pay attention to this.

Updated to use only restclient.Config.

deads2k · 2018-06-13T18:53:52Z

pkg/cmd/controller/start.go

+}
+
+// initControllerClientSets returns kubernetes clientset and etcdproxy clientset.
+func initControllerClientSets(kubeconfig *restclient.Config) (*kubernetes.Clientset, *clientset.Clientset, error) {


nit: single use, single path helper functions aren't super useful.

Updated functions for both Clientsets and Informers.

deads2k · 2018-06-13T18:54:59Z

pkg/cmd/controller/start.go

+func controllerNamespace(namespace string) (string, error) {
+	if namespace != "" {
+		return namespace, nil
+	} else if data, err := ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil {


nit: no need for else

} if data, er { }

deads2k · 2018-06-13T18:57:21Z

pkg/cmd/controller/config/config.go

+)
+
+// Config is configuration struct for EtcdProxyController. It's used to wire CLI and the controller.
+type EtcdProxyController struct {


I expected this directly in the controller package.

Moved to the controller package, but decided to leave the struct definitions in the config.go file. Is that okay?

deads2k · 2018-06-13T18:59:18Z

pkg/controller/etcdproxy/etcdproxy_controller_test.go

@@ -32,6 +32,7 @@ import (

 	etcdclient "github.com/xmudrii/etcdproxy-controller/pkg/client/clientset/versioned/fake"
 	etcdlisters "github.com/xmudrii/etcdproxy-controller/pkg/client/listers/etcd/v1alpha1"
+	"github.com/xmudrii/etcdproxy-controller/pkg/options"


This dep strikes me as odd

Fixed. Looks like that one caused import cycles as well.

deads2k · 2018-06-13T19:00:30Z

pkg/options/options.go

+}
+
+func NewCoreEtcdOptions() *CoreEtcdOptions {
+	return &CoreEtcdOptions{}


I like the construct of setting the defaults here, so

{ CAConfigMapName: "etcd-coreserving-ca", }

Do I need to rename the function to something like NewCoreEtcdDefaultOptions, or NewCoreEtcdOptions is good enough?

deads2k · 2018-06-13T19:01:01Z

pkg/options/options.go

+
+func (e *EtcdProxyOptions) AddFlags(fs *pflag.FlagSet) {
+	fs.StringVar(&e.CoreEtcdOptions.URL, "etcd-core-url", "", "The address of the core etcd server.")
+	fs.StringVar(&e.CoreEtcdOptions.CAConfigMapName, "etcd-core-ca-configmap", "etcd-coreserving-ca", "The name of the ConfigMap where CA is stored.")


if you set defaults in the actual object, then this becomes fs.StringVar(&e.CoreEtcdOptions.CAConfigMapName, "etcd-core-ca-configmap", e.CoreEtcdOptions.CAConfigMapName, "The name of the ConfigMap where CA is stored.") and someone can override them.

That sounds much better. Updated.

sttts · 2018-06-14T08:17:39Z

pkg/options/options.go

+}
+
+// EtcdProxyOptions type is used to pass information from cli to the controller.
+type EtcdProxyOptions struct {


same question as above: are these the EtcdProxyControllerOptions?

sttts · 2018-06-14T11:49:40Z

pkg/controller/etcdproxy/config.go

+// EtcdProxyControllerConfig type is used to pass information from cli to the controller.
+type EtcdProxyControllerConfig struct {
+	// CoreEtcdOptions contains information needed to wire up ReplicaSets and the core etcd.
+	CoreEtcdOptions *CoreEtcdConfig


strip Options postfix

Fixed, but GitHub doesn't see the change for some reason.

sttts · 2018-06-14T11:49:54Z

pkg/controller/etcdproxy/etcdproxy_controller.go

-	// etcdProxyOptions is used to wire information used by controller to create ReplicaSets.
-	etcdProxyOptions *EtcdProxyOptions
+	// etcdProxyControllerConfig is used to wire information used by controller to create ReplicaSets.
+	etcdProxyControllerConfig *EtcdProxyControllerConfig


just call it config

sttts · 2018-06-14T11:50:04Z

pkg/controller/etcdproxy/etcdproxy_controller.go

@@ -119,7 +101,7 @@ func NewEtcdProxyController(
 	replicasetsInformer appsinformers.ReplicaSetInformer,
 	servicesInformer corev1informers.ServiceInformer,
 	etcdstorageInformer informers.EtcdStorageInformer,
-	etcdProxyOptions *EtcdProxyOptions) *EtcdProxyController {
+	etcdProxyControllerConfig *EtcdProxyControllerConfig) *EtcdProxyController {


just config

sttts · 2018-06-14T11:50:23Z

pkg/options/options.go

+// EtcdProxyControllerOptions type is used to pass information from cli to the controller.
+type EtcdProxyControllerOptions struct {
+	// CoreEtcdOptions contains information needed to wire up ReplicaSets and the core etcd.
+	CoreEtcdOptions *CoreEtcdOptions


strip Options postfix

deads2k · 2018-06-14T17:20:06Z

main.go

-		glog.Fatalf("Error detecting controller namespace: %s", err.Error())
+	cmd := controller.NewCommandEtcdProxyControllerStart(stopCh)
+	if err := cmd.Execute(); err != nil {
+		fmt.Errorf("unable to run etcdproxy command: %v", err)


This doesn't actually print the error.

fmt.Fprintf(os.Stderr, message)

Actually, reading it, just glog.Fatal(err) will do

deads2k · 2018-06-14T17:21:24Z

pkg/cmd/controller/start.go

+	cmd := &cobra.Command{
+		Short: "Start EtcdProxyController",
+		Long:  "Start EtcdProxyController",
+		RunE: func(c *cobra.Command, args []string) error {


I discovered that this prints a pretty useless usage message. I refactored my command to simply glog.Fatal on errors

Same for me. I think this should be fixed now.

deads2k · 2018-06-14T19:31:00Z

pkg/options/options.go

+	return utilerrors.NewAggregate(errors)
+}
+
+func (e *EtcdProxyControllerOptions) Config() (*etcdproxy.EtcdProxyControllerConfig, error) {


nit: I prefer the ApplyTo construct so that I can easily have multiple options applied to different aspects of the same config type.

deads2k

Just the nit. You can update that later

deads2k · 2018-06-14T19:32:33Z

Looks like @sttts comments are addressed.

lgtm. merging

This was referenced Jun 8, 2018

Add manifest for deploying the controller #3

Merged

Create controller for handling etcd proxy pods and services #2

Merged

xmudrii added the do-not-merge/wip Indicates Work-In-Progress Pull Requests. label Jun 8, 2018

xmudrii added this to the Evaluation 1 milestone Jun 8, 2018

xmudrii mentioned this pull request Jun 11, 2018

Make etcd image configurable. Add OwnerRef if object exists. Tighten functions to a private helper. #13

Merged

xmudrii force-pushed the cobra branch 4 times, most recently from 7666b18 to 17b9a69 Compare June 13, 2018 15:15

sttts reviewed Jun 13, 2018

View reviewed changes

xmudrii force-pushed the cobra branch 3 times, most recently from 2f03a8e to 392282b Compare June 13, 2018 16:26

xmudrii mentioned this pull request Jun 13, 2018

Add documentation describing required setup and how to run EtcdProxyController #16

Merged

sttts reviewed Jun 13, 2018

View reviewed changes

deads2k reviewed Jun 13, 2018

View reviewed changes

xmudrii mentioned this pull request Jun 13, 2018

Remove license headers from files #17

Closed

xmudrii force-pushed the cobra branch from 392282b to fcd4aad Compare June 13, 2018 23:08

xmudrii added ready-for-review Indicates that a Pull Request is ready for review. and removed do-not-merge/wip Indicates Work-In-Progress Pull Requests. labels Jun 13, 2018

xmudrii requested a review from sttts June 13, 2018 23:16

sttts reviewed Jun 14, 2018

View reviewed changes

xmudrii force-pushed the cobra branch from 8c886ac to 2ffac9d Compare June 14, 2018 11:25

sttts reviewed Jun 14, 2018

View reviewed changes

xmudrii force-pushed the cobra branch 2 times, most recently from 9a96c22 to bdd7604 Compare June 14, 2018 12:49

Refactor the main package. Switch to cobra for CLI.

989d7bb

xmudrii force-pushed the cobra branch 3 times, most recently from 26db878 to f3793a7 Compare June 14, 2018 13:45

deads2k reviewed Jun 14, 2018

View reviewed changes

xmudrii force-pushed the cobra branch 3 times, most recently from 279b2f9 to ff75adf Compare June 14, 2018 19:19

xmudrii added 4 commits June 14, 2018 21:20

vendor: vendor cobra, glog and k8s.io/apiserver

de9e022

vendor: run dep ensure

c3e7e5d

Implement Options and Config pattern for EtcdProxyController

d6bab32

Switch to glog in cobra instead of returning error

b86b370

xmudrii force-pushed the cobra branch from ff75adf to b86b370 Compare June 14, 2018 19:20

deads2k reviewed Jun 14, 2018

View reviewed changes

deads2k approved these changes Jun 14, 2018

View reviewed changes

deads2k merged commit 8f43b92 into master Jun 14, 2018

xmudrii deleted the cobra branch June 14, 2018 19:37

xmudrii changed the title ~~Refactor the main package to use cobra and update documentation~~ Refactor the main package to use Cobra Jun 14, 2018

This was referenced Jun 14, 2018

Remove Config function in favor of Validate and ApplyTo #20

Merged

Use double-slash prefixed flags in controller deployment manifest #21

Merged

Refactor the main package to use Cobra #15

Refactor the main package to use Cobra #15

Conversation

xmudrii commented Jun 8, 2018 • edited Loading

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

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

xmudrii Jun 13, 2018 • edited Loading

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

xmudrii Jun 14, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k left a comment

Choose a reason for hiding this comment

deads2k commented Jun 14, 2018 • edited Loading

xmudrii commented Jun 8, 2018 •

edited

Loading

xmudrii Jun 13, 2018 •

edited

Loading

xmudrii Jun 14, 2018 •

edited

Loading

deads2k commented Jun 14, 2018 •

edited

Loading