feature(service list): Print NAMESPACE column as the first column when --all-namespaces is specified (#366)

* Make `kn service list --all-namespace` print namespace column

`kn service list` should print NAMESPACE column when specifying `--all-namespace` option, which follows the kubectl convention.

* Put services in default namespace at the top of the list

* Switch output of column definitions by priority field

A priority field is handled as follows:
- priority 0: print namespace column
- priority 1: print default columns
- priority 2: print additional columns for wide option

* Don't use AddFlags to bind all namespaces flag

* Update CHANGELOG

* Remove commented codes

* Fix import path

* Clean up

* Remove unnecessary return

* Swap the order of records

CHANGELOG.adoc

@@ -50,6 +50,10 @@
 | kn version command shows supported Serving and API versions
 | https://github.com/knative/client/pull/370[#370]
 
+| 🎁
+| Print NAMESPACE column as the first column when --all-namespaces is specified
+| https://github.com/knative/client/pull/366[#366]
+
 |===
 
 ## v0.2.0 (2019-07-10)

pkg/kn/commands/human_readable_flags.go

@@ -30,7 +30,7 @@ import (
 // Given the following flag values, a printer can be requested that knows
 // how to handle printing based on these values.
 type HumanPrintFlags struct {
-	//TODO: Add more flags as required
+	WithNamespace bool
 }
 
 // AllowedFormats returns more customized formating options
@@ -42,7 +42,7 @@ func (f *HumanPrintFlags) AllowedFormats() []string {
 // ToPrinter receives returns a printer capable of
 // handling human-readable output.
 func (f *HumanPrintFlags) ToPrinter(getHandlerFunc func(h hprinters.PrintHandler)) (hprinters.ResourcePrinter, error) {
-	p := hprinters.NewTablePrinter(hprinters.PrintOptions{})
+	p := hprinters.NewTablePrinter(hprinters.PrintOptions{f.WithNamespace})
 	getHandlerFunc(p)
 	return p, nil
 }
@@ -50,7 +50,7 @@ func (f *HumanPrintFlags) ToPrinter(getHandlerFunc func(h hprinters.PrintHandler
 // AddFlags receives a *cobra.Command reference and binds
 // flags related to human-readable printing to it
 func (f *HumanPrintFlags) AddFlags(c *cobra.Command) {
-	//TODO: Add more flags as required
+	// TODO: Add more flags as required
 }
 
 // NewHumanPrintFlags returns flags associated with
@@ -59,6 +59,11 @@ func NewHumanPrintFlags() *HumanPrintFlags {
 	return &HumanPrintFlags{}
 }
 
+// EnsureWithNamespace sets the "WithNamespace" humanreadable option to true.
+func (f *HumanPrintFlags) EnsureWithNamespace() {
+	f.WithNamespace = true
+}
+
 // Private functions
 
 // conditionsValue returns the True conditions count among total conditions

pkg/kn/commands/revision/human_readable_flags.go

@@ -26,13 +26,13 @@ import (
 // RevisionListHandlers adds print handlers for revision list command
 func RevisionListHandlers(h hprinters.PrintHandler) {
 	RevisionColumnDefinitions := []metav1beta1.TableColumnDefinition{
-		{Name: "Name", Type: "string", Description: "Name of the revision."},
-		{Name: "Service", Type: "string", Description: "Name of the Knative service."},
-		{Name: "Generation", Type: "string", Description: "Generation of the revision"},
-		{Name: "Age", Type: "string", Description: "Age of the revision."},
-		{Name: "Conditions", Type: "string", Description: "Conditions describing statuses of the revision."},
-		{Name: "Ready", Type: "string", Description: "Ready condition status of the revision."},
-		{Name: "Reason", Type: "string", Description: "Reason for non-ready condition of the revision."},
+		{Name: "Name", Type: "string", Description: "Name of the revision.", Priority: 1},
+		{Name: "Service", Type: "string", Description: "Name of the Knative service.", Priority: 1},
+		{Name: "Generation", Type: "string", Description: "Generation of the revision", Priority: 1},
+		{Name: "Age", Type: "string", Description: "Age of the revision.", Priority: 1},
+		{Name: "Conditions", Type: "string", Description: "Conditions describing statuses of the revision.", Priority: 1},
+		{Name: "Ready", Type: "string", Description: "Ready condition status of the revision.", Priority: 1},
+		{Name: "Reason", Type: "string", Description: "Reason for non-ready condition of the revision.", Priority: 1},
 	}
 	h.TableHandler(RevisionColumnDefinitions, printRevision)
 	h.TableHandler(RevisionColumnDefinitions, printRevisionList)

pkg/kn/commands/route/human_readable_flags.go

@@ -27,11 +27,11 @@ import (
 // RouteListHandlers adds print handlers for route list command
 func RouteListHandlers(h hprinters.PrintHandler) {
 	kRouteColumnDefinitions := []metav1beta1.TableColumnDefinition{
-		{Name: "Name", Type: "string", Description: "Name of the Knative route."},
-		{Name: "URL", Type: "string", Description: "URL of the Knative route."},
-		{Name: "Age", Type: "string", Description: "Age of the Knative route."},
-		{Name: "Conditions", Type: "string", Description: "Conditions describing statuses of route components."},
-		{Name: "Traffic", Type: "integer", Description: "Traffic configured for route."},
+		{Name: "Name", Type: "string", Description: "Name of the Knative route.", Priority: 1},
+		{Name: "URL", Type: "string", Description: "URL of the Knative route.", Priority: 1},
+		{Name: "Age", Type: "string", Description: "Age of the Knative route.", Priority: 1},
+		{Name: "Conditions", Type: "string", Description: "Conditions describing statuses of route components.", Priority: 1},
+		{Name: "Traffic", Type: "integer", Description: "Traffic configured for route.", Priority: 1},
 	}
 	h.TableHandler(kRouteColumnDefinitions, printRoute)
 	h.TableHandler(kRouteColumnDefinitions, printKRouteList)

pkg/kn/commands/service/human_readable_flags.go

@@ -15,6 +15,8 @@
 package service
 
 import (
+	"sort"
+
 	metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"knative.dev/client/pkg/kn/commands"
@@ -25,16 +27,16 @@ import (
 // ServiceListHandlers adds print handlers for service list command
 func ServiceListHandlers(h hprinters.PrintHandler) {
 	kServiceColumnDefinitions := []metav1beta1.TableColumnDefinition{
-		{Name: "Name", Type: "string", Description: "Name of the Knative service."},
-		{Name: "Url", Type: "string", Description: "URL of the Knative service."},
-		//{Name: "LastCreatedRevision", Type: "string", Description: "Name of last revision created."},
-		//{Name: "LastReadyRevision", Type: "string", Description: "Name of last ready revision."},
-		{Name: "Generation", Type: "integer", Description: "Sequence number of 'Generation' of the service that was last processed by the controller."},
-		{Name: "Age", Type: "string", Description: "Age of the service."},
-		{Name: "Conditions", Type: "string", Description: "Conditions describing statuses of service components."},
-		{Name: "Ready", Type: "string", Description: "Ready condition status of the service."},
-		{Name: "Reason", Type: "string", Description: "Reason for non-ready condition of the service."},
+		{Name: "Namespace", Type: "string", Description: "Namespace of the Knative service", Priority: 0},
+		{Name: "Name", Type: "string", Description: "Name of the Knative service.", Priority: 1},
+		{Name: "Url", Type: "string", Description: "URL of the Knative service.", Priority: 1},
+		{Name: "Generation", Type: "integer", Description: "Sequence number of 'Generation' of the service that was last processed by the controller.", Priority: 1},
+		{Name: "Age", Type: "string", Description: "Age of the service.", Priority: 1},
+		{Name: "Conditions", Type: "string", Description: "Conditions describing statuses of service components.", Priority: 1},
+		{Name: "Ready", Type: "string", Description: "Ready condition status of the service.", Priority: 1},
+		{Name: "Reason", Type: "string", Description: "Reason for non-ready condition of the service.", Priority: 1},
 	}
+
 	h.TableHandler(kServiceColumnDefinitions, printKService)
 	h.TableHandler(kServiceColumnDefinitions, printKServiceList)
 }
@@ -44,6 +46,11 @@ func ServiceListHandlers(h hprinters.PrintHandler) {
 // printKServiceList populates the knative service list table rows
 func printKServiceList(kServiceList *servingv1alpha1.ServiceList, options hprinters.PrintOptions) ([]metav1beta1.TableRow, error) {
 	rows := make([]metav1beta1.TableRow, 0, len(kServiceList.Items))
+
+	if options.AllNamespaces {
+		return printKServiceWithNaemspace(kServiceList, options)
+	}
+
 	for _, ksvc := range kServiceList.Items {
 		r, err := printKService(&ksvc, options)
 		if err != nil {
@@ -54,6 +61,39 @@ func printKServiceList(kServiceList *servingv1alpha1.ServiceList, options hprint
 	return rows, nil
 }
 
+// printKServiceWithNaemspace populates the knative service table rows with namespace column
+func printKServiceWithNaemspace(kServiceList *servingv1alpha1.ServiceList, options hprinters.PrintOptions) ([]metav1beta1.TableRow, error) {
+	rows := make([]metav1beta1.TableRow, 0, len(kServiceList.Items))
+
+	// temporary slice for sorting services in non-default namespace
+	others := []metav1beta1.TableRow{}
+
+	for _, ksvc := range kServiceList.Items {
+		// Fill in with services in `default` namespace at first
+		if ksvc.Namespace == "default" {
+			r, err := printKService(&ksvc, options)
+			if err != nil {
+				return nil, err
+			}
+			rows = append(rows, r...)
+			continue
+		}
+		// put other services in temporary slice
+		r, err := printKService(&ksvc, options)
+		if err != nil {
+			return nil, err
+		}
+		others = append(others, r...)
+	}
+
+	// sort other services list alphabetically by namespace
+	sort.SliceStable(others, func(i, j int) bool {
+		return others[i].Cells[0].(string) < others[j].Cells[0].(string)
+	})
+
+	return append(rows, others...), nil
+}
+
 // printKService populates the knative service table rows
 func printKService(kService *servingv1alpha1.Service, options hprinters.PrintOptions) ([]metav1beta1.TableRow, error) {
 	name := kService.Name
@@ -69,6 +109,11 @@ func printKService(kService *servingv1alpha1.Service, options hprinters.PrintOpt
 	row := metav1beta1.TableRow{
 		Object: runtime.RawExtension{Object: kService},
 	}
+
+	if options.AllNamespaces {
+		row.Cells = append(row.Cells, kService.Namespace)
+	}
+
 	row.Cells = append(row.Cells,
 		name,
 		url,

pkg/kn/commands/service/service_list.go

@@ -57,6 +57,12 @@ func NewServiceListCommand(p *commands.KnParams) *cobra.Command {
 				fmt.Fprintf(cmd.OutOrStdout(), "No resources found.\n")
 				return nil
 			}
+
+			// empty namespace indicates all-namespaces flag is specified
+			if namespace == "" {
+				serviceListFlags.EnsureWithNamespace()
+			}
+
 			printer, err := serviceListFlags.ToPrinter()
 			if err != nil {
 				return err

pkg/kn/commands/service/service_list_flags.go

@@ -46,7 +46,7 @@ func (f *ServiceListFlags) ToPrinter() (hprinters.ResourcePrinter, error) {
 		}
 		return p, nil
 	}
-	// if no flags specified, use the table printing
+
 	p, err := f.HumanReadableFlags.ToPrinter(ServiceListHandlers)
 	if err != nil {
 		return nil, err
@@ -58,7 +58,6 @@ func (f *ServiceListFlags) ToPrinter() (hprinters.ResourcePrinter, error) {
 // flags related to humanreadable and template printing.
 func (f *ServiceListFlags) AddFlags(cmd *cobra.Command) {
 	f.GenericPrintFlags.AddFlags(cmd)
-	f.HumanReadableFlags.AddFlags(cmd)
 }
 
 // NewServiceListFlags returns flags associated with humanreadable,
@@ -69,3 +68,9 @@ func NewServiceListFlags() *ServiceListFlags {
 		HumanReadableFlags: commands.NewHumanPrintFlags(),
 	}
 }
+
+// EnsureWithNamespace ensures that humanreadable flags return
+// a printer capable of printing with a "namespace" column.
+func (f *ServiceListFlags) EnsureWithNamespace() {
+	f.HumanReadableFlags.EnsureWithNamespace()
+}

pkg/kn/commands/service/service_list_mock_test.go

@@ -0,0 +1,85 @@
+// Copyright © 2019 The Knative Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package service
+
+import (
+	"strings"
+	"testing"
+
+	"gotest.tools/assert"
+	"k8s.io/apimachinery/pkg/api/errors"
+	knclient "knative.dev/client/pkg/serving/v1alpha1"
+	"knative.dev/client/pkg/util"
+	"knative.dev/serving/pkg/apis/serving/v1alpha1"
+)
+
+func TestServiceListAllNamespaceMock(t *testing.T) {
+	client := knclient.NewMockKnClient(t)
+
+	r := client.Recorder()
+
+	r.GetService("svc1", nil, errors.NewNotFound(v1alpha1.Resource("service"), "svc1"))
+	r.CreateService(knclient.Any(), nil)
+	r.WaitForService("svc1", knclient.Any(), nil)
+	r.GetService("svc1", getServiceWithNamespace("svc1", "default"), nil)
+
+	r.GetService("svc2", nil, errors.NewNotFound(v1alpha1.Resource("service"), "foo"))
+	r.CreateService(knclient.Any(), nil)
+	r.WaitForService("svc2", knclient.Any(), nil)
+	r.GetService("svc2", getServiceWithNamespace("svc2", "foo"), nil)
+
+	r.GetService("svc3", nil, errors.NewNotFound(v1alpha1.Resource("service"), "svc3"))
+	r.CreateService(knclient.Any(), nil)
+	r.WaitForService("svc3", knclient.Any(), nil)
+	r.GetService("svc3", getServiceWithNamespace("svc3", "bar"), nil)
+
+	r.ListServices(knclient.Any(), &v1alpha1.ServiceList{
+		Items: []v1alpha1.Service{
+			*getServiceWithNamespace("svc1", "default"),
+			*getServiceWithNamespace("svc2", "foo"),
+			*getServiceWithNamespace("svc3", "bar"),
+		},
+	}, nil)
+
+	output, err := executeServiceCommand(client, "create", "svc1", "--image", "gcr.io/foo/bar:baz", "--namespace", "default")
+	assert.NilError(t, err)
+	assert.Assert(t, util.ContainsAll(output, "created", "svc1", "default", "Waiting"))
+
+	output, err = executeServiceCommand(client, "create", "svc2", "--image", "gcr.io/foo/bar:baz", "--namespace", "foo")
+	assert.NilError(t, err)
+	assert.Assert(t, util.ContainsAll(output, "created", "svc2", "foo", "Waiting"))
+
+	output, err = executeServiceCommand(client, "create", "svc3", "--image", "gcr.io/foo/bar:baz", "--namespace", "bar")
+	assert.NilError(t, err)
+	assert.Assert(t, util.ContainsAll(output, "created", "svc3", "bar", "Waiting"))
+
+	output, err = executeServiceCommand(client, "list", "--all-namespaces")
+	assert.NilError(t, err)
+
+	outputLines := strings.Split(output, "\n")
+	assert.Assert(t, util.ContainsAll(outputLines[0], "NAMESPACE", "NAME", "URL", "GENERATION", "AGE", "CONDITIONS", "READY", "REASON"))
+	assert.Assert(t, util.ContainsAll(outputLines[1], "default", "svc1"))
+	assert.Assert(t, util.ContainsAll(outputLines[2], "bar", "svc3"))
+	assert.Assert(t, util.ContainsAll(outputLines[3], "foo", "svc2"))
+
+	r.Validate()
+}
+
+func getServiceWithNamespace(name, namespace string) *v1alpha1.Service {
+	service := v1alpha1.Service{}
+	service.Name = name
+	service.Namespace = namespace
+	return &service
+}

pkg/kn/commands/service/service_list_test.go

@@ -76,9 +76,9 @@ func TestGetEmpty(t *testing.T) {
 }
 
 func TestServiceListDefaultOutput(t *testing.T) {
-	service1 := createMockServiceWithParams("foo", "http://foo.default.example.com", 2)
-	service3 := createMockServiceWithParams("sss", "http://sss.default.example.com", 3)
-	service2 := createMockServiceWithParams("bar", "http://bar.default.example.com", 1)
+	service1 := createMockServiceWithParams("foo", "default", "http://foo.default.example.com", 2)
+	service3 := createMockServiceWithParams("sss", "default", "http://sss.default.example.com", 3)
+	service2 := createMockServiceWithParams("bar", "default", "http://bar.default.example.com", 1)
 	serviceList := &v1alpha1.ServiceList{Items: []v1alpha1.Service{*service1, *service2, *service3}}
 	action, output, err := fakeServiceList([]string{"service", "list"}, serviceList)
 	if err != nil {
@@ -96,8 +96,29 @@ func TestServiceListDefaultOutput(t *testing.T) {
 	assert.Check(t, util.ContainsAll(output[3], "sss", "sss.default.example.com", "3"))
 }
 
+func TestServiceListAllNamespacesOutput(t *testing.T) {
+	service1 := createMockServiceWithParams("foo", "default", "http://foo.default.example.com", 1)
+	service2 := createMockServiceWithParams("bar", "foo", "http://bar.foo.example.com", 2)
+	service3 := createMockServiceWithParams("sss", "bar", "http://sss.bar.example.com", 3)
+	serviceList := &v1alpha1.ServiceList{Items: []v1alpha1.Service{*service1, *service2, *service3}}
+	action, output, err := fakeServiceList([]string{"service", "list", "--all-namespaces"}, serviceList)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if action == nil {
+		t.Errorf("No action")
+	} else if !action.Matches("list", "services") {
+		t.Errorf("Bad action %v", action)
+	}
+	// Outputs in alphabetical order
+	assert.Check(t, util.ContainsAll(output[0], "NAMESPACE", "NAME", "URL", "GENERATION", "AGE", "CONDITIONS", "READY", "REASON"))
+	assert.Check(t, util.ContainsAll(output[1], "default", "foo", "foo.default.example.com", "1"))
+	assert.Check(t, util.ContainsAll(output[2], "bar", "sss", "sss.bar.example.com", "3"))
+	assert.Check(t, util.ContainsAll(output[3], "foo", "bar", "bar.foo.example.com", "2"))
+}
+
 func TestServiceGetOneOutput(t *testing.T) {
-	service := createMockServiceWithParams("foo", "foo.default.example.com", 1)
+	service := createMockServiceWithParams("foo", "default", "foo.default.example.com", 1)
 	serviceList := &v1alpha1.ServiceList{Items: []v1alpha1.Service{*service}}
 	action, output, err := fakeServiceList([]string{"service", "list", "foo"}, serviceList)
 	if err != nil {
@@ -113,13 +134,13 @@ func TestServiceGetOneOutput(t *testing.T) {
 }
 
 func TestServiceGetWithTwoSrvName(t *testing.T) {
-	service := createMockServiceWithParams("foo", "foo.default.example.com", 1)
+	service := createMockServiceWithParams("foo", "default", "foo.default.example.com", 1)
 	serviceList := &v1alpha1.ServiceList{Items: []v1alpha1.Service{*service}}
 	_, _, err := fakeServiceList([]string{"service", "list", "foo", "bar"}, serviceList)
 	assert.ErrorContains(t, err, "'kn service list' accepts maximum 1 argument")
 }
 
-func createMockServiceWithParams(name, urlS string, generation int64) *v1alpha1.Service {
+func createMockServiceWithParams(name, namespace, urlS string, generation int64) *v1alpha1.Service {
 	url, _ := apis.ParseURL(urlS)
 	service := &v1alpha1.Service{
 		TypeMeta: metav1.TypeMeta{
@@ -128,7 +149,7 @@ func createMockServiceWithParams(name, urlS string, generation int64) *v1alpha1.
 		},
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name,
-			Namespace: "default",
+			Namespace: namespace,
 		},
 		Spec: v1alpha1.ServiceSpec{
 			DeprecatedRunLatest: &v1alpha1.RunLatestType{},

pkg/printers/interface.go

@@ -39,4 +39,5 @@ func (fn ResourcePrinterFunc) PrintObj(obj runtime.Object, w io.Writer) error {
 // PrintOptions for different table printing options
 type PrintOptions struct {
 	//TODO: Add options for eg: with-kind, server-printing, wide etc
+	AllNamespaces bool
 }

pkg/printers/tableprinter.go

@@ -76,6 +76,9 @@ func printRowsForHandlerEntry(output io.Writer, handler *handlerEntry, obj runti
 
 	var headers []string
 	for _, column := range handler.columnDefinitions {
+		if !options.AllNamespaces && column.Priority == 0 {
+			continue
+		}
 		headers = append(headers, strings.ToUpper(column.Name))
 	}
 	printHeader(headers, output)