Repository: knative/client (go)

Issue:
Confusing Error Message from kn service describe When Revision Not Available
### Bug report 

When using `kn service describe` on a Service where the Revision is not available, the following error message is returned:

```
resource name may not be empty
```

It would be helpful to display a more informative error message or display that the Revision is not ready as part of the describe command. 

Debugging this locally, this error comes from [`completeWithLatestRevisions`](https://github.com/knative/client/blob/c41e9fd923299a735865572d79e8770aae9655d9/pkg/kn/commands/service/describe.go#L303) and is returned here:

https://github.com/knative/client/blob/c41e9fd923299a735865572d79e8770aae9655d9/pkg/kn/commands/service/describe.go#L119

![image](https://user-images.githubusercontent.com/34258252/81018101-d23d5d00-8e31-11ea-9748-da92d5cfc564.png)

It appears an empty string is being passed to `GetRevision` causing the error. 

### Expected behavior

`kn service describe` should describe the Service without issue. Also, regardless of the root of this issue, there should be more information about why this error message is occurring. 

### Steps to reproduce the problem

Find a way to create a `Service` with a `Revision` stuck in a pending state:

```
$ kn service list
NAME            URL                                                                                     LATEST
  AGE     CONDITIONS   READY     REASON
helloworld-go   http://helloworld-go.deploy-your-first-application-using-knative-w01-s001.example.com
  6m49s   0 OK / 3     Unknown   RevisionMissing : Configuration "helloworld-go" is waiting for a Revision to b
ecome ready.
```

Run `kn service describe` on the service. 

### kn version

```
Version:      v0.14.0
Build Date:   2020-04-22 08:56:32
Git Revision: c41e9fd
Supported APIs:
* Serving
  - serving.knative.dev/v1 (knative-serving v0.14.0)
* Eventing
  - sources.knative.dev/v1alpha2 (knative-eventing v0.14.1)
  - eventing.knative.dev/v1alpha1 (knative-eventing v0.14.1)
```

### Knative (serving/eventing) version

Serving: v0.14.0


Pull request:
Skip LatestReadyRevisionName if Revision is Pending or Unknown
## Description

This pull request adds a check for if `LatestReadyRevisionName` is `""` with `kn service describe`, indicating the Revision is in a Pending or Unknown state. This will prevent `kn service describe` from returning an error message of `resource name may not be empty` due to `LatestReadyRevisionName` being an empty string when passed to the `GetRevision` function in ` completeWithLatestRevisions`.

## Changes

Add a check for if `revisionName == ""` and continue to the next loop iteration if true. Also added a unit test for this case and edited helper functions. 

Open to any different interpretation of how to treat this case as far as whether the revisionName should be skipped or other details should be returned.

## Reference

Fixes #823 

/lint


Patch:
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
index fac439f4..af95da59 100644
--- a/CHANGELOG.adoc
+++ b/CHANGELOG.adoc
@@ -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%"]
diff --git a/pkg/kn/commands/service/describe.go b/pkg/kn/commands/service/describe.go
index 7741d343..f8191520 100644
--- a/pkg/kn/commands/service/describe.go
+++ b/pkg/kn/commands/service/describe.go
@@ -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 revisionName == "" || revisionsSeen.Has(revisionName) {
 			continue
 		}
 		revisionsSeen.Insert(revisionName)


Test patch:
diff --git a/pkg/kn/commands/service/describe_test.go b/pkg/kn/commands/service/describe_test.go
index dc2543e2..da4143e5 100644
--- a/pkg/kn/commands/service/describe_test.go
+++ b/pkg/kn/commands/service/describe_test.go
@@ -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:
@@ -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")
@@ -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")
@@ -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)
 
@@ -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)
 
@@ -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)
 
@@ -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)
@@ -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)
 
@@ -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)
 
@@ -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"
@@ -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{
@@ -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)
@@ -525,8 +551,8 @@ func createTestService(name string, revisionNames []string, conditions duckv1.Co
 	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))
+	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",
@@ -541,8 +567,8 @@ func createTestService(name string, revisionNames []string, conditions duckv1.Co
 		},
 		Status: servingv1.ServiceStatus{
 			RouteStatusFields: servingv1.RouteStatusFields{
-				URL:     addressUrl,
-				Address: &duckv1.Addressable{URL: serviceUrl},
+				URL:     addressURL,
+				Address: &duckv1.Addressable{URL: serviceURL},
 			},
 			Status: duckv1.Status{
 				Conditions: conditions,
@@ -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)
 
@@ -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)
 
@@ -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,
 			},
 		},
 	}
@@ -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
+}