Human-readable revision describe (#475)

* Revision describe rebaseable

* Fix up for rebase

* Some tests

* moretests

* Forgot part of the refactor!

* Fix unit tests

* Change e2e tests and respond to feedback

docs/cmd/kn_revision_describe.md

@@ -16,8 +16,9 @@ kn revision describe NAME [flags]
       --allow-missing-template-keys   If true, ignore any errors in templates when a field or map key is missing in the template. Only applies to golang and jsonpath output formats. (default true)
   -h, --help                          help for describe
   -n, --namespace string              Specify the namespace to operate in.
-  -o, --output string                 Output format. One of: json|yaml|name|go-template|go-template-file|template|templatefile|jsonpath|jsonpath-file. (default "yaml")
+  -o, --output string                 Output format. One of: json|yaml|name|go-template|go-template-file|template|templatefile|jsonpath|jsonpath-file.
       --template string               Template string or path to template file to use when -o=go-template, -o=go-template-file. The template format is golang templates [http://golang.org/pkg/text/template/#pkg-overview].
+  -v, --verbose                       More output.
 ```
 
 ### Options inherited from parent commands

pkg/kn/commands/describe.go

@@ -34,8 +34,8 @@ const TruncateAt = 100
 func WriteMetadata(dw printers.PrefixWriter, m *metav1.ObjectMeta, printDetails bool) {
 	dw.WriteAttribute("Name", m.Name)
 	dw.WriteAttribute("Namespace", m.Namespace)
-	WriteMapDesc(dw, m.Labels, l("Labels"), "", printDetails)
-	WriteMapDesc(dw, m.Annotations, l("Annotations"), "", printDetails)
+	WriteMapDesc(dw, m.Labels, "Labels", printDetails)
+	WriteMapDesc(dw, m.Annotations, "Annotations", printDetails)
 	dw.WriteAttribute("Age", Age(m.CreationTimestamp.Time))
 }
 
@@ -52,7 +52,7 @@ func keyIsBoring(k string) bool {
 
 // Write a map either compact in a single line (possibly truncated) or, if printDetails is set,
 // over multiple line, one line per key-value pair. The output is sorted by keys.
-func WriteMapDesc(dw printers.PrefixWriter, m map[string]string, label string, labelPrefix string, details bool) {
+func WriteMapDesc(dw printers.PrefixWriter, m map[string]string, label string, details bool) {
 	if len(m) == 0 {
 		return
 	}
@@ -69,16 +69,17 @@ func WriteMapDesc(dw printers.PrefixWriter, m map[string]string, label string, l
 	sort.Strings(keys)
 
 	if details {
-		l := labelPrefix + label
-
-		for _, key := range keys {
+		for i, key := range keys {
+			l := ""
+			if i == 0 {
+				l = printers.Label(label)
+			}
 			dw.WriteColsLn(l, key+"="+m[key])
-			l = labelPrefix
 		}
 		return
 	}
 
-	dw.WriteColsLn(label, joinAndTruncate(keys, m, TruncateAt-len(label)-2))
+	dw.WriteColsLn(printers.Label(label), joinAndTruncate(keys, m, TruncateAt-len(label)-2))
 }
 
 func Age(t time.Time) string {
@@ -180,9 +181,30 @@ func WriteConditions(dw printers.PrefixWriter, conditions []apis.Condition, prin
 	}
 }
 
-// Format label (extracted so that color could be added more easily to all labels)
-func l(label string) string {
-	return label + ":"
+// Writer a slice compact (printDetails == false) in one line, or over multiple line
+// with key-value line-by-line (printDetails == true)
+func WriteSliceDesc(dw printers.PrefixWriter, s []string, label string, printDetails bool) {
+
+	if len(s) == 0 {
+		return
+	}
+
+	if printDetails {
+		for i, value := range s {
+			if i == 0 {
+				dw.WriteColsLn(printers.Label(label), value)
+			} else {
+				dw.WriteColsLn("", value)
+			}
+		}
+		return
+	}
+
+	joined := strings.Join(s, ", ")
+	if len(joined) > TruncateAt {
+		joined = joined[:TruncateAt-4] + " ..."
+	}
+	dw.WriteAttribute(label, joined)
 }
 
 // Join to key=value pair, comma separated, and truncate if longer than a limit

pkg/kn/commands/describe_test.go

@@ -36,15 +36,15 @@ var testMap = map[string]string{
 func TestWriteMapDesc(t *testing.T) {
 	buf := &bytes.Buffer{}
 	dw := printers.NewBarePrefixWriter(buf)
-	WriteMapDesc(dw, testMap, "eggs", "", false)
-	assert.Equal(t, buf.String(), "eggs\ta=b, c=d, foo=bar\n")
+	WriteMapDesc(dw, testMap, "eggs", false)
+	assert.Equal(t, buf.String(), "eggs:\ta=b, c=d, foo=bar\n")
 }
 
 func TestWriteMapDescDetailed(t *testing.T) {
 	buf := &bytes.Buffer{}
 	dw := printers.NewBarePrefixWriter(buf)
-	WriteMapDesc(dw, testMap, "eggs", "", true)
-	assert.Equal(t, buf.String(), "eggs\ta=b\n\tc=d\n\tfoo=bar\n\tserving.knative.dev/funky=chicken\n")
+	WriteMapDesc(dw, testMap, "eggs", true)
+	assert.Equal(t, buf.String(), "eggs:\ta=b\n\tc=d\n\tfoo=bar\n\tserving.knative.dev/funky=chicken\n")
 }
 
 func TestWriteMapTruncated(t *testing.T) {
@@ -55,7 +55,7 @@ func TestWriteMapTruncated(t *testing.T) {
 	for i := 0; i < 1000; i++ {
 		items[strconv.Itoa(i)] = strconv.Itoa(i + 1)
 	}
-	WriteMapDesc(dw, items, "eggs", "", false)
+	WriteMapDesc(dw, items, "eggs", false)
 	assert.Assert(t, len(strings.TrimSpace(buf.String())) <= TruncateAt)
 }
 

pkg/kn/commands/revision/describe.go

@@ -16,15 +16,31 @@ package revision
 
 import (
 	"errors"
+	"fmt"
+	"io"
+	"regexp"
+	"strconv"
+	"strings"
 
 	"github.com/spf13/cobra"
+	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 	"knative.dev/client/pkg/kn/commands"
+	"knative.dev/client/pkg/printers"
+	clientserving "knative.dev/client/pkg/serving"
+	servingserving "knative.dev/serving/pkg/apis/serving"
+	"knative.dev/serving/pkg/apis/serving/v1alpha1"
 )
 
+// Matching image digest
+var imageDigestRegexp = regexp.MustCompile(`(?i)sha256:([0-9a-f]{64})`)
+
 func NewRevisionDescribeCommand(p *commands.KnParams) *cobra.Command {
-	revisionDescribePrintFlags := genericclioptions.NewPrintFlags("").WithDefaultOutput("yaml")
-	revisionDescribeCmd := &cobra.Command{
+
+	// For machine readable output
+	machineReadablePrintFlags := genericclioptions.NewPrintFlags("")
+
+	command := &cobra.Command{
 		Use:   "describe NAME",
 		Short: "Describe revisions.",
 		RunE: func(cmd *cobra.Command, args []string) error {
@@ -46,19 +62,229 @@ func NewRevisionDescribeCommand(p *commands.KnParams) *cobra.Command {
 				return err
 			}
 
-			printer, err := revisionDescribePrintFlags.ToPrinter()
-			if err != nil {
-				return err
+			if machineReadablePrintFlags.OutputFlagSpecified() {
+				printer, err := machineReadablePrintFlags.ToPrinter()
+				if err != nil {
+					return err
+				}
+				return printer.PrintObj(revision, cmd.OutOrStdout())
 			}
-
-			err = printer.PrintObj(revision, cmd.OutOrStdout())
+			printDetails, err := cmd.Flags().GetBool("verbose")
 			if err != nil {
 				return err
 			}
-			return nil
+			var service *v1alpha1.Service
+			serviceName, ok := revision.Labels[servingserving.ServiceLabelKey]
+			if printDetails && ok {
+				service, err = client.GetService(serviceName)
+				if err != nil {
+					return err
+				}
+			}
+			// Do the human-readable printing thing.
+			return describe(cmd.OutOrStdout(), revision, service, printDetails)
 		},
 	}
-	commands.AddNamespaceFlags(revisionDescribeCmd.Flags(), false)
-	revisionDescribePrintFlags.AddFlags(revisionDescribeCmd)
-	return revisionDescribeCmd
+	flags := command.Flags()
+	commands.AddNamespaceFlags(flags, false)
+	machineReadablePrintFlags.AddFlags(command)
+	flags.BoolP("verbose", "v", false, "More output.")
+	return command
+}
+
+func describe(w io.Writer, revision *v1alpha1.Revision, service *v1alpha1.Service, printDetails bool) error {
+	dw := printers.NewPrefixWriter(w)
+	commands.WriteMetadata(dw, &revision.ObjectMeta, printDetails)
+	WriteImage(dw, revision)
+	WritePort(dw, revision)
+	WriteEnv(dw, revision, printDetails)
+	WriteScale(dw, revision)
+	WriteConcurrencyOptions(dw, revision)
+	WriteResources(dw, revision)
+	serviceName, ok := revision.Labels[servingserving.ServiceLabelKey]
+	if ok {
+		serviceSection := dw.WriteAttribute("Service", serviceName)
+		if printDetails {
+			serviceSection.WriteAttribute("Configuration Generation", revision.Labels[servingserving.ConfigurationGenerationLabelKey])
+			serviceSection.WriteAttribute("Latest Created", strconv.FormatBool(revision.Name == service.Status.LatestCreatedRevisionName))
+			serviceSection.WriteAttribute("Latest Ready", strconv.FormatBool(revision.Name == service.Status.LatestReadyRevisionName))
+			percent, tags := trafficForRevision(revision.Name, service)
+			if percent != 0 {
+				serviceSection.WriteAttribute("Traffic", strconv.FormatInt(int64(percent), 10)+"%")
+			}
+			if len(tags) > 0 {
+				commands.WriteSliceDesc(serviceSection, tags, "Tags", printDetails)
+			}
+		}
+
+	}
+	dw.WriteLine()
+	commands.WriteConditions(dw, revision.Status.Conditions, printDetails)
+	if err := dw.Flush(); err != nil {
+		return err
+	}
+	return nil
+}
+
+func WriteConcurrencyOptions(dw printers.PrefixWriter, revision *v1alpha1.Revision) {
+	target := clientserving.ConcurrencyTarget(&revision.ObjectMeta)
+	limit := revision.Spec.ContainerConcurrency
+	if target != nil || limit != nil && *limit != 0 {
+		section := dw.WriteAttribute("Concurrency", "")
+		if limit != nil && *limit != 0 {
+			section.WriteAttribute("Limit", strconv.FormatInt(int64(*limit), 10))
+		}
+		if target != nil {
+			section.WriteAttribute("Target", strconv.Itoa(*target))
+		}
+	}
+}
+
+// Write the image attribute (with
+func WriteImage(dw printers.PrefixWriter, revision *v1alpha1.Revision) {
+	c, err := clientserving.ContainerOfRevisionSpec(&revision.Spec)
+	if err != nil {
+		dw.WriteAttribute("Image", "Unknown")
+		return
+	}
+	image := c.Image
+	// Check if the user image is likely a more user-friendly description
+	pinnedDesc := "at"
+	userImage := clientserving.UserImage(&revision.ObjectMeta)
+	imageDigest := revision.Status.ImageDigest
+	if userImage != "" && imageDigest != "" {
+		var parts []string
+		if strings.Contains(image, "@") {
+			parts = strings.Split(image, "@")
+		} else {
+			parts = strings.Split(image, ":")
+		}
+		// Check if the user image refers to the same thing.
+		if strings.HasPrefix(userImage, parts[0]) {
+			pinnedDesc = "pinned to"
+			image = userImage
+		}
+	}
+	if imageDigest != "" {
+		image = fmt.Sprintf("%s (%s %s)", image, pinnedDesc, shortenDigest(imageDigest))
+	}
+	dw.WriteAttribute("Image", image)
+}
+
+func WritePort(dw printers.PrefixWriter, revision *v1alpha1.Revision) {
+	port := clientserving.Port(&revision.Spec)
+	if port != nil {
+		dw.WriteAttribute("Port", strconv.FormatInt(int64(*port), 10))
+	}
+}
+
+func WriteEnv(dw printers.PrefixWriter, revision *v1alpha1.Revision, printDetails bool) {
+	env := stringifyEnv(revision)
+	if env != nil {
+		commands.WriteSliceDesc(dw, env, "Env", printDetails)
+	}
+}
+
+func WriteScale(dw printers.PrefixWriter, revision *v1alpha1.Revision) {
+	// Scale spec if given
+	scale, err := clientserving.ScalingInfo(&revision.ObjectMeta)
+	if err != nil {
+		dw.WriteAttribute("Scale", fmt.Sprintf("Misformatted: %v", err))
+	}
+	if scale != nil && (scale.Max != nil || scale.Min != nil) {
+		dw.WriteAttribute("Scale", formatScale(scale.Min, scale.Max))
+	}
+}
+
+func WriteResources(dw printers.PrefixWriter, r *v1alpha1.Revision) {
+	c, err := clientserving.ContainerOfRevisionSpec(&r.Spec)
+	if err != nil {
+		return
+	}
+	requests := c.Resources.Requests
+	limits := c.Resources.Limits
+	writeResourcesHelper(dw, "Memory", requests.Memory(), limits.Memory())
+	writeResourcesHelper(dw, "CPU", requests.Cpu(), limits.Cpu())
+}
+
+// Write request ... limits or only one of them
+func writeResourcesHelper(dw printers.PrefixWriter, label string, request *resource.Quantity, limit *resource.Quantity) {
+	value := ""
+	if !request.IsZero() && !limit.IsZero() {
+		value = request.String() + " ... " + limit.String()
+	} else if !request.IsZero() {
+		value = request.String()
+	} else if !limit.IsZero() {
+		value = limit.String()
+	}
+
+	if value == "" {
+		return
+	}
+
+	dw.WriteAttribute(label, value)
+}
+
+// Extract pure sha sum and shorten to 8 digits,
+// as the digest should to be user consumable. Use the resource via `kn service get`
+// to get to the full sha
+func shortenDigest(digest string) string {
+	match := imageDigestRegexp.FindStringSubmatch(digest)
+	if len(match) > 1 {
+		return string(match[1][:6])
+	}
+	return digest
+}
+
+// Format scale in the format "min ... max" with max = ∞ if not set
+func formatScale(minScale *int, maxScale *int) string {
+	ret := "0"
+	if minScale != nil {
+		ret = strconv.Itoa(*minScale)
+	}
+
+	ret += " ... "
+
+	if maxScale != nil {
+		ret += strconv.Itoa(*maxScale)
+	} else {
+		ret += "∞"
+	}
+	return ret
+}
+
+func stringifyEnv(revision *v1alpha1.Revision) []string {
+	container, err := clientserving.ContainerOfRevisionSpec(&revision.Spec)
+	if err != nil {
+		return nil
+	}
+
+	envVars := make([]string, 0, len(container.Env))
+	for _, env := range container.Env {
+		value := env.Value
+		if env.ValueFrom != nil {
+			value = "[ref]"
+		}
+		envVars = append(envVars, fmt.Sprintf("%s=%s", env.Name, value))
+	}
+	return envVars
+}
+
+func trafficForRevision(name string, service *v1alpha1.Service) (int64, []string) {
+	if len(service.Status.Traffic) == 0 {
+		return 0, nil
+	}
+	var percent int64
+	tags := []string{}
+	for _, target := range service.Status.Traffic {
+		if target.RevisionName == name {
+			if target.Percent != nil {
+				percent += *target.Percent
+			}
+			if target.Tag != "" {
+				tags = append(tags, target.Tag)
+			}
+		}
+	}
+	return percent, tags
 }