Reduce CLI code to only handle user inputs (like flags and arguments) and output

[invidian]

Currently, our cli/cmd package does a lot of things like:

• checking cluster health: https://github.com/kinvolk/lokomotive/blob/cc8cafa989d1578ec2a055fdb1174f19e4433856/cli/cmd/health.go
• orchestrating cluster creation: https://github.com/kinvolk/lokomotive/blob/cc8cafa989d1578ec2a055fdb1174f19e4433856/cli/cmd/cluster-apply.go
• rendering manifests: https://github.com/kinvolk/lokomotive/blob/cc8cafa989d1578ec2a055fdb1174f19e4433856/cli/cmd/component-render-manifest.go

All those functions are not tested and are difficult to test. There is also a lot of duplicated code for each subcommand.

In my opinion, we should reduce the responsibility of this package to handle:

• CLI subcommands definition with descriptions etc
• flags handling
• controlling user inputs (like confirmation dialogs) and outputs (creating logger and passing it to the code)

Other code should be encapsulated into some structure with clearly defined exported API, which can easily be tested.

For example,

lokomotive/cli/cmd/health.go

Lines 46 to 111 in cc8cafa

	kubeconfig, err := getKubeconfig()
	if err != nil {
	contextLogger.Fatalf("Error in finding kubeconfig file: %s", err)
	}
	client, err := k8sutil.NewClientset(kubeconfig)
	if err != nil {
	contextLogger.Fatalf("Error in creating setting up Kubernetes client: %q", err)
	}
	p, diags := getConfiguredPlatform()
	if diags.HasErrors() {
	for _, diagnostic := range diags {
	contextLogger.Error(diagnostic.Error())
	}
	contextLogger.Fatal("Errors found while loading cluster configuration")
	}
	if p == nil {
	contextLogger.Fatal("No cluster configured")
	}
	cluster, err := lokomotive.NewCluster(client, p.Meta().ExpectedNodes)
	if err != nil {
	contextLogger.Fatalf("Error in creating new Lokomotive cluster: %q", err)
	}
	ns, err := cluster.GetNodeStatus()
	if err != nil {
	contextLogger.Fatalf("Error getting node status: %q", err)
	}
	ns.PrettyPrint()
	if !ns.Ready() {
	contextLogger.Fatalf("The cluster is not completely ready.")
	}
	components, err := cluster.Health()
	if err != nil {
	contextLogger.Fatalf("Error in getting Lokomotive cluster health: %q", err)
	}
	w := tabwriter.NewWriter(os.Stdout, 0, 0, 4, ' ', 0)
	// Print the header.
	fmt.Fprintln(w, "Name\tStatus\tMessage\tError\t")
	// An empty line between header and the body.
	fmt.Fprintln(w, "\t\t\t\t")
	for _, component := range components {
	// The client-go library defines only one `ComponenetConditionType` at the moment,
	// which is `ComponentHealthy`. However, iterating over the list keeps this from
	// breaking in case client-go adds another `ComponentConditionType`.
	for _, condition := range component.Conditions {
	line := fmt.Sprintf(
	"%s\t%s\t%s\t%s\t",
	component.Name, condition.Status, condition.Message, condition.Error,
	)
	fmt.Fprintln(w, line)
	}
	w.Flush()
	}

could be changed into:

    c, err := hclConfig.WithLogger(contextLogger).Load()
    if err != nil {
      contextLogger.Fatalf("Loading cluster configuration failed: %v", err)
    }

    h, err := c.Health()
    if err != nil {
      contextLogger.Fatalf("Checking cluster health failed: %v", err)
    }

    contextLogger.Infof(h.AsTable())

#596 should also be taken into account while working on this.

[johananl]

+1 for treating cli as a "UI" package.

[invidian]

PRs #1013 #1015 #1018 and #1098 has been merged, which improved the shape of cli/cmd package a lot, but there are still some tasks, which needs to be finished before we can close this issue:

• cli/cmd/cluster now exposes an (Go) API to cli/cmd package to run cluster operations. However, the API is not granular enough to allow cli/cmd package trigger asking question for confirmation about operations. We should split methods like Apply() or ComponentDelete() so askForConfirmation can be called in cli/cmd.
• We should remove fmt.Print* calls from cli/cmd/cluster. They all should be most likely be replaced by using logger or we should again make API more granular to allow nice message printing.