Update cli subcommands to print errors when encountered (#221)

Signed-off-by: Kevin Lingerfelt <[email protected]>

cli/cmd/check.go

@@ -24,18 +24,20 @@ var checkCmd = &cobra.Command{
 local system, the Conduit control plane, and connectivity between those. The process will exit with non-zero check if
 problems were found.`,
 	Args: cobra.NoArgs,
-	Run: exitSilentlyOnError(func(cmd *cobra.Command, args []string) error {
+	Run: func(cmd *cobra.Command, args []string) {
 
 		kubectl, err := k8s.NewKubectl(shell.NewUnixShell())
 		if err != nil {
 			fmt.Fprintf(os.Stderr, "Error with kubectl: %s\n", err.Error())
-			return statusCheckResultWasError(os.Stdout)
+			statusCheckResultWasError(os.Stdout)
+			os.Exit(2)
 		}
 
 		kubeApi, err := k8s.NewK8sAPI(shell.NewUnixShell(), kubeconfigPath)
 		if err != nil {
 			fmt.Fprintf(os.Stderr, "Error with Kubernetes API: %s\n", err.Error())
-			return statusCheckResultWasError(os.Stdout)
+			statusCheckResultWasError(os.Stdout)
+			os.Exit(2)
 		}
 
 		var conduitApi pb.ApiClient
@@ -46,11 +48,15 @@ problems were found.`,
 		}
 		if err != nil {
 			fmt.Fprintf(os.Stderr, "Error with Conduit API: %s\n", err.Error())
-			return statusCheckResultWasError(os.Stdout)
+			statusCheckResultWasError(os.Stdout)
+			os.Exit(2)
 		}
 
-		return checkStatus(os.Stdout, kubectl, kubeApi, healthcheck.NewGrpcStatusChecker(public.ConduitApiSubsystemName, conduitApi))
-	}),
+		err = checkStatus(os.Stdout, kubectl, kubeApi, healthcheck.NewGrpcStatusChecker(public.ConduitApiSubsystemName, conduitApi))
+		if err != nil {
+			os.Exit(2)
+		}
+	},
 }
 
 func checkStatus(w io.Writer, checkers ...healthcheck.StatusChecker) error {

cli/cmd/completion.go

@@ -36,17 +36,14 @@ var completionCmd = &cobra.Command{
 	Example:   example,
 	Args:      cobra.ExactArgs(1),
 	ValidArgs: []string{"bash", "zsh"},
-	Run: exitSilentlyOnError(func(cmd *cobra.Command, args []string) error {
-
+	Run: func(cmd *cobra.Command, args []string) {
 		out, err := getCompletion(args[0])
 		if err != nil {
 			log.Fatal(err.Error())
 		} else {
 			fmt.Printf(out)
 		}
-
-		return err
-	}),
+	},
 }
 
 func init() {

cli/cmd/root.go

@@ -1,8 +1,6 @@
 package cmd
 
 import (
-	"os"
-
 	"github.com/runconduit/conduit/controller/api/public"
 	pb "github.com/runconduit/conduit/controller/gen/public"
 	"github.com/runconduit/conduit/pkg/k8s"
@@ -55,20 +53,3 @@ func newPublicAPIClient() (pb.ApiClient, error) {
 	}
 	return public.NewExternalClient(controlPlaneNamespace, kubeApi)
 }
-
-// Exit with non-zero exit status without printing the command line usage and
-// without printing the error message.
-//
-// When a `RunE` command returns an error, Cobra will print the usage message
-// so the `RunE` function needs to handle any non-usage errors itself without
-// returning an error. `exitSilentlyOnError` can be used as the `Run` (not
-// `RunE`) function to help with this.
-//
-// TODO: This is used by the `version` command now; it should be used by other commands too.
-func exitSilentlyOnError(f func(cmd *cobra.Command, args []string) error) func(cmd *cobra.Command, args []string) {
-	return func(cmd *cobra.Command, args []string) {
-		if err := f(cmd, args); err != nil {
-			os.Exit(2) // Reserve 1 for usage errors.
-		}
-	}
-}

cli/cmd/tap.go

@@ -38,9 +38,9 @@ var tapCmd = &cobra.Command{
 Valid targets include:
  * Pods (default/hello-world-h4fb2)
  * Deployments (default/hello-world)`,
-	Run: exitSilentlyOnError(func(cmd *cobra.Command, args []string) error {
+	RunE: func(cmd *cobra.Command, args []string) error {
 		if len(args) != 2 {
-			return errors.New("please specify a target")
+			return errors.New("please specify a resource type and target")
 		}
 
 		// We don't validate inputs because they are validated on the server.
@@ -59,7 +59,7 @@ Valid targets include:
 		friendlyNameForResourceType := strings.ToLower(args[0])
 		validatedResourceType, err := k8s.CanonicalKubernetesNameFromFriendlyName(friendlyNameForResourceType)
 		if err != nil {
-			return fmt.Errorf("unsupported resourceType [%s]", friendlyNameForResourceType)
+			return fmt.Errorf("unsupported resource type [%s]", friendlyNameForResourceType)
 		}
 
 		client, err := newPublicAPIClient()
@@ -68,7 +68,7 @@ Valid targets include:
 		}
 
 		return requestTapFromApi(os.Stdout, client, args[1], validatedResourceType, partialReq)
-	}),
+	},
 }
 
 func init() {
@@ -97,12 +97,11 @@ func requestTapFromApi(w io.Writer, client pb.ApiClient, targetName string, reso
 			Pod: targetName,
 		}
 	default:
-		return fmt.Errorf("unsupported resourceType [%s]", resourceType)
+		return fmt.Errorf("unsupported resource type [%s]", resourceType)
 	}
 
 	rsp, err := client.Tap(context.Background(), req)
 	if err != nil {
-		fmt.Fprintln(w, err.Error())
 		return err
 	}
 

cli/cmd/version.go

@@ -3,6 +3,7 @@ package cmd
 import (
 	"context"
 	"fmt"
+	"os"
 
 	pb "github.com/runconduit/conduit/controller/gen/public"
 	"github.com/runconduit/conduit/pkg/version"
@@ -16,44 +17,31 @@ var versionCmd = &cobra.Command{
 	Short: "Print the client and server version information",
 	Long:  "Print the client and server version information.",
 	Args:  cobra.NoArgs,
-	Run: exitSilentlyOnError(func(cmd *cobra.Command, args []string) error {
+	Run: func(cmd *cobra.Command, args []string) {
+		fmt.Printf("Client version: %s\n", version.Version)
+
 		client, err := newPublicAPIClient()
 		if err != nil {
-			return err
+			fmt.Fprintf(os.Stderr, "Error connecting to server: %s\n", err)
+			return
 		}
 
-		versions := getVersions(client)
-
-		fmt.Printf("Client version: %s\n", versions.Client)
-		fmt.Printf("Server version: %s\n", versions.Server)
+		fmt.Printf("Server version: %s\n", getServerVersion(client))
 
-		return err
-	}),
+		return
+	},
 }
 
 func init() {
 	RootCmd.AddCommand(versionCmd)
 	addControlPlaneNetworkingArgs(versionCmd)
 }
 
-type versions struct {
-	Server string
-	Client string
-}
-
-func getVersions(client pb.ApiClient) versions {
+func getServerVersion(client pb.ApiClient) string {
 	resp, err := client.Version(context.Background(), &pb.Empty{})
 	if err != nil {
-		return versions{
-			Client: version.Version,
-			Server: DefaultVersionString,
-		}
-	}
-
-	versions := versions{
-		Server: resp.GetReleaseVersion(),
-		Client: version.Version,
+		return DefaultVersionString
 	}
 
-	return versions
+	return resp.GetReleaseVersion()
 }

cli/cmd/version_test.go

@@ -6,37 +6,34 @@ import (
 
 	"github.com/runconduit/conduit/controller/api/public"
 	pb "github.com/runconduit/conduit/controller/gen/public"
-	"github.com/runconduit/conduit/pkg/version"
 )
 
-func TestGetVersion(t *testing.T) {
-	t.Run("Returns existing versions from server and client", func(t *testing.T) {
+func TestGetServerVersion(t *testing.T) {
+	t.Run("Returns existing version from server", func(t *testing.T) {
 		expectedServerVersion := "1.2.3"
-		expectedClientVersion := version.Version
 		mockClient := &public.MockConduitApiClient{}
 		mockClient.VersionInfoToReturn = &pb.VersionInfo{
 			ReleaseVersion: expectedServerVersion,
 		}
 
-		versions := getVersions(mockClient)
+		version := getServerVersion(mockClient)
 
-		if versions.Client != expectedClientVersion || versions.Server != expectedServerVersion {
-			t.Fatalf("Expected client version to be [%s], was [%s]; expecting server version to be [%s], was [%s]",
-				versions.Client, expectedClientVersion, versions.Server, expectedServerVersion)
+		if version != expectedServerVersion {
+			t.Fatalf("Expected server version to be [%s], was [%s]",
+				expectedServerVersion, version)
 		}
 	})
 
-	t.Run("Returns undefined when cannot get server version", func(t *testing.T) {
+	t.Run("Returns unavailable when cannot get server version", func(t *testing.T) {
 		expectedServerVersion := "unavailable"
-		expectedClientVersion := version.Version
 		mockClient := &public.MockConduitApiClient{}
 		mockClient.ErrorToReturn = errors.New("expected")
 
-		versions := getVersions(mockClient)
+		version := getServerVersion(mockClient)
 
-		if versions.Client != expectedClientVersion || versions.Server != expectedServerVersion {
-			t.Fatalf("Expected client version to be [%s], was [%s]; expecting server version to be [%s], was [%s]",
-				expectedClientVersion, versions.Client, expectedServerVersion, versions.Server)
+		if version != expectedServerVersion {
+			t.Fatalf("Expected server version to be [%s], was [%s]",
+				expectedServerVersion, version)
 		}
 	})
 }

controller/api/public/client.go

@@ -178,10 +178,9 @@ func newClient(apiURL *url.URL, httpClientToUse *http.Client) (pb.ApiClient, err
 }
 
 func NewInternalClient(kubernetesApiHost string) (pb.ApiClient, error) {
-	apiURL := &url.URL{
-		Scheme: "http",
-		Host:   kubernetesApiHost,
-		Path:   "/",
+	apiURL, err := url.Parse(fmt.Sprintf("http://%s/", kubernetesApiHost))
+	if err != nil {
+		return nil, err
 	}
 
 	return newClient(apiURL, http.DefaultClient)