Fix error when output is set to name

[daisy-ycguo]

Describe

Fix the error when calling kn service list -o name. Because the default name printer only supports unstructured.UnstructuredList, we have to convert servingv1.ServiceList to unstructured.UnstructuredList when output flag is set.

Changes

• Convert servingv1.ServiceList to unstructured.UnstructuredList when output flag is set in kn service list -o name

Reference

Fixes #754

[navidshaikh]

see if you can reuse https://github.com/knative/pkg/blob/master/apis/duck/unstructured.go#L32

[daisy-ycguo]

Thanks for the information.
I checked pkg/apis/duck, and noticed it only accepts an interface duck.OneOfOurs. Unfortunately, neither servingv1.ServiceList nor servingv1.Service , runtime.Object cannot satisfy that interface. So I cannot leverage it.

[maximilien]

See feedback

[maximilien]

s/unstructedList/unstructuredList/

[daisy-ycguo]

changed in the new version.

[maximilien]

Where is the unit test for this ToUnstructuredList exported function?

[daisy-ycguo]

see TestToUnstructuredList in pkg/util/unstructured_test.go

[navidshaikh]

We can solve this and also improve the list printing for all command groups.
If we pull the following check earlier in pkg/kn/commands/flags/listprint.go

			if serviceListFlags.GenericPrintFlags.OutputFlagSpecified() {
				unstructedList, err := util.ToUnstructuredList(serviceList)

and add another method Print to ListPrintFlags taking care of finding correct printer and above check+conversion,
we solve this for other list commands too.
I tried doing ^^ on your PR and replaced the print call for revision list (can be done in similar way for route.. etc)

➜  client git:(f5a4863b) ✗ git --no-pager diff .
diff --git a/pkg/kn/commands/flags/listprint.go b/pkg/kn/commands/flags/listprint.go
index a19cca49..2ec1c81d 100644
--- a/pkg/kn/commands/flags/listprint.go
+++ b/pkg/kn/commands/flags/listprint.go
@@ -15,11 +15,15 @@
 package flags
 
 import (
+	"io"
+
 	"github.com/spf13/cobra"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 
 	"knative.dev/client/pkg/kn/commands"
 	hprinters "knative.dev/client/pkg/printers"
+	"knative.dev/client/pkg/util"
 )
 
 // ListFlags composes common printer flag structs
@@ -56,6 +60,23 @@ func (f *ListPrintFlags) ToPrinter() (hprinters.ResourcePrinter, error) {
 	return p, nil
 }
 
+func (f *ListPrintFlags) Print(obj runtime.Object, w io.Writer) error {
+	printer, err := f.ToPrinter()
+	if err != nil {
+		return err
+	}
+
+	if f.GenericPrintFlags.OutputFlagSpecified() {
+		unstructedList, err := util.ToUnstructuredList(obj)
+		if err != nil {
+			return err
+		}
+		return printer.PrintObj(unstructedList, w)
+	}
+
+	return printer.PrintObj(obj, w)
+}
+
 // AddFlags receives a *cobra.Command reference and binds
 // flags related to humanreadable and template printing.
 func (f *ListPrintFlags) AddFlags(cmd *cobra.Command) {
diff --git a/pkg/kn/commands/revision/list.go b/pkg/kn/commands/revision/list.go
index 88f733f5..3821d76e 100644
--- a/pkg/kn/commands/revision/list.go
+++ b/pkg/kn/commands/revision/list.go
@@ -102,13 +102,7 @@ func NewRevisionListCommand(p *commands.KnParams) *cobra.Command {
 			// Sort revisions by namespace, service, generation (in this order)
 			sortRevisions(revisionList)
 
-			// Print out infos via printer framework
-			printer, err := revisionListFlags.ToPrinter()
-			if err != nil {
-				return err
-			}
-
-			return printer.PrintObj(revisionList, cmd.OutOrStdout())
+			return revisionListFlags.Print(revisionList, cmd.OutOrStdout())
 		},
 	}
 	commands.AddNamespaceFlags(revisionListCommand.Flags(), true)
diff --git a/pkg/kn/commands/service/list.go b/pkg/kn/commands/service/list.go
index 974db353..10a3308c 100644
--- a/pkg/kn/commands/service/list.go
+++ b/pkg/kn/commands/service/list.go
@@ -24,7 +24,6 @@ import (
 	"knative.dev/client/pkg/kn/commands"
 	"knative.dev/client/pkg/kn/commands/flags"
 	clientservingv1 "knative.dev/client/pkg/serving/v1"
-	"knative.dev/client/pkg/util"
 )
 
 // NewServiceListCommand represents 'kn service list' command
@@ -66,11 +65,6 @@ func NewServiceListCommand(p *commands.KnParams) *cobra.Command {
 				serviceListFlags.EnsureWithNamespace()
 			}
 
-			printer, err := serviceListFlags.ToPrinter()
-			if err != nil {
-				return err
-			}
-
 			// Sort serviceList by namespace and name (in this order)
 			sort.SliceStable(serviceList.Items, func(i, j int) bool {
 				a := serviceList.Items[i]
@@ -82,15 +76,7 @@ func NewServiceListCommand(p *commands.KnParams) *cobra.Command {
 				return a.ObjectMeta.Name < b.ObjectMeta.Name
 			})
 
-			if serviceListFlags.GenericPrintFlags.OutputFlagSpecified() {
-				unstructedList, err := util.ToUnstructuredList(serviceList)
-				if err != nil {
-					return err
-				}
-				return printer.PrintObj(unstructedList, cmd.OutOrStdout())
-			}
-
-			return printer.PrintObj(serviceList, cmd.OutOrStdout())
+			return serviceListFlags.Print(serviceList, cmd.OutOrStdout())
 		},
 	}
 	commands.AddNamespaceFlags(serviceListCommand.Flags(), true)

here is the output:

➜  client git:(f5a4863b) ✗ ./kn revision list -oname
revision.serving.knative.dev/svc-tgzvv-1
➜  client git:(f5a4863b) ✗ ./kn service list -oname 
service.serving.knative.dev/svc

[navidshaikh]

/retest

[navidshaikh]

@daisy-ycguo needs rebase and changes in e2e as e2e refactor PR is in now

test/e2e/basic_workflow_test.go:52:19: not enough arguments in call to serviceListOutput
	have (*test.KnRunResultCollector, string)
	want (*test.KnRunResultCollector, *test.KnRunResultCollector, string)

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/flags/listprint.go	62.5%	56.0%	-6.5
pkg/kn/commands/service/list.go	89.5%	93.8%	4.3
pkg/util/unstructured.go	Do not exist	78.3%

[daisy-ycguo]

@navidshaikh This PR is ready for review.

[navidshaikh]

/lgtm
/approve

Thanks Daisy!

[navidshaikh]

Suggested change

[navidshaikh]

Suggested change

	// Copyright © 2019 The Knative Authors
	// Copyright © 2020 The Knative Authors

[navidshaikh]

does it require to set creationTimeStamp field ?

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: daisy-ycguo, maximilien, navidshaikh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [maximilien,navidshaikh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment