Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add revision information to service list #441

Merged
merged 1 commit into from
Oct 25, 2019
Merged

Add revision information to service list #441

merged 1 commit into from
Oct 25, 2019

Conversation

daisy-ycguo
Copy link
Member

Fixes #360

Proposed Changes

  • Add revision LATESTCREATED and LATESTREADY to the output of kn service list
  • Remove Generation from the output of kn service list

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 11, 2019
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 11, 2019
@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/kn/commands/service/human_readable_flags.go 92.5% 92.7% 0.2

rhuss
rhuss requested changes Oct 11, 2019
View reviewed changes
Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that showing the latest revision makes more sense than showing the generation, but I doubt that we show latestCreatedRevision, as this should be typically the same as latestReadyRevision and if not, Ready would be false and show up in kn service list anyway. For further investigation a kn service describe is needed then.

I would call the column just LATEST then.

pkg/kn/commands/service/human_readable_flags.go Outdated Show resolved Hide resolved
pkg/kn/commands/service/human_readable_flags.go Outdated Show resolved Hide resolved
maximilien
maximilien reviewed Oct 11, 2019
View reviewed changes
Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit...

pkg/kn/commands/service/list_mock_test.go Outdated Show resolved Hide resolved
@daisy-ycguo
Copy link
Member Author

@rhuss @maximilien Thank you for the reviewing.
I refer to the output of kubectl get ksvc to design the output of kn service list. LATESTCREATED and LATESTREADY are displayed in the output of kubectl get ksvc.

Considering your comments, I'm going to keep only lastReadyRevision as LATEST REVISION in the output of kn service list. Please let me know if it's OK.

$ kubectl get ksvc
NAME          URL                                                                          LATESTCREATED       LATESTREADY         READY     REASON
fib-knative   http://fib-knative-default.knative-guoyc.au-syd.containers.appdomain.cloud   fib-knative-x7zj9   fib-knative-x7zj9   True

@daisy-ycguo
Copy link
Member Author

/retest

@daisy-ycguo
Copy link
Member Author

I think it's ready for a second round review. @maximilien @rhuss Thanks.

navidshaikh
navidshaikh reviewed Oct 23, 2019
View reviewed changes
pkg/kn/commands/service/human_readable_flags.go Outdated
@@ -30,7 +30,7 @@ func ServiceListHandlers(h hprinters.PrintHandler) {
{Name: "Namespace", Type: "string", Description: "Namespace of the Knative service", Priority: 0},
{Name: "Name", Type: "string", Description: "Name of the Knative service.", Priority: 1},
{Name: "Url", Type: "string", Description: "URL of the Knative service.", Priority: 1},
{Name: "Generation", Type: "integer", Description: "Sequence number of 'Generation' of the service that was last processed by the controller.", Priority: 1},
{Name: "Latest Revision", Type: "string", Description: "Name of the latest revision.", Priority: 1},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have space in column names? Can we change the column name to Latest as suggested in review?
Also per changes, the description should read, "Name of the latest ready revision."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

NAME     URL                                 LATEST REVISION   AGE   CONDITIONS   READY   REASON
random   http://random.default.example.com   random-kndhm-4    8d    1 OK / 3     False   RevisionMissing : Revision "random-yylcn-2" referenced in traffic not found.

The header looks a bit confusing when there is a space (looks like two column headers). So please either LATEST or LATESTREVISION. Both would be ok for me (so the former is easier to ea IMO)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to Latest. The comment is changed to Name of the latest ready revision..

rhuss
rhuss requested changes Oct 24, 2019
View reviewed changes
Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit (backing @navidshaikh comment), otherwise looks good to me.

pkg/kn/commands/service/human_readable_flags.go Outdated
@@ -30,7 +30,7 @@ func ServiceListHandlers(h hprinters.PrintHandler) {
{Name: "Namespace", Type: "string", Description: "Namespace of the Knative service", Priority: 0},
{Name: "Name", Type: "string", Description: "Name of the Knative service.", Priority: 1},
{Name: "Url", Type: "string", Description: "URL of the Knative service.", Priority: 1},
{Name: "Generation", Type: "integer", Description: "Sequence number of 'Generation' of the service that was last processed by the controller.", Priority: 1},
{Name: "Latest Revision", Type: "string", Description: "Name of the latest revision.", Priority: 1},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

NAME     URL                                 LATEST REVISION   AGE   CONDITIONS   READY   REASON
random   http://random.default.example.com   random-kndhm-4    8d    1 OK / 3     False   RevisionMissing : Revision "random-yylcn-2" referenced in traffic not found.

The header looks a bit confusing when there is a space (looks like two column headers). So please either LATEST or LATESTREVISION. Both would be ok for me (so the former is easier to ea IMO)

@daisy-ycguo
Copy link
Member Author

thank you @navidshaikh and @rhuss . It's ready to review again.

navidshaikh
navidshaikh approved these changes Oct 25, 2019
View reviewed changes
Copy link
Collaborator

@navidshaikh navidshaikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/assign @rhuss
/hold

@knative-prow-robot knative-prow-robot assigned rhuss and navidshaikh and unassigned rhuss Oct 25, 2019
@knative-prow-robot knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 25, 2019
rhuss
rhuss approved these changes Oct 25, 2019
View reviewed changes
Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/lgtm
/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 25, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: daisy-ycguo, navidshaikh, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit 492f1dc into knative:master Oct 25, 2019
dsimansk added a commit to dsimansk/client that referenced this pull request Sep 27, 2024
@dsimansk
[release-v1.14] Update func to pick up Docker update (knative#441)
5a8beef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maximilien maximilien maximilien left review comments

@navidshaikh navidshaikh navidshaikh approved these changes

@rhuss rhuss rhuss approved these changes

@mattmoor mattmoor Awaiting requested review from mattmoor

Assignees

@rhuss rhuss

@navidshaikh navidshaikh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

have kn service list show latest revision name
7 participants
@daisy-ycguo @knative-metrics-robot @knative-prow-robot @maximilien @rhuss @navidshaikh @googlebot