List revisions sorted by configuration generation (#332)

* List revisions sorted by generation order

We use a configuration generation as a sort key, which is obtained from a label on configuration resource.

* Change column name from order to generation

* Fix revision sorting issue

* Fix revision e2e test

* Just sort by name if invalid type of generation found

* Update CHANGELOG

CHANGELOG.adoc

@@ -26,6 +26,10 @@
 | Better error handling when providing wrong kubeconfig option
 | https://github.com/knative/client/pull/222[#222]
 
+| 🎁
+| List revisions sorted by configuration generation
+| https://github.com/knative/client/pull/332[#332]
+
 |===
 
 ## v0.2.0 (2019-07-10)

pkg/kn/commands/revision/human_readable_flags.go

@@ -28,6 +28,7 @@ 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."},
@@ -56,6 +57,7 @@ func printRevisionList(revisionList *servingv1alpha1.RevisionList, options hprin
 func printRevision(revision *servingv1alpha1.Revision, options hprinters.PrintOptions) ([]metav1beta1.TableRow, error) {
 	name := revision.Name
 	service := revision.Labels[serving.ServiceLabelKey]
+	generation := revision.Labels[serving.ConfigurationGenerationLabelKey]
 	age := commands.TranslateTimestampSince(revision.CreationTimestamp)
 	conditions := commands.ConditionsValue(revision.Status.Conditions)
 	ready := commands.ReadyCondition(revision.Status.Conditions)
@@ -66,6 +68,7 @@ func printRevision(revision *servingv1alpha1.Revision, options hprinters.PrintOp
 	row.Cells = append(row.Cells,
 		name,
 		service,
+		generation,
 		age,
 		conditions,
 		ready,

pkg/kn/commands/revision/revision_list.go

@@ -16,6 +16,10 @@ package revision
 
 import (
 	"fmt"
+	"sort"
+	"strconv"
+
+	"github.com/knative/serving/pkg/apis/serving"
 
 	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
 	"github.com/spf13/cobra"
@@ -80,6 +84,28 @@ func NewRevisionListCommand(p *commands.KnParams) *cobra.Command {
 					return nil
 				}
 			}
+
+			// sort revisionList by configuration generation key
+			sort.SliceStable(revisionList.Items, func(i, j int) bool {
+				a := revisionList.Items[i]
+				b := revisionList.Items[j]
+
+				// Convert configuration generation key from string to int for avoiding string comparison.
+				agen, err := strconv.Atoi(a.Labels[serving.ConfigurationGenerationLabelKey])
+				if err != nil {
+					return a.Name < b.Name
+				}
+				bgen, err := strconv.Atoi(b.Labels[serving.ConfigurationGenerationLabelKey])
+				if err != nil {
+					return a.Name < b.Name
+				}
+
+				if agen != bgen {
+					return agen > bgen
+				}
+				return a.Name < b.Name
+			})
+
 			printer, err := revisionListFlags.ToPrinter()
 			if err != nil {
 				return err

pkg/kn/commands/revision/revision_list_test.go

@@ -29,6 +29,8 @@ import (
 	"github.com/knative/client/pkg/util"
 )
 
+var revisionListHeader = []string{"NAME", "SERVICE", "GENERATION", "AGE", "CONDITIONS", "READY", "REASON"}
+
 func fakeRevisionList(args []string, response *v1alpha1.RevisionList) (action client_testing.Action, output []string, err error) {
 	knParams := &commands.KnParams{}
 	cmd, fakeServing, buf := commands.CreateTestKnCommand(NewRevisionCommand(knParams), knParams)
@@ -75,9 +77,16 @@ func TestRevisionListEmptyByName(t *testing.T) {
 }
 
 func TestRevisionListDefaultOutput(t *testing.T) {
-	revision1 := createMockRevisionWithParams("foo-abcd", "foo")
+	revision1 := createMockRevisionWithParams("foo-abcd", "foo", "1")
-	revision2 := createMockRevisionWithParams("bar-wxyz", "bar")
+	revision2 := createMockRevisionWithParams("bar-abcd", "bar", "1")
-	RevisionList := &v1alpha1.RevisionList{Items: []v1alpha1.Revision{*revision1, *revision2}}
+	revision3 := createMockRevisionWithParams("foo-wxyz", "foo", "2")
+	revision4 := createMockRevisionWithParams("bar-wxyz", "bar", "2")
+	// Validate edge case for catching the sorting issue caused by string comparison
+	revision5 := createMockRevisionWithParams("foo-wxyz", "foo", "10")
+	revision6 := createMockRevisionWithParams("bar-wxyz", "bar", "10")
+
+	RevisionList := &v1alpha1.RevisionList{Items: []v1alpha1.Revision{
+		*revision1, *revision2, *revision3, *revision4, *revision5, *revision6}}
 	action, output, err := fakeRevisionList([]string{"revision", "list"}, RevisionList)
 	if err != nil {
 		t.Fatal(err)
@@ -87,16 +96,20 @@ func TestRevisionListDefaultOutput(t *testing.T) {
 	} else if !action.Matches("list", "revisions") {
 		t.Errorf("Bad action %v", action)
 	}
-	assert.Check(t, util.ContainsAll(output[0], "NAME", "SERVICE", "AGE", "CONDITIONS", "READY", "REASON"))
+	assert.Check(t, util.ContainsAll(output[0], revisionListHeader...))
-	assert.Check(t, util.ContainsAll(output[1], "foo-abcd", "foo"))
+	assert.Check(t, util.ContainsAll(output[1], "bar-wxyz", "bar", "10"))
-	assert.Check(t, util.ContainsAll(output[2], "bar-wxyz", "bar"))
+	assert.Check(t, util.ContainsAll(output[2], "foo-wxyz", "foo", "10"))
+	assert.Check(t, util.ContainsAll(output[3], "bar-wxyz", "bar", "2"))
+	assert.Check(t, util.ContainsAll(output[4], "foo-wxyz", "foo", "2"))
+	assert.Check(t, util.ContainsAll(output[5], "bar-abcd", "bar", "1"))
+	assert.Check(t, util.ContainsAll(output[6], "foo-abcd", "foo", "1"))
 }
 
 func TestRevisionListForService(t *testing.T) {
-	revision1 := createMockRevisionWithParams("foo-abcd", "svc1")
+	revision1 := createMockRevisionWithParams("foo-abcd", "svc1", "1")
-	revision2 := createMockRevisionWithParams("bar-wxyz", "svc1")
+	revision2 := createMockRevisionWithParams("bar-wxyz", "svc1", "2")
-	revision3 := createMockRevisionWithParams("foo-abcd", "svc2")
+	revision3 := createMockRevisionWithParams("foo-abcd", "svc2", "1")
-	revision4 := createMockRevisionWithParams("bar-wxyz", "svc2")
+	revision4 := createMockRevisionWithParams("bar-wxyz", "svc2", "2")
 	RevisionList := &v1alpha1.RevisionList{Items: []v1alpha1.Revision{*revision1, *revision2, *revision3, *revision4}}
 	action, output, err := fakeRevisionList([]string{"revision", "list", "-s", "svc1"}, RevisionList)
 	if err != nil {
@@ -107,9 +120,9 @@ func TestRevisionListForService(t *testing.T) {
 	} else if !action.Matches("list", "revisions") {
 		t.Errorf("Bad action %v", action)
 	}
-	assert.Check(t, util.ContainsAll(output[0], "NAME", "SERVICE", "AGE", "CONDITIONS", "READY", "REASON"))
+	assert.Check(t, util.ContainsAll(output[0], revisionListHeader...))
-	assert.Check(t, util.ContainsAll(output[1], "foo-abcd", "svc1"))
+	assert.Check(t, util.ContainsAll(output[1], "bar-wxyz", "svc1"))
-	assert.Check(t, util.ContainsAll(output[2], "bar-wxyz", "svc1"))
+	assert.Check(t, util.ContainsAll(output[2], "foo-abcd", "svc1"))
 	action, output, err = fakeRevisionList([]string{"revision", "list", "-s", "svc2"}, RevisionList)
 	if err != nil {
 		t.Fatal(err)
@@ -119,9 +132,9 @@ func TestRevisionListForService(t *testing.T) {
 	} else if !action.Matches("list", "revisions") {
 		t.Errorf("Bad action %v", action)
 	}
-	assert.Check(t, util.ContainsAll(output[0], "NAME", "SERVICE", "AGE", "CONDITIONS", "READY", "REASON"))
+	assert.Check(t, util.ContainsAll(output[0], revisionListHeader...))
-	assert.Check(t, util.ContainsAll(output[1], "foo-abcd", "svc2"))
+	assert.Check(t, util.ContainsAll(output[1], "bar-wxyz", "svc2"))
-	assert.Check(t, util.ContainsAll(output[2], "bar-wxyz", "svc"))
+	assert.Check(t, util.ContainsAll(output[2], "foo-abcd", "svc2"))
 	//test for non existent service
 	action, output, err = fakeRevisionList([]string{"revision", "list", "-s", "svc3"}, RevisionList)
 	if err != nil {
@@ -137,7 +150,7 @@ func TestRevisionListForService(t *testing.T) {
 }
 
 func TestRevisionListOneOutput(t *testing.T) {
-	revision := createMockRevisionWithParams("foo-abcd", "foo")
+	revision := createMockRevisionWithParams("foo-abcd", "foo", "1")
 	RevisionList := &v1alpha1.RevisionList{Items: []v1alpha1.Revision{*revision}}
 	action, output, err := fakeRevisionList([]string{"revision", "list", "foo-abcd"}, RevisionList)
 	if err != nil {
@@ -149,7 +162,7 @@ func TestRevisionListOneOutput(t *testing.T) {
 		t.Errorf("Bad action %v", action)
 	}
 
-	assert.Assert(t, util.ContainsAll(output[0], "NAME", "SERVICE", "AGE", "CONDITIONS", "READY", "REASON"))
+	assert.Assert(t, util.ContainsAll(output[0], revisionListHeader...))
 	assert.Assert(t, util.ContainsAll(output[1], "foo", "foo-abcd"))
 }
 
@@ -159,7 +172,7 @@ func TestRevisionListOutputWithTwoRevName(t *testing.T) {
 	assert.ErrorContains(t, err, "'kn revision list' accepts maximum 1 argument")
 }
 
-func createMockRevisionWithParams(name, svcName string) *v1alpha1.Revision {
+func createMockRevisionWithParams(name, svcName, generation string) *v1alpha1.Revision {
 	revision := &v1alpha1.Revision{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "Revision",
@@ -168,7 +181,10 @@ func createMockRevisionWithParams(name, svcName string) *v1alpha1.Revision {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name,
 			Namespace: "default",
-			Labels:    map[string]string{serving.ServiceLabelKey: svcName},
+			Labels: map[string]string{
+				serving.ServiceLabelKey:                 svcName,
+				serving.ConfigurationGenerationLabelKey: generation,
+			},
 		},
 	}
 	return revision

test/e2e/revision_test.go

@@ -18,6 +18,7 @@ package e2e
 
 import (
 	"fmt"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -39,6 +40,14 @@ func TestRevision(t *testing.T) {
 		test.revisionDescribeWithPrintFlags(t, "hello")
 	})
 
+	t.Run("update hello service and increase the count of configuration generation", func(t *testing.T) {
+		test.serviceUpdate(t, "hello", []string{"--env", "TARGET=kn", "--port", "8888"})
+	})
+
+	t.Run("show a list of revisions sorted by the count of configuration generation", func(t *testing.T) {
+		test.revisionListWithService(t, "hello")
+	})
+
 	t.Run("delete latest revision from hello service and return no error", func(t *testing.T) {
 		test.revisionDelete(t, "hello")
 	})
@@ -48,6 +57,24 @@ func TestRevision(t *testing.T) {
 	})
 }
 
+func (test *e2eTest) revisionListWithService(t *testing.T, serviceNames ...string) {
+	for _, svcName := range serviceNames {
+		confGen := test.findConfigurationGeneration(t, svcName)
+
+		out, err := test.kn.RunWithOpts([]string{"revision", "list", "-s", svcName}, runOpts{})
+		assert.NilError(t, err)
+
+		outputLines := strings.Split(out, "\n")
+		// Ignore the last line because it is an empty string caused by splitting a line break
+		// at the end of the output string
+		for _, line := range outputLines[1 : len(outputLines)-1] {
+			revName := test.findRevisionByGeneration(t, svcName, confGen)
+			assert.Check(t, util.ContainsAll(line, revName, svcName, strconv.Itoa(confGen)))
+			confGen--
+		}
+	}
+}
+
 func (test *e2eTest) revisionDelete(t *testing.T, serviceName string) {
 	revName := test.findRevision(t, serviceName)
 
@@ -68,10 +95,35 @@ func (test *e2eTest) revisionDescribeWithPrintFlags(t *testing.T, serviceName st
 }
 
 func (test *e2eTest) findRevision(t *testing.T, serviceName string) string {
-	revName, err := test.kn.RunWithOpts([]string{"revision", "list", "-o=jsonpath={.items[0].metadata.name}"}, runOpts{})
+	revName, err := test.kn.RunWithOpts([]string{"revision", "list", "-s", serviceName, "-o=jsonpath={.items[0].metadata.name}"}, runOpts{})
 	assert.NilError(t, err)
 	if strings.Contains(revName, "No resources found.") {
 		t.Errorf("Could not find revision name.")
 	}
 	return revName
 }
+
+func (test *e2eTest) findRevisionByGeneration(t *testing.T, serviceName string, generation int) string {
+	maxGen := test.findConfigurationGeneration(t, serviceName)
+	revName, err := test.kn.RunWithOpts([]string{"revision", "list", "-s", serviceName,
+		fmt.Sprintf("-o=jsonpath={.items[%d].metadata.name}", maxGen-generation)}, runOpts{})
+	assert.NilError(t, err)
+	if strings.Contains(revName, "No resources found.") {
+		t.Errorf("Could not find revision name.")
+	}
+	return revName
+}
+
+func (test *e2eTest) findConfigurationGeneration(t *testing.T, serviceName string) int {
+	confGenStr, err := test.kn.RunWithOpts([]string{"revision", "list", "-s", serviceName, "-o=jsonpath={.items[0].metadata.labels.serving\\.knative\\.dev/configurationGeneration}"}, runOpts{})
+	assert.NilError(t, err)
+	if confGenStr == "" {
+		t.Errorf("Could not find configuration generation.")
+	}
+	confGen, err := strconv.Atoi(confGenStr)
+	if err != nil {
+		t.Errorf("Invalid type of configuration generation: %s", err)
+	}
+
+	return confGen
+}