Skip LatestReadyRevisionName if Revision is Pending or Unknown #825
Conversation
There was a problem hiding this comment.
@danielhelfand: 2 warnings.
In response to this:
Description
This pull request adds a check for if
LatestReadyRevisionNameis""withkn service describe, indicating the Revision is in a Pending or Unknown state. This will preventkn service describefrom returning an error message ofresource name may not be emptydue toLatestReadyRevisionNamebeing an empty string when passed to theGetRevisionfunction incompleteWithLatestRevisions.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
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
| 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)) |
There was a problem hiding this comment.
Golint naming: var serviceUrl should be serviceURL. More info.
| 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)) |
There was a problem hiding this comment.
Golint naming: var addressUrl should be addressURL. More info.
knative-prow-robot
added
the
needs-ok-to-test
|
Hi @danielhelfand. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
knative-prow-robot
added
the
size/L
|
/ok-to-test |
knative-prow-robot
added
ok-to-test
pkg/kn/commands/service/describe.go
Outdated
| @@ -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 == "" { | |||
There was a problem hiding this comment.
I would move the check for revisionName == "" to the first position as it doesn't make sense to check for an empty name in the has anyway.
There was a problem hiding this comment.
Good catch! Addressed.
knative-prow-robot
added
the
approved
|
The following is the coverage report on the affected files.
|
|
Thanks ! /lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielhelfand, maximilien, rhuss The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
change receivers name to make more sense
Description
This pull request adds a check for if
LatestReadyRevisionNameis""withkn service describe, indicating the Revision is in a Pending or Unknown state. This will preventkn service describefrom returning an error message ofresource name may not be emptydue toLatestReadyRevisionNamebeing an empty string when passed to theGetRevisionfunction incompleteWithLatestRevisions.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