Add boilerplate for clusterctl #119

karan · 2018-05-01T17:43:09Z

What this PR does / why we need it: Adds boilerplate for clusterctl tool.

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 #112

Special notes for your reviewer: This is only a boilerplate to unblock anyone who wants to start working on adding features.

@kubernetes/kube-deploy-reviewers

karan · 2018-05-01T17:43:19Z

/assign @jessicaochen

jessicaochen · 2018-05-01T19:17:59Z

clusterctl/CONTRIBUTING.md

@@ -0,0 +1 @@
+# Contributing Guidelines


Is it intentional to have empty conributing guidelines for this PR?

Yes. I'm happy to remove the file though, but I wanted to link to something from the README.

jessicaochen · 2018-05-01T19:18:19Z

clusterctl/README.md

+
+### Prerequisites
+
+Follow the steps listed at [CONTRIBUTING.md](https://github.com/kubernetes/kube-deploy/blob/master/cluster-api/clusterctl/CONTRIBUTING.md) to:


Wrong repo?

jessicaochen · 2018-05-01T19:18:54Z

clusterctl/README.md

+
+Follow the steps listed at [CONTRIBUTING.md](https://github.com/kubernetes/kube-deploy/blob/master/cluster-api/clusterctl/CONTRIBUTING.md) to:
+
+1. Build the `clusterctl` tool 


probably will want an example of how to build.

jessicaochen · 2018-05-01T19:19:08Z

clusterctl/README.md

+Follow the steps listed at [CONTRIBUTING.md](https://github.com/kubernetes/kube-deploy/blob/master/cluster-api/clusterctl/CONTRIBUTING.md) to:
+
+1. Build the `clusterctl` tool 
+3. Create a `machines.yaml` file configured for your cluster. See the provided template for an example.


How come it jumped from 1 to 3?

MD renders ordered lists just fine even if the numbers are wrong here. I'll fix them nevertheless.

jessicaochen · 2018-05-01T19:19:24Z

clusterctl/README.md

+1. Build the `clusterctl` tool 
+3. Create a `machines.yaml` file configured for your cluster. See the provided template for an example.
+
+### Limitation


(nit)
Limitations

jessicaochen · 2018-05-01T19:20:41Z

clusterctl/README.md

+object. There will be defaults running. This will make the tool easily pluggage
+(change the controller spec) instead of changing the tool or mucking with flags.
+
+1. Create a cluster: `./clusterctl create cluster -c cluster.yaml -m machines.yaml -e extras.yaml`


I would leave the create cluster details out. They can be filled in as the implementation is checked in. That way the docs and reality keep in sync.

jessicaochen · 2018-05-01T19:28:10Z

clusterctl/cmd/root.go

+}
+
+func init() {
+	RootCmd.PersistentFlags().StringVarP(&kubeConfig, "kubeconfig", "k", "", "location for the kubernetes config file. If not provided, $HOME/.kube/config is used")


Please leave all flags out of the skeleton. They can be filled in as code uses them. This ensures the the flags actually do something when they appear and also ensure they are defined in a way that works and is accurate to the behavior of the code.

For example, I already know this definition of kubeconfig will not be accurate for bootstrapping.

jessicaochen · 2018-05-01T19:30:45Z

clusterctl/cmd/create.go

+	createCmd.Flags().StringVarP(&co.Machine, "machines", "m", "", "machine yaml file")
+	createCmd.Flags().StringVarP(&co.Extras, "extras", "e", "", "extras yaml file")
+
+	RootCmd.AddCommand(createCmd)


I thought the goal was something like:

./clusterctl create cluster .....

This seems to produce:

./clusterctl create ....

Or did I miss something?

Based on community feedback, we decided to align ourselves with the rest of the tooling -- This is also reflected in https://docs.google.com/document/d/1-sYb3EdkRga49nULH1kSwuQFf1o6GvAw_POrsNo5d8c/edit#.

jessicaochen · 2018-05-01T19:32:25Z

clusterctl/cmd/create.go

+*/
+
+package cmd
+


Would recommend the cluster subcommand files be named with the cluster prefix. That way we do not have confusion when we have other NOUNS that happen to have a similar verb eg. create.

cluster_create.go or clustercreate.go instead of create.go

Now that we are going with the kubectl/kops model of VERB NOUN, this comment is moot?

jessicaochen · 2018-05-01T19:34:30Z

clusterctl/cmd/create.go

+import (
+	"os"
+
+	//"fmt"


Is it intentional to have a commented out import?

Removed it. Typo

jessicaochen · 2018-05-02T17:52:02Z

clusterctl/cmd/create.go

+*/
+
+package cmd
+


jessicaochen · 2018-05-02T17:53:38Z

clusterctl/cmd/create.go

+	return errors.NotImplementedError
+}
+func init() {
+	RootCmd.AddCommand(createCmd)


I believe we are doing VERB NOUN right now. This makes me expect
clusterctl create cluster .....
to be the create commmand instead of
clusterctl create ....
which is what this seems to be doing.

Good catch! Added the subcommands

jessicaochen · 2018-05-02T17:55:18Z

clusterctl/README.md

+### Creating a cluster
+
+**NOT YET SUPPORTED!**
+


I would cut out everything other than NOT YET SUPPORTED! in the creating a cluster section till it actually is supported. Similar reasoning to flags, it is best to keep the implementation and documentation in sync.

jessicaochen · 2018-05-02T17:57:37Z

clusterctl/cmd/root.go

+	},
+}
+
+var (


On my previous comment on flags, I do mean all flags on all commands should not be implemented yet. They can be implemented when there is actually functionality to back it. This way the usage and description is accurate to how it is actually used.

The flags have been removed, I guess I missed the var.

The var seems to still be here. There also still seem to be flags on some of the commands that are not backed by implementation.

Weird my linter didn't complain.

spew · 2018-05-03T13:36:23Z

clusterctl/cmd/create.go

+		if co.Cluster == "" {
+			glog.Error("Please provide yaml file for cluster definition.")
+			cmd.Help()
+			os.Exit(1)


We are doing the following:

cmd.Help() os.Exit(...)

in 4 locations in the Create command and there are similar sections in Delete and Register commands. Theoretically this number will grow as the scope of this application increases. Is there a simplification we can make so that it is only called in a single place for the various error scenarios?

Good point. DOne

jessicaochen · 2018-05-07T18:09:16Z

clusterctl/cmd/register.go

+var ro = &RegisterOptions{}
+
+var registerCmd = &cobra.Command{
+	Use:   "register",


Should register also follow VERB NOUN? ie. "register cluster" instead of just "register"?

Whenever register is implemented, it can be branched out. For now, I'm leaving it as is.

I do not think this is the right approach.

Either we structure it as it is supposed to be structured ie. VERB NOUN or we leave it out entirely and when it is implemented, it is added with the right structure.

I have decided to remove it and leave it up to implementer to add this cmd as well.

jessicaochen · 2018-05-07T18:10:06Z

clusterctl/cmd/root.go

+	},
+}
+
+var (


The var seems to still be here. There also still seem to be flags on some of the commands that are not backed by implementation.

jessicaochen · 2018-05-08T17:27:48Z

clusterctl/cmd/register.go

+var ro = &RegisterOptions{}
+
+var registerCmd = &cobra.Command{
+	Use:   "register",


I do not think this is the right approach.

Either we structure it as it is supposed to be structured ie. VERB NOUN or we leave it out entirely and when it is implemented, it is added with the right structure.

jessicaochen · 2018-05-08T17:29:48Z

clusterctl/cmd/create_cluster.go

+type CreateOptions struct {
+	Cluster string
+	Machine string
+	Extras  string


I thought we were using a different name for this.

Yes, good catch. I changed it.

jessicaochen

/approve
/lgtm

k8s-ci-robot · 2018-05-10T16:59:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jessicaochen, karan

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:

~~OWNERS~~ [jessicaochen,karan]

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

Use same logger throughout the codebase

Add boilerplate for clusterctl

3efe03d

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 1, 2018

k8s-ci-robot assigned jessicaochen May 1, 2018

jessicaochen suggested changes May 1, 2018

View reviewed changes

jessicaochen suggested changes May 2, 2018

View reviewed changes

spew reviewed May 3, 2018

View reviewed changes

karan force-pushed the clusterctl branch from ce1e375 to a254107 Compare May 4, 2018 22:39

jessicaochen suggested changes May 7, 2018

View reviewed changes

karan force-pushed the clusterctl branch from a254107 to 391dbbc Compare May 7, 2018 21:36

jessicaochen suggested changes May 8, 2018

View reviewed changes

remove flags from clusterctl boilerplate

0ec639a

karan force-pushed the clusterctl branch from 391dbbc to 0ec639a Compare May 9, 2018 17:37

jessicaochen approved these changes May 10, 2018

View reviewed changes

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

k8s-ci-robot merged commit fd68ff1 into kubernetes-sigs:master May 10, 2018

karan mentioned this pull request May 25, 2018

Add validate cluster command to clusterctl #203

Merged

karan deleted the clusterctl branch June 6, 2018 15:21

chuckha pushed a commit to chuckha/cluster-api that referenced this pull request Oct 2, 2019

Merge pull request kubernetes-sigs#119 from vincepri/klogr-everywhere

6537609

Use same logger throughout the codebase


		### Prerequisites

		Follow the steps listed at [CONTRIBUTING.md](https://github.com/kubernetes/kube-deploy/blob/master/cluster-api/clusterctl/CONTRIBUTING.md) to:


		Follow the steps listed at [CONTRIBUTING.md](https://github.com/kubernetes/kube-deploy/blob/master/cluster-api/clusterctl/CONTRIBUTING.md) to:

		1. Build the `clusterctl` tool

+              	},
+              }
+              var (

		*/

		package cmd

		*/

		package cmd

Add boilerplate for clusterctl #119

Add boilerplate for clusterctl #119

Conversation

karan commented May 1, 2018

karan commented May 1, 2018

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

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

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

jessicaochen left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 10, 2018