Merge pull request #715 from vmware-tanzu/kctrl-ux-enhancements-1

Bunch of UX fixes tweaking output

cli/pkg/kctrl/cmd/app/app.go

@@ -4,8 +4,6 @@
 package app
 
 import (
-	"fmt"
-
 	"github.com/spf13/cobra"
 	kcv1alpha1 "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1"
 	kcpkgv1alpha1 "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/packaging/v1alpha1"
@@ -29,7 +27,30 @@ func isOwnedByPackageInstall(app *kcv1alpha1.App) bool {
 	return false
 }
 
-func getAppDescription(name string, namespace string) string {
-	description := fmt.Sprintf("app/%s (kappctrl.k14s.io/v1alpha1)  namespace: %s", name, namespace)
-	return description
+// Returns app status string and a bool indicating if it is a failure
+func appStatus(app *kcv1alpha1.App) (string, bool) {
+	if len(app.Status.Conditions) == 0 {
+		return "", false
+	}
+	if app.Spec.Canceled {
+		return "Canceled", true
+	}
+	if app.Spec.Paused {
+		return "Paused", true
+	}
+	for _, condition := range app.Status.Conditions {
+		switch condition.Type {
+		case kcv1alpha1.ReconcileFailed:
+			return "Reconcile failed", true
+		case kcv1alpha1.ReconcileSucceeded:
+			return "Reconcile succeeded", false
+		case kcv1alpha1.DeleteFailed:
+			return "Deletion failed", true
+		case kcv1alpha1.Reconciling:
+			return "Reconciling", false
+		case kcv1alpha1.Deleting:
+			return "Deleting", false
+		}
+	}
+	return app.Status.FriendlyDescription, false
 }

cli/pkg/kctrl/cmd/app/app_tailer.go

@@ -112,7 +112,7 @@ func (o *AppTailer) printUpdate(oldStatus kcv1alpha1.AppStatus, status kcv1alpha
 	}
 
 	if o.hasReconciled(status) {
-		o.statusUI.PrintLogLine("App reconciled", "", false, status.Deploy.UpdatedAt.Time)
+		o.statusUI.PrintLogLine("Deploy succeeded", "", false, status.Deploy.UpdatedAt.Time)
 		o.stopWatch(false)
 	}
 	failed, errMsg := o.hasFailed(status)
@@ -123,6 +123,7 @@ func (o *AppTailer) printUpdate(oldStatus kcv1alpha1.AppStatus, status kcv1alpha
 }
 
 func (o *AppTailer) printInfo(app kcv1alpha1.App) {
+	status, isFailing := appStatus(&app)
 	table := uitable.Table{
 		Transpose: true,
 
@@ -137,7 +138,7 @@ func (o *AppTailer) printInfo(app kcv1alpha1.App) {
 		Rows: [][]uitable.Value{{
 			uitable.NewValueString(app.Name),
 			uitable.NewValueString(app.Namespace),
-			uitable.NewValueString(o.statusString(app.Status)),
+			uitable.ValueFmt{V: uitable.NewValueString(status), Error: isFailing},
 			uitable.NewValueString(o.metricString(app.Status)),
 		}},
 	}
@@ -155,27 +156,6 @@ func (o *AppTailer) metricString(status kcv1alpha1.AppStatus) string {
 	}
 }
 
-func (o *AppTailer) statusString(status kcv1alpha1.AppStatus) string {
-	if len(status.Conditions) < 1 {
-		return ""
-	}
-	for _, condition := range status.Conditions {
-		switch condition.Type {
-		case kcv1alpha1.ReconcileFailed:
-			return color.RedString("Reconcile failed")
-		case kcv1alpha1.ReconcileSucceeded:
-			return color.GreenString("Reconcile succeeded")
-		case kcv1alpha1.DeleteFailed:
-			return color.RedString("Deletion failed")
-		case kcv1alpha1.Reconciling:
-			return "Reconciling"
-		case kcv1alpha1.Deleting:
-			return "Deleting"
-		}
-	}
-	return status.FriendlyDescription
-}
-
 func (o *AppTailer) hasReconciled(status kcv1alpha1.AppStatus) bool {
 	for _, condition := range status.Conditions {
 		if condition.Type == kcv1alpha1.ReconcileSucceeded && condition.Status == corev1.ConditionTrue {

cli/pkg/kctrl/cmd/app/get.go

@@ -14,7 +14,6 @@ import (
 	cmdcore "github.com/vmware-tanzu/carvel-kapp-controller/cli/pkg/kctrl/cmd/core"
 	"github.com/vmware-tanzu/carvel-kapp-controller/cli/pkg/kctrl/logger"
 	kcv1alpha1 "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1"
-	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -60,12 +59,12 @@ func (o *GetOptions) Run() error {
 		return err
 	}
 
-	isFailing := o.isFailing(app.Status.Conditions)
+	status, isFailing := appStatus(app)
 
 	failingStageHeader := uitable.NewHeader("Failing Stage")
-	failingStageHeader.Hidden = !isFailing
+	failingStageHeader.Hidden = len(app.Status.UsefulErrorMessage) == 0
 	errorMessageHeader := uitable.NewHeader("Useful Error Message")
-	errorMessageHeader.Hidden = !isFailing
+	errorMessageHeader.Hidden = len(app.Status.UsefulErrorMessage) == 0
 
 	table := uitable.Table{
 		Transpose: true,
@@ -85,7 +84,7 @@ func (o *GetOptions) Run() error {
 			uitable.NewValueString(app.Namespace),
 			uitable.NewValueString(app.Name),
 			uitable.NewValueString(app.Spec.ServiceAccountName),
-			uitable.NewValueString(app.Status.FriendlyDescription),
+			uitable.ValueFmt{V: uitable.NewValueString(status), Error: isFailing},
 			uitable.NewValueInterface(o.formatOwnerReferences(app.OwnerReferences)),
 			uitable.NewValueInterface(app.Status.Conditions),
 			uitable.NewValueString(o.failingStage(app.Status)),
@@ -108,15 +107,6 @@ func (o *GetOptions) formatOwnerReferences(references []metav1.OwnerReference) [
 	return referenceList
 }
 
-func (o *GetOptions) isFailing(conditions []kcv1alpha1.AppCondition) bool {
-	for _, condition := range conditions {
-		if condition.Type == kcv1alpha1.ReconcileFailed && condition.Status == corev1.ConditionTrue {
-			return true
-		}
-	}
-	return false
-}
-
 // TODO: Do we need to check observed generation here?
 func (o *GetOptions) failingStage(status kcv1alpha1.AppStatus) string {
 	if status.Fetch.ExitCode != 0 {

cli/pkg/kctrl/cmd/app/list.go

@@ -32,7 +32,7 @@ func NewListOptions(ui ui.UI, depsFactory cmdcore.DepsFactory, logger logger.Log
 func NewListCmd(o *ListOptions, flagsFactory cmdcore.FlagsFactory) *cobra.Command {
 	cmd := &cobra.Command{
 		Use:     "list",
-		Aliases: []string{"g"},
+		Aliases: []string{"l", "ls"},
 		Short:   "List Apps",
 		RunE:    func(_ *cobra.Command, _ []string) error { return o.Run() },
 	}
@@ -83,6 +83,7 @@ func (o *ListOptions) Run() error {
 	}
 
 	for _, app := range appList.Items {
+		status, isFailing := appStatus(&app)
 		sinceDeployAge := cmdcore.NewValueAge(time.Time{})
 		if app.Status.Deploy != nil {
 			sinceDeployAge = cmdcore.NewValueAge(app.Status.Deploy.UpdatedAt.Time)
@@ -91,7 +92,7 @@ func (o *ListOptions) Run() error {
 		table.Rows = append(table.Rows, []uitable.Value{
 			cmdcore.NewValueNamespace(app.Namespace),
 			uitable.NewValueString(app.Name),
-			uitable.NewValueString(app.Status.FriendlyDescription),
+			uitable.ValueFmt{V: uitable.NewValueString(status), Error: isFailing},
 			sinceDeployAge,
 			uitable.NewValueString(o.owner(app.OwnerReferences)),
 			cmdcore.NewValueAge(app.CreationTimestamp.Time),

cli/pkg/kctrl/cmd/package/available/get.go

@@ -100,7 +100,7 @@ func (o *GetOptions) Run(args []string) error {
 
 	if o.ValuesSchema {
 		if pkgVersion == "" {
-			return fmt.Errorf("Package version is required when --values-schema flag is declared")
+			return fmt.Errorf("Package version is required when --values-schema flag is declared (hint: to specify a particular version use the format: '-p <package-name>/<version>')")
 		}
 		return o.showValuesSchema(client, pkgName, pkgVersion)
 	}
@@ -180,7 +180,7 @@ func (o *GetOptions) show(client pkgclient.Interface, pkgName, pkgVersion string
 		}...)
 	} else {
 		if len(o.DefaultValuesFile) > 0 {
-			return fmt.Errorf("Package version is required when --default-values-file-output flag is declared")
+			return fmt.Errorf("Package version is required when --default-values-file-output flag is declared (hint: to specify a particular version use the format: '-p <package-name>/<version>')")
 		}
 		listOpts := metav1.ListOptions{}
 		if len(o.Name) > 0 {

cli/pkg/kctrl/cmd/package/installed/create_or_update.go

@@ -88,7 +88,7 @@ func NewCreateCmd(o *CreateOrUpdateOptions, flagsFactory cmdcore.FlagsFactory) *
 	}
 
 	cmd.Flags().StringVarP(&o.packageName, "package", "p", "", "Set package name (required)")
-	cmd.Flags().StringVar(&o.version, "version", "", "Set package version (required)")
+	cmd.Flags().StringVarP(&o.version, "version", "v", "", "Set package version (required)")
 	cmd.Flags().StringVar(&o.serviceAccountName, "service-account-name", "", "Name of an existing service account used to install underlying package contents, optional")
 	cmd.Flags().StringVar(&o.valuesFile, "values-file", "", "The path to the configuration values file, optional")
 	cmd.Flags().BoolVar(&o.values, "values", true, "Add or keep values supplied to package install, optional")
@@ -129,7 +129,7 @@ func NewInstallCmd(o *CreateOrUpdateOptions, flagsFactory cmdcore.FlagsFactory)
 	}
 
 	cmd.Flags().StringVarP(&o.packageName, "package", "p", "", "Set package name (required)")
-	cmd.Flags().StringVar(&o.version, "version", "", "Set package version (required)")
+	cmd.Flags().StringVarP(&o.version, "version", "v", "", "Set package version (required)")
 	cmd.Flags().StringVar(&o.serviceAccountName, "service-account-name", "", "Name of an existing service account used to install underlying package contents, optional")
 	cmd.Flags().StringVar(&o.valuesFile, "values-file", "", "The path to the configuration values file, optional")
 	cmd.Flags().BoolVar(&o.values, "values", true, "Add or keep values supplied to package install, optional")
@@ -169,7 +169,7 @@ func NewUpdateCmd(o *CreateOrUpdateOptions, flagsFactory cmdcore.FlagsFactory) *
 	}
 
 	cmd.Flags().StringVarP(&o.packageName, "package", "p", "", "Name of package install to be updated")
-	cmd.Flags().StringVar(&o.version, "version", "", "Set package version")
+	cmd.Flags().StringVarP(&o.version, "version", "v", "", "Set package version")
 	cmd.Flags().StringVar(&o.valuesFile, "values-file", "", "The path to the configuration values file, optional")
 	cmd.Flags().BoolVar(&o.values, "values", true, "Add or keep values supplied to package install, optional")
 
@@ -201,7 +201,10 @@ func (o *CreateOrUpdateOptions) RunCreate(args []string) error {
 			return err
 		}
 
-		o.showVersions(pkgClient)
+		err = o.showVersions(pkgClient)
+		if err != nil {
+			return err
+		}
 		return fmt.Errorf("Expected package version to be non empty")
 	}
 
@@ -385,7 +388,7 @@ func (o *CreateOrUpdateOptions) dropValuesSecret(client kubernetes.Interface) (b
 	valuesSecret, err := client.CoreV1().Secrets(o.NamespaceFlags.Name).Get(context.Background(), secretName, metav1.GetOptions{})
 	if err != nil {
 		if errors.IsNotFound(err) {
-			o.statusUI.PrintMessagef("Values secret '%s' not found", secretName)
+			o.statusUI.PrintMessagef("Values secret '%s' not found in namespace '%s'", secretName, o.NamespaceFlags.Name)
 			return true, nil
 		}
 		return false, fmt.Errorf("Getting values secret: %s", err.Error())
@@ -926,6 +929,10 @@ func (o *CreateOrUpdateOptions) showVersions(client pkgclient.Interface) error {
 		return err
 	}
 
+	if len(pkgList.Items) == 0 {
+		return fmt.Errorf("No versions of package '%s' found in namespace '%s'", o.packageName, o.NamespaceFlags.Name)
+	}
+
 	table := uitable.Table{
 		Title: fmt.Sprintf("Available Versions of %s", o.packageName),
 		Header: []uitable.Header{

cli/pkg/kctrl/cmd/package/installed/get.go

@@ -15,6 +15,7 @@ import (
 	"github.com/spf13/cobra"
 	cmdcore "github.com/vmware-tanzu/carvel-kapp-controller/cli/pkg/kctrl/cmd/core"
 	"github.com/vmware-tanzu/carvel-kapp-controller/cli/pkg/kctrl/logger"
+	kcv1alpha1 "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1"
 	kcpkgv1alpha1 "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/packaging/v1alpha1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -107,6 +108,8 @@ func (o *GetOptions) Run(args []string) error {
 		return nil
 	}
 
+	status, isFailing := packageInstallStatus(pkgi)
+
 	errorMessageHeader := uitable.NewHeader("Useful Error Message")
 	errorMessageHeader.Hidden = len(pkgi.Status.UsefulErrorMessage) == 0
 
@@ -128,7 +131,7 @@ func (o *GetOptions) Run(args []string) error {
 			uitable.NewValueString(pkgi.Name),
 			uitable.NewValueString(pkgi.Spec.PackageRef.RefName),
 			uitable.NewValueString(pkgi.Status.Version),
-			uitable.NewValueString(packageInstallStatus(pkgi)),
+			uitable.ValueFmt{V: uitable.NewValueString(status), Error: isFailing},
 			uitable.NewValueInterface(pkgi.Status.Conditions),
 			uitable.NewValueString(color.RedString(pkgi.Status.UsefulErrorMessage)),
 		}},
@@ -140,6 +143,9 @@ func (o *GetOptions) Run(args []string) error {
 }
 
 func (o *GetOptions) getSecretData(pkgi *kcpkgv1alpha1.PackageInstall) ([]byte, error) {
+	if len(pkgi.Spec.Values) == 0 {
+		return nil, fmt.Errorf("No values have been supplied to package installation '%s' in namespace '%s'", o.Name, o.NamespaceFlags.Name)
+	}
 
 	if len(pkgi.Spec.Values) != 1 {
 		return nil, fmt.Errorf("Expected 1 values reference, found %d", len(pkgi.Spec.Values))
@@ -208,12 +214,28 @@ func (o *GetOptions) showValuesData(pkgi *kcpkgv1alpha1.PackageInstall) error {
 	return nil
 }
 
-func packageInstallStatus(pkgi *kcpkgv1alpha1.PackageInstall) string {
+// Returns pkgi status string and a bool indicating if it is a failure
+func packageInstallStatus(pkgi *kcpkgv1alpha1.PackageInstall) (string, bool) {
 	if pkgi.Spec.Canceled {
-		return "Canceled"
+		return "Canceled", true
 	}
 	if pkgi.Spec.Paused {
-		return "Paused"
+		return "Paused", true
+	}
+
+	for _, condition := range pkgi.Status.Conditions {
+		switch condition.Type {
+		case kcv1alpha1.ReconcileFailed:
+			return "Reconcile failed", true
+		case kcv1alpha1.ReconcileSucceeded:
+			return "Reconcile succeeded", false
+		case kcv1alpha1.DeleteFailed:
+			return "Deletion failed", true
+		case kcv1alpha1.Reconciling:
+			return "Reconciling", false
+		case kcv1alpha1.Deleting:
+			return "Deleting", false
+		}
 	}
-	return pkgi.Status.FriendlyDescription
+	return pkgi.Status.FriendlyDescription, false
 }

cli/pkg/kctrl/cmd/package/installed/list.go

@@ -41,7 +41,7 @@ func NewListCmd(o *ListOptions, flagsFactory cmdcore.FlagsFactory) *cobra.Comman
 				[]string{"package", "installed", "list"},
 			},
 			cmdcore.Example{"List installed packages in all namespaces",
-				[]string{"package", "installed", "list", "A"}},
+				[]string{"package", "installed", "list", "-A"}},
 		}.Description("", o.pkgCmdTreeOpts),
 		SilenceUsage: true,
 		Annotations:  map[string]string{"table": ""},
@@ -91,12 +91,13 @@ func (o *ListOptions) Run() error {
 	}
 
 	for _, pkgi := range pkgiList.Items {
+		status, isFailing := packageInstallStatus(&pkgi)
 		table.Rows = append(table.Rows, []uitable.Value{
 			cmdcore.NewValueNamespace(pkgi.Namespace),
 			uitable.NewValueString(pkgi.Name),
 			uitable.NewValueString(pkgi.Spec.PackageRef.RefName),
 			uitable.NewValueString(pkgi.Status.Version),
-			uitable.NewValueString(packageInstallStatus(&pkgi)),
+			uitable.ValueFmt{V: uitable.NewValueString(status), Error: isFailing},
 		})
 	}
 

cli/pkg/kctrl/cmd/package/repository/add_or_update.go

@@ -20,6 +20,7 @@ import (
 	kcclient "github.com/vmware-tanzu/carvel-kapp-controller/pkg/client/clientset/versioned"
 	versions "github.com/vmware-tanzu/carvel-vendir/pkg/vendir/versions/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 )
@@ -132,7 +133,7 @@ func (o *AddOrUpdateOptions) Run(args []string) error {
 	existingRepository, err := client.PackagingV1alpha1().PackageRepositories(o.NamespaceFlags.Name).Get(
 		context.Background(), o.Name, metav1.GetOptions{})
 	if err != nil {
-		if strings.Contains(err.Error(), "not found") && o.CreateRepository {
+		if errors.IsNotFound(err) && o.CreateRepository {
 			return o.add(client)
 		}
 		return err

cli/test/e2e/app_commands_test.go

@@ -99,7 +99,7 @@ spec:
 
 		require.Contains(t, out, "Fetch succeeded")
 		require.Contains(t, out, "Template succeeded")
-		require.Contains(t, out, "App reconciled")
+		require.Contains(t, out, "Deploy succeeded")
 	})
 
 	logger.Section("pause app", func() {
@@ -116,7 +116,7 @@ spec:
 			"namespace":        "kctrl-test",
 			"owner_references": "",
 			"service_account":  "kappctrl-e2e-ns-sa",
-			"status":           "Canceled/paused",
+			"status":           "Paused",
 		}}
 		require.Exactly(t, expectedOutputRows, replaceAgeAndSinceDeployed(output.Tables[0].Rows))
 	})

cli/test/e2e/package_install_test.go

@@ -152,7 +152,7 @@ spec:
 
 		require.Contains(t, out, "Fetch succeeded")
 		require.Contains(t, out, "Template succeeded")
-		require.Contains(t, out, "App reconciled")
+		require.Contains(t, out, "Deploy succeeded")
 	})
 
 	logger.Section("package installed update", func() {
@@ -209,7 +209,7 @@ spec:
 
 		require.Contains(t, out, "Fetch succeeded")
 		require.Contains(t, out, "Template succeeded")
-		require.Contains(t, out, "App reconciled")
+		require.Contains(t, out, "Deploy succeeded")
 	})
 
 	logger.Section("package install delete", func() {