Repository: knative/client (go)

Issue:
Don't treat namespace "default" specially when sorting after namespaces
In https://github.com/knative/client/blob/71c9238b7eb065b56af08bd1b47d0d21ee6db755/pkg/kn/commands/service/human_readable_flags.go#L71-L87 the "default" name is treated specially so that it ends up at the top of the list.

I would like to revert this because I think the "default" namespace is in no ways special except that maybe that it is the namespace for your starting context when you login to a cluster.

Also, I think the sorting doesn't belong in the printing part but when the list is built up (where it already is sorted after generation).

As a side effect, removing this special treatment would simplify the code considerably.

Pull request:
refactor(service list): Removed special "default" namespace handling
Fixes #607.

Patch:
diff --git a/pkg/kn/commands/service/human_readable_flags.go b/pkg/kn/commands/service/human_readable_flags.go
index db54cd02..cc1c0cbc 100644
--- a/pkg/kn/commands/service/human_readable_flags.go
+++ b/pkg/kn/commands/service/human_readable_flags.go
@@ -15,13 +15,12 @@
 package service
 
 import (
-	"sort"
-
 	metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1"
 	"k8s.io/apimachinery/pkg/runtime"
+	servingv1alpha1 "knative.dev/serving/pkg/apis/serving/v1alpha1"
+
 	"knative.dev/client/pkg/kn/commands"
 	hprinters "knative.dev/client/pkg/printers"
-	servingv1alpha1 "knative.dev/serving/pkg/apis/serving/v1alpha1"
 )
 
 // ServiceListHandlers adds print handlers for service list command
@@ -47,10 +46,6 @@ func ServiceListHandlers(h hprinters.PrintHandler) {
 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 {
@@ -61,39 +56,6 @@ 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
diff --git a/pkg/kn/commands/service/list.go b/pkg/kn/commands/service/list.go
index 9ae2db92..c8cd3aba 100644
--- a/pkg/kn/commands/service/list.go
+++ b/pkg/kn/commands/service/list.go
@@ -69,9 +69,15 @@ func NewServiceListCommand(p *commands.KnParams) *cobra.Command {
 				return err
 			}
 
-			// Sort serviceList by name
+			// Sort serviceList by namespace and name (in this order)
 			sort.SliceStable(serviceList.Items, func(i, j int) bool {
-				return serviceList.Items[i].ObjectMeta.Name < serviceList.Items[j].ObjectMeta.Name
+				a := serviceList.Items[i]
+				b := serviceList.Items[j]
+
+				if a.Namespace != b.Namespace {
+					return a.Namespace < b.Namespace
+				}
+				return a.ObjectMeta.Name < b.ObjectMeta.Name
 			})
 
 			err = printer.PrintObj(serviceList, cmd.OutOrStdout())


Test patch:
diff --git a/pkg/kn/commands/service/list_mock_test.go b/pkg/kn/commands/service/list_mock_test.go
index b989603f..f1e5af52 100644
--- a/pkg/kn/commands/service/list_mock_test.go
+++ b/pkg/kn/commands/service/list_mock_test.go
@@ -36,8 +36,8 @@ func TestServiceListAllNamespaceMock(t *testing.T) {
 
 	outputLines := strings.Split(output, "\n")
 	assert.Assert(t, util.ContainsAll(outputLines[0], "NAMESPACE", "NAME", "URL", "LATEST", "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[1], "bar", "svc3"))
+	assert.Assert(t, util.ContainsAll(outputLines[2], "default", "svc1"))
 	assert.Assert(t, util.ContainsAll(outputLines[3], "foo", "svc2"))
 
 	r.Validate()
diff --git a/pkg/kn/commands/service/list_test.go b/pkg/kn/commands/service/list_test.go
index 720500bf..3f76b7c4 100644
--- a/pkg/kn/commands/service/list_test.go
+++ b/pkg/kn/commands/service/list_test.go
@@ -103,8 +103,8 @@ func TestServiceListAllNamespacesOutput(t *testing.T) {
 	}
 	// Outputs in alphabetical order
 	assert.Check(t, util.ContainsAll(output[0], "NAMESPACE", "NAME", "URL", "LATEST", "AGE", "CONDITIONS", "READY", "REASON"))
-	assert.Check(t, util.ContainsAll(output[1], "default", "foo", "foo.default.example.com", "foo-xyz"))
-	assert.Check(t, util.ContainsAll(output[2], "bar", "sss", "sss.bar.example.com", "sss-xyz"))
+	assert.Check(t, util.ContainsAll(output[1], "bar", "sss", "sss.bar.example.com", "sss-xyz"))
+	assert.Check(t, util.ContainsAll(output[2], "default", "foo", "foo.default.example.com", "foo-xyz"))
 	assert.Check(t, util.ContainsAll(output[3], "foo", "bar", "bar.foo.example.com", "bar-xyz"))
 }