Skip to content

Commit

Permalink
skip LatestReadyRevisionName if Revision is pending or unknown
Browse files Browse the repository at this point in the history
  • Loading branch information
@danielhelfand
danielhelfand committed May 5, 2020
1 parent c41e9fd commit 5d7fc4e
Show file tree
Hide file tree
Showing 4 changed files with 131 additions and 22 deletions.
8 changes: 8 additions & 0 deletions CHANGELOG.adoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,14 @@
| https://github.com/knative/client/pull/[#]
////

[cols="1,10,3", options="header", width="100%"]
|===
| | Description | PR

| 🐛
| Skip LatestReadyRevisionName if Revision is Pending or Unknown
| https://github.com/knative/client/pull/825[#825]

## v0.14.0 (2020-04-21)

[cols="1,10,3", options="header", width="100%"]
Expand Down
1 change: 1 addition & 0 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,7 @@ knative.dev/eventing v0.14.1 h1:YmnEl3IBVRkBcVYWPMWZegRGifeI7ibcA9xuhHWvAaw=
knative.dev/eventing v0.14.1/go.mod h1:UxweNv8yXhsdHJitcb9R6rmfNaUD2DFi9GWwNRyIs58=
knative.dev/pkg v0.0.0-20200414233146-0eed424fa4ee h1:G1QedLB/RxF4QTyL1Pq9M1QK1uj8khQgTypofUXrG20=
knative.dev/pkg v0.0.0-20200414233146-0eed424fa4ee/go.mod h1:pgODObA1dTyhNoFxPZTTjNWfx6F0aKsKzn+vaT9XO/Q=
knative.dev/pkg v0.0.0-20200504180943-4a2ba059b008 h1:ta/5dsSVJZuuilpa84232sHBiPAb2RZ5+rKmnQZvS1k=
knative.dev/serving v0.14.0 h1:9iDyOqTciNuAh2D5KJP0soOq23FDR4HQHdIQNBQ/rAE=
knative.dev/serving v0.14.0/go.mod h1:x2n255JS2XBI39tmjZ8CwTxIf9EKNMCrkVuiOttLRm0=
knative.dev/test-infra v0.0.0-20200413202711-9cf64fb1b912 h1:RZJe3OFkal3m9lorMSS2BaEnBs4OPj2o5/Oiw0JXlQY=
Expand Down
2 changes: 1 addition & 1 deletion pkg/kn/commands/service/describe.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ func orderByConfigurationGeneration(descs []*revisionDesc) []*revisionDesc {

func completeWithLatestRevisions(client clientservingv1.KnServingClient, service *servingv1.Service, revisionsSeen sets.String, descs []*revisionDesc) ([]*revisionDesc, error) {
for _, revisionName := range []string{service.Status.LatestCreatedRevisionName, service.Status.LatestReadyRevisionName} {
if revisionsSeen.Has(revisionName) {
if revisionsSeen.Has(revisionName) || revisionName == "" {
continue
}
revisionsSeen.Insert(revisionName)
Expand Down
142 changes: 121 additions & 21 deletions pkg/kn/commands/service/describe_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func TestServiceDescribeBasic(t *testing.T) {

// Get service & revision
r.GetService("foo", &expectedService, nil)
rev1 := createTestRevision("rev1", 1)
rev1 := createTestRevision("rev1", 1, goodConditions())
r.GetRevision("rev1", &rev1, nil)

// Testing:
Expand Down Expand Up @@ -92,7 +92,7 @@ func TestServiceDescribeSad(t *testing.T) {
expectedService := createTestService("foo", []string{"rev1"}, goodConditions())
expectedService.Status.Conditions[0].Status = v1.ConditionFalse
r.GetService("foo", &expectedService, nil)
rev1 := createTestRevision("rev1", 1)
rev1 := createTestRevision("rev1", 1, goodConditions())
r.GetRevision("rev1", &rev1, nil)

output, err := executeServiceCommand(client, "describe", "foo")
Expand All @@ -114,7 +114,7 @@ func TestServiceDescribeLatest(t *testing.T) {

// Get service & revision
r.GetService("foo", &expectedService, nil)
rev1 := createTestRevision("rev1", 1)
rev1 := createTestRevision("rev1", 1, goodConditions())
r.GetRevision("rev1", &rev1, nil)

output, err := executeServiceCommand(client, "describe", "foo")
Expand All @@ -141,8 +141,8 @@ func TestServiceDescribeLatestNotInTraffic(t *testing.T) {

// Get service & revision
r.GetService("foo", &expectedService, nil)
rev1 := createTestRevision("rev1", 1)
rev2 := createTestRevision("rev2", 2)
rev1 := createTestRevision("rev1", 1, goodConditions())
rev2 := createTestRevision("rev2", 2, goodConditions())
r.GetRevision("rev1", &rev1, nil)
r.GetRevision("rev2", &rev2, nil)

Expand Down Expand Up @@ -172,8 +172,8 @@ func TestServiceDescribeEachNamedOnce(t *testing.T) {

// Get service & revision
r.GetService("foo", &expectedService, nil)
rev1 := createTestRevision("rev1", 1)
rev2 := createTestRevision("rev2", 2)
rev1 := createTestRevision("rev1", 1, goodConditions())
rev2 := createTestRevision("rev2", 2, goodConditions())
r.GetRevision("rev1", &rev1, nil)
r.GetRevision("rev2", &rev2, nil)

Expand Down Expand Up @@ -204,7 +204,7 @@ func TestServiceDescribeLatestAndCurrentBothHaveTrafficEntries(t *testing.T) {

// Get service & revision
r.GetService("foo", &expectedService, nil)
rev1 := createTestRevision("rev1", 1)
rev1 := createTestRevision("rev1", 1, goodConditions())
r.GetRevision("rev1", &rev1, nil)
r.GetRevision("rev1", &rev1, nil)

Expand Down Expand Up @@ -232,8 +232,8 @@ func TestServiceDescribeLatestCreatedIsBroken(t *testing.T) {

// Get service & revision
r.GetService("foo", &expectedService, nil)
rev1 := createTestRevision("rev1", 1)
rev2 := createTestRevision("rev2", 2)
rev1 := createTestRevision("rev1", 1, goodConditions())
rev2 := createTestRevision("rev2", 2, goodConditions())
rev2.Status.Conditions[0].Status = v1.ConditionFalse
r.GetRevision("rev1", &rev1, nil)
r.GetRevision("rev2", &rev2, nil)
Expand Down Expand Up @@ -272,7 +272,7 @@ func TestServiceDescribeScaling(t *testing.T) {

// Get service & revision
r.GetService("foo", &expectedService, nil)
rev1 := createTestRevision("rev1", 1)
rev1 := createTestRevision("rev1", 1, goodConditions())
addScaling(&rev1, data.minScale, data.maxScale, data.target, data.limit, data.utilization)
r.GetRevision("rev1", &rev1, nil)

Expand Down Expand Up @@ -341,7 +341,7 @@ func TestServiceDescribeResources(t *testing.T) {

// Get service & revision
r.GetService("foo", &expectedService, nil)
rev1 := createTestRevision("rev1", 1)
rev1 := createTestRevision("rev1", 1, goodConditions())
addResourceLimits(&rev1.Spec.Containers[0].Resources, data.reqMem, data.limitMem, data.reqCPU, data.limitCPU)
r.GetRevision("rev1", &rev1, nil)

Expand Down Expand Up @@ -385,10 +385,10 @@ func TestServiceDescribeUserImageVsImage(t *testing.T) {
expectedService := createTestService("foo", []string{"rev1", "rev2", "rev3", "rev4"}, goodConditions())
r.GetService("foo", &expectedService, nil)

rev1 := createTestRevision("rev1", 1)
rev2 := createTestRevision("rev2", 2)
rev3 := createTestRevision("rev3", 3)
rev4 := createTestRevision("rev4", 4)
rev1 := createTestRevision("rev1", 1, goodConditions())
rev2 := createTestRevision("rev2", 2, goodConditions())
rev3 := createTestRevision("rev3", 3, goodConditions())
rev4 := createTestRevision("rev4", 4, goodConditions())

// Different combinations of image annotations and not.
rev1.Spec.Containers[0].Image = "gcr.io/test/image:latest"
Expand Down Expand Up @@ -438,9 +438,9 @@ func TestServiceDescribeVerbose(t *testing.T) {
expectedService.Status.Traffic[0].Percent = ptr.Int64(int64(100))
r.GetService("foo", &expectedService, nil)

rev1 := createTestRevision("rev1", 1)
rev2 := createTestRevision("rev2", 2)
rev3 := createTestRevision("rev3", 3)
rev1 := createTestRevision("rev1", 1, goodConditions())
rev2 := createTestRevision("rev2", 2, goodConditions())
rev3 := createTestRevision("rev3", 3, goodConditions())

revList := servingv1.RevisionList{
TypeMeta: metav1.TypeMeta{
Expand Down Expand Up @@ -515,6 +515,32 @@ func validateServiceOutput(t *testing.T, service string, output string) {
assert.Assert(t, util.ContainsAll(output, "Ready", "RoutesReady", "OK", "TYPE", "AGE", "REASON"))
}

func TestServiceDescribeRevisionUnknown(t *testing.T) {
client := knclient.NewMockKnServiceClient(t)

r := client.Recorder()

expectedService := createTestServiceRevisionUnknown("foo", []string{"rev1"}, unknownConditions())

r.GetService("foo", &expectedService, nil)
rev1 := createTestRevision("rev1", 1, unknownConditions())
r.GetRevision("rev1", &rev1, nil)

output, err := executeServiceCommand(client, "describe", "foo")
assert.NilError(t, err)

validateServiceOutput(t, "foo", output)
assert.Assert(t, util.ContainsAll(output, "123456"))
assert.Assert(t, util.ContainsAll(output, "Annotations:", "anno1=aval1, anno2=aval2, anno3="))
assert.Assert(t, cmp.Regexp(`(?m)\s*Annotations:.*\.\.\.$`, output))
assert.Assert(t, util.ContainsAll(output, "Labels:", "label1=lval1, label2=lval2\n"))
assert.Assert(t, util.ContainsAll(output, "[1]"))

assert.Equal(t, strings.Count(output, "rev1"), 1)

r.Validate()
}

func createTestService(name string, revisionNames []string, conditions duckv1.Conditions) servingv1.Service {

labelMap := make(map[string]string)
Expand Down Expand Up @@ -570,6 +596,61 @@ func createTestService(name string, revisionNames []string, conditions duckv1.Co
return service
}

func createTestServiceRevisionUnknown(name string, revisionNames []string, conditions duckv1.Conditions) servingv1.Service {
labelMap := make(map[string]string)
labelMap["label1"] = "lval1"
labelMap["label2"] = "lval2"
annoMap := make(map[string]string)
annoMap["anno1"] = "aval1"
annoMap["anno2"] = "aval2"
annoMap["anno3"] = "very_long_value_which_should_be_truncated_in_normal_output_if_we_make_it_even_longer"

serviceUrl, _ := apis.ParseURL(fmt.Sprintf("https://%s.default.svc.cluster.local", name))
addressUrl, _ := apis.ParseURL(fmt.Sprintf("https://%s.default.example.com", name))
service := servingv1.Service{
TypeMeta: metav1.TypeMeta{
Kind: "Service",
APIVersion: "serving.knative.dev/v1",
},
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: "default",
Labels: labelMap,
Annotations: annoMap,
CreationTimestamp: metav1.Time{Time: time.Now().Add(-30 * time.Second)},
},
Status: servingv1.ServiceStatus{
RouteStatusFields: servingv1.RouteStatusFields{
URL: addressUrl,
Address: &duckv1.Addressable{URL: serviceUrl},
},
Status: duckv1.Status{
Conditions: conditions,
},
},
}
service.Status.LatestCreatedRevisionName = revisionNames[len(revisionNames)-1]
// LatestReadyRevisionName == "" when Revision is unknown
service.Status.LatestReadyRevisionName = ""

if len(revisionNames) > 0 {
trafficTargets := make([]servingv1.TrafficTarget, 0)
for _, rname := range revisionNames {
url, _ := apis.ParseURL(fmt.Sprintf("https://%s", rname))
target := servingv1.TrafficTarget{
RevisionName: rname,
ConfigurationName: name,
Percent: ptr.Int64(int64(100 / len(revisionNames))),
URL: url,
}
trafficTargets = append(trafficTargets, target)
}
service.Status.Traffic = trafficTargets
}

return service
}

func createTestServiceWithServiceAccount(name string, revisionNames []string, serviceAccountName string, conditions duckv1.Conditions) servingv1.Service {
service := createTestService(name, revisionNames, conditions)

Expand Down Expand Up @@ -626,7 +707,7 @@ func getResourceListQuantity(mem string, cpu string) v1.ResourceList {
return list
}

func createTestRevision(revision string, gen int64) servingv1.Revision {
func createTestRevision(revision string, gen int64, conditions duckv1.Conditions) servingv1.Revision {
labels := make(map[string]string)
labels[api_serving.ConfigurationGenerationLabelKey] = fmt.Sprintf("%d", gen)

Expand Down Expand Up @@ -666,7 +747,7 @@ func createTestRevision(revision string, gen int64) servingv1.Revision {
Status: servingv1.RevisionStatus{
ImageDigest: "gcr.io/test/image@" + imageDigest,
Status: duckv1.Status{
Conditions: goodConditions(),
Conditions: conditions,
},
},
}
Expand All @@ -690,3 +771,22 @@ func goodConditions() duckv1.Conditions {
})
return ret
}

func unknownConditions() duckv1.Conditions {
ret := make(duckv1.Conditions, 0)
ret = append(ret, apis.Condition{
Type: apis.ConditionReady,
Status: v1.ConditionUnknown,
LastTransitionTime: apis.VolatileTime{
Inner: metav1.Time{Time: time.Now()},
},
})
ret = append(ret, apis.Condition{
Type: servingv1.ServiceConditionRoutesReady,
Status: v1.ConditionUnknown,
LastTransitionTime: apis.VolatileTime{
Inner: metav1.Time{Time: time.Now()},
},
})
return ret
}

0 comments on commit 5d7fc4e

Please sign in to comment.