From 8e5302c741bff9c1fd708ac9fedd8e9fc457fdfb Mon Sep 17 00:00:00 2001 From: Tsubasa Nagasawa Date: Tue, 6 Aug 2019 19:13:54 +0900 Subject: [PATCH] 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 | 4 ++ .../commands/revision/human_readable_flags.go | 3 + pkg/kn/commands/revision/revision_list.go | 26 +++++++++ .../commands/revision/revision_list_test.go | 56 ++++++++++++------- test/e2e/revision_test.go | 54 +++++++++++++++++- 5 files changed, 122 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 9c7a490f2..96bbdc019 100644 --- a/CHANGELOG.adoc +++ b/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) diff --git a/pkg/kn/commands/revision/human_readable_flags.go b/pkg/kn/commands/revision/human_readable_flags.go index e6064a2de..60f133771 100644 --- a/pkg/kn/commands/revision/human_readable_flags.go +++ b/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, diff --git a/pkg/kn/commands/revision/revision_list.go b/pkg/kn/commands/revision/revision_list.go index cb60b525c..063940e5e 100644 --- a/pkg/kn/commands/revision/revision_list.go +++ b/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 diff --git a/pkg/kn/commands/revision/revision_list_test.go b/pkg/kn/commands/revision/revision_list_test.go index b02a038f9..fa1bd5140 100644 --- a/pkg/kn/commands/revision/revision_list_test.go +++ b/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") - revision2 := createMockRevisionWithParams("bar-wxyz", "bar") - RevisionList := &v1alpha1.RevisionList{Items: []v1alpha1.Revision{*revision1, *revision2}} + revision1 := createMockRevisionWithParams("foo-abcd", "foo", "1") + revision2 := createMockRevisionWithParams("bar-abcd", "bar", "1") + 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[1], "foo-abcd", "foo")) - assert.Check(t, util.ContainsAll(output[2], "bar-wxyz", "bar")) + assert.Check(t, util.ContainsAll(output[0], revisionListHeader...)) + assert.Check(t, util.ContainsAll(output[1], "bar-wxyz", "bar", "10")) + 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") - revision2 := createMockRevisionWithParams("bar-wxyz", "svc1") - revision3 := createMockRevisionWithParams("foo-abcd", "svc2") - revision4 := createMockRevisionWithParams("bar-wxyz", "svc2") + revision1 := createMockRevisionWithParams("foo-abcd", "svc1", "1") + revision2 := createMockRevisionWithParams("bar-wxyz", "svc1", "2") + revision3 := createMockRevisionWithParams("foo-abcd", "svc2", "1") + 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[1], "foo-abcd", "svc1")) - assert.Check(t, util.ContainsAll(output[2], "bar-wxyz", "svc1")) + assert.Check(t, util.ContainsAll(output[0], revisionListHeader...)) + assert.Check(t, util.ContainsAll(output[1], "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[1], "foo-abcd", "svc2")) - assert.Check(t, util.ContainsAll(output[2], "bar-wxyz", "svc")) + assert.Check(t, util.ContainsAll(output[0], revisionListHeader...)) + assert.Check(t, util.ContainsAll(output[1], "bar-wxyz", "svc2")) + 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 diff --git a/test/e2e/revision_test.go b/test/e2e/revision_test.go index d58b1a065..a47783007 100644 --- a/test/e2e/revision_test.go +++ b/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 +}