etcd management, phases & fixes

[xetys]

a multi PR with the following contents:

quickfix for #266
introduction of cluster etcd management: #267
fixing static apiserver-count #185
introducing phases system #269

[mavimo]

@xetys this PR move the project one level up, nice!!! I'll take some time for me to review it (for sure not before the next weekend)...

[mavimo]

Add empty line between std lib package and external

Suggested change

	"fmt"
	"fmt"

[mavimo]

This should be omitted if we force cobra command to 1 positional argument using Args configuration, like:

var clusterCmd = &cobra.Command{
	Use:   "cluster",
	// ...
	Args: cobra.ExactArgs(1),
	// ...
}

We should also use a more sophisticated validation adding ValidArgs, that report the list of available cluster we have, eg:

var clusterCmd = &cobra.Command{
	Use:   "cluster",
	// ...
	ValidArgs: AppConf.Config.AllClustersName(),
	Args: cobra.ExactValidArgs(1),
	// ...
}

[xetys]

That is a good hint. I will try to solve it partially when I can, but actually, this is a little bit out of the scope of this PR. I think replacing all validations (because this file only defines the validation function, but the cluster root command doesn't use it. Other commands already do. But yes, we should do this at some point.

[mavimo]

move after stdlib packages

[mavimo]

Also return error code if we have error and we trap it, otherwise return error to cobra. RunE to allow the cobra library to return formatted message and error code, eg:

var backupCmd = &cobra.Command{
	// ...
	RunE: func(cmd *cobra.Command, args []string) error {
		snapshotName, _ := cmd.Flags().GetString("snapshot-name")
		etcdManager := getEtcdManager(cmd, args)

 		return etcdManager.CreateSnapshot(snapshotName)
	}

[mavimo]

Use RunE and return error on failure

[mavimo]

IMHO we make the code more comprehensible if we do no use code like step := 3 that hide what we are supposing to do here. Maybe we can iterate over step, add a step name and check name here? Or we can use a chained list on step definition so we can know what's the next step?

[xetys]

The issue with the steps here is, that it's currently a pity with the UIProgress. If you once ran over 100% before it is actually done, it sends a done signal to the channel and the bar synchronization is not really working. This leads to the progress always showing some work, as it actually is already complete. You must know the steps before.

I'm also considering a different way of showing progress. Initially, I liked the UI Progress, but it is so ugly in handling. WDYT?

[mavimo]

I know that is a good idea be DRY but IMHO we should declare the command without helper like declarePhaseCommand and moving function outside init (imho should be awesome if we are able to move all this stuff in a package that can be invoked from the cmd part of the application)

[mavimo]

Maybe I'm a bit ripetitive but using RunE and return error when we should stop command is a better approach than use custom function like FatalOnError ;)

[mavimo]

Using 6 here is really confusing, I suggest to move into a const list (maybe iota?) to define a more comprehensible name using a liked list

[mavimo]

@xetys I add some note. Some should be repeated on code but IMHO is not useful until a decision on how to proceed.
There are three main topic IMHO:

1. Use cobra feature to argument validation/command error reporting instead of custom commands
2. Use chained list to define PhaseChain so each command is aware of the next one and can invoke the "execute next" operation if needed.
3. Better manage how we should start on command execution when we start from a specific task

I'll like also to discuss some folder structure organisation (eg grouping phases command in a single folder) and package organisation (eg move the actions outside the cmd package and move into pkg folder, than invoke it on commands, imho separate operational task from the command that define how we expose the UI (cmd should be a kind of ui, but we should expose it also a web page or WS) is better approach.

PS: I REALLY like the work you do on this PR, ad hope that my topic as keep as suggestion that can help to improve the project and not as critique on your work ;)

[xetys]

Well, it took me some time to start. I did some of your hints. However, stuff like redesign the PreRunE to valid args or better chain design is something I would like to see in a different PR.