Skip to content

Commit

Permalink
List revisions sorted by configuration generation (#332)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@toVersus @knative-prow-robot
toVersus authored and knative-prow-robot committed Aug 6, 2019
1 parent d30beba commit 8e5302c
Show file tree
Hide file tree
Showing 5 changed files with 122 additions and 21 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.adoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions pkg/kn/commands/revision/human_readable_flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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."},
Expand Down Expand Up @@ -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)
Expand All @@ -66,6 +68,7 @@ func printRevision(revision *servingv1alpha1.Revision, options hprinters.PrintOp
row.Cells = append(row.Cells,
name,
service,
generation,
age,
conditions,
ready,
Expand Down
26 changes: 26 additions & 0 deletions pkg/kn/commands/revision/revision_list.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
56 changes: 36 additions & 20 deletions pkg/kn/commands/revision/revision_list_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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)
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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"))
}

Expand All @@ -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",
Expand All @@ -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
Expand Down
54 changes: 53 additions & 1 deletion test/e2e/revision_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package e2e

import (
"fmt"
"strconv"
"strings"
"testing"

Expand All @@ -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")
})
Expand All @@ -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)

Expand All @@ -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
}

0 comments on commit 8e5302c

Please sign in to comment.