feature(service list): Print NAMESPACE column as the first column when --all-namespaces is specified #366

toVersus · 2019-08-15T09:20:12Z

/lint

Proposed Changes

kn service list should print NAMESPACE column when specifying --all-namespaces option, which follows the kubectl convention.

NAMESPACE should be displayed as the first column when --all-namespaces is specified

I know we can find a namespace we deployed our service by inspecting an URL column. It tells us a namespace by default because its FQDN is composed of {{.Name}}.{{.Namespace}}.{{.Domain}}. However, we can easily change this FQDN template by modifying domainTemplate field in config-network.yaml. In this case, we have no idea to see a namespace from the output of kn service list --all-namespace at a glance. Also, I think it is useful to see a bunch of services in the same namespace by sorting in alphabetical order.

Print NAMESPACE as the first column when --all-namespaces is specified in kn service list command
Show services in default namespace at the top of the list
Add unit tests using fake services as well as mock framework

Before:

$ ./kn service list -A
NAME    URL                                GENERATION   AGE     CONDITIONS   READY   REASON
bar     http://bar-bar.example.com         1            73m     3 OK / 3     True
echo    http://echo-bar.example.com        1            73m     3 OK / 3     True
echo    http://echo-default.example.com    1            6h10m   3 OK / 3     True
echo    http://echo-foo.example.com        1            74m     3 OK / 3     True
foo     http://foo-default.example.com     1            6h9m    3 OK / 3     True
foo     http://foo-foo.example.com         1            74m     3 OK / 3     True
hello   http://hello-bar.example.com       1            72m     3 OK / 3     True
hello   http://hello-default.example.com   1            42h     3 OK / 3     True
hello   http://hello-foo.example.com       1            73m     3 OK / 3     True

After:

$ ./kn service list -A
NAMESPACE   NAME    URL                                GENERATION   AGE     CONDITIONS   READY   REASON
default     echo    http://echo-default.example.com    1            5h51m   3 OK / 3     True
default     foo     http://foo-default.example.com     1            5h51m   3 OK / 3     True
default     hello   http://hello-default.example.com   1            42h     3 OK / 3     True
bar         bar     http://bar-bar.example.com         1            54m     3 OK / 3     True
bar         echo    http://echo-bar.example.com        1            54m     3 OK / 3     True
bar         hello   http://hello-bar.example.com       1            54m     3 OK / 3     True
foo         echo    http://echo-foo.example.com        1            55m     3 OK / 3     True
foo         foo     http://foo-foo.example.com         1            56m     3 OK / 3     True
foo         hello   http://hello-foo.example.com       1            55m     3 OK / 3     True

knative-prow-robot

@toVersus: 1 warning.

In response to this:

/lint

Proposed Changes

kn service list should print NAMESPACE column when specifying --all-namespaces option, which follows the kubectl convention.

NAMESPACE should be displayed as the first column when --all-namespaces is specified

I know we can find a namespace we deployed our service by inspecting an URL column. It tells us a namespace by default because its FQDN is composed of {{.Name}}.{{.Namespace}}.{{.Domain}}. However, we can easily change this FQDN template by modifying domainTemplate field in config-network.yaml. In this case, we have no idea to see a namespace from the output of kn service list --all-namespace at a glance. Also, I think it is useful to see a bunch of services in the same namespace by sorting in alphabetical order.

Print NAMESPACE as the first column when --all-namespaces is specified in kn service list command

Show services in default namespace at the top of the list

Add unit tests using fake services as well as mock framework

Before:
$ ./kn service list -A
NAME    URL                                GENERATION   AGE     CONDITIONS   READY   REASON
bar     http://bar-bar.example.com         1            73m     3 OK / 3     True
echo    http://echo-bar.example.com        1            73m     3 OK / 3     True
echo    http://echo-default.example.com    1            6h10m   3 OK / 3     True
echo    http://echo-foo.example.com        1            74m     3 OK / 3     True
foo     http://foo-default.example.com     1            6h9m    3 OK / 3     True
foo     http://foo-foo.example.com         1            74m     3 OK / 3     True
hello   http://hello-bar.example.com       1            72m     3 OK / 3     True
hello   http://hello-default.example.com   1            42h     3 OK / 3     True
hello   http://hello-foo.example.com       1            73m     3 OK / 3     True
After:
$ ./kn service list -A
NAMESPACE   NAME    URL                                GENERATION   AGE     CONDITIONS   READY   REASON
default     echo    http://echo-default.example.com    1            5h51m   3 OK / 3     True
default     foo     http://foo-default.example.com     1            5h51m   3 OK / 3     True
default     hello   http://hello-default.example.com   1            42h     3 OK / 3     True
bar         bar     http://bar-bar.example.com         1            54m     3 OK / 3     True
bar         echo    http://echo-bar.example.com        1            54m     3 OK / 3     True
bar         hello   http://hello-bar.example.com       1            54m     3 OK / 3     True
foo         echo    http://echo-foo.example.com        1            55m     3 OK / 3     True
foo         foo     http://foo-foo.example.com         1            56m     3 OK / 3     True
foo         hello   http://hello-foo.example.com       1            55m     3 OK / 3     True

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.

pkg/kn/commands/service/human_readable_flags.go

knative-prow-robot · 2019-08-15T09:20:27Z

Hi @toVersus. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

duglin · 2019-08-15T11:47:28Z

Clearly including the NS in there is +1 :-)

I'm just wondering about people using kn within scripts and possibly parsing the output, and whether having the number of columns change based on whether -A is used or not is a good or bad thing. Meaning, should NAMESPACE be there even w/o -A? Honestly, I can't decide. It might depend on whether people consider the use of -A to make it a totally new command or just a variant of service list and the same "output processor" should work for both.

I do parse output from cmds like this a lot, but I don't know if I'm in a very small minority.

toVersus · 2019-08-15T12:32:39Z

@duglin I'm not an enthusiast to use kn within my custom scripts, so I'm not sure about this :)

Meaning, should NAMESPACE be there even w/o -A

I think it feels redundant because kn service list or kn service list -n <namespace> will only print the services in specific namespace. It is clear for the users without NAMESPACE column.

It might depend on whether people consider the use of -A to make it a totally new command or just a variant of service list and the same "output processor" should work for both.

IMO, the motivation of -A option is to ensure the human readability, which is the same as -o wide option. It is not designed for improving machine readability for the use in scripts, so it is acceptable to change the number of columns.

navidshaikh · 2019-08-16T07:36:37Z

pkg/kn/commands/human_readable_flags.go

+	if err != nil {
+		fmt.Fprintln(c.OutOrStdout(), err)
+	}
+	f.AllNamespaces = all


Can we make this other way round and handle it in more generic way ?

As in, flags defined in HumanPrintFlags, gets added to all resource listing commands.

ToPrinter handles it for services, revisions and routes.

WDYT ?

I'm not sure I can handle it in more generic way, but I changed to handle --all-namespace option in a similar way to kubectl get.

pkg/kn/commands/service/human_readable_flags.go

pkg/kn/commands/service/service_list_flags.go

`kn service list` should print NAMESPACE column when specifying `--all-namespace` option, which follows the kubectl convention.

A priority field is handled as follows: - priority 0: print namespace column - priority 1: print default columns - priority 2: print additional columns for wide option

pkg/kn/commands/service/human_readable_flags.go

navidshaikh

Few things to clean up and a suggestion to add --with-namespace for kn service list command (not to be implemented right now but up for discussion).

pkg/kn/commands/human_readable_flags.go

pkg/kn/commands/service/service_list.go

pkg/kn/commands/service/human_readable_flags.go

pkg/kn/commands/human_readable_flags.go

navidshaikh

@toVersus a minor change request, we're good then. Thanks!

pkg/kn/commands/service/service_list_flags.go

pkg/kn/commands/service/service_list_mock_test.go

pkg/kn/commands/service/service_list_test.go

navidshaikh

/lgtm

knative-prow-robot · 2019-08-22T06:32:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: navidshaikh, toVersus

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:

~~OWNERS~~ [navidshaikh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-metrics-robot · 2019-08-22T06:34:18Z

The following is the coverage report on pkg/.
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	95.0%	92.5%	-2.5
pkg/kn/commands/service/service_list.go	87.5%	88.2%	0.7

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 15, 2019

knative-prow-robot requested review from cppforlife and sixolet August 15, 2019 09:20

knative-prow-robot reviewed Aug 15, 2019

View reviewed changes

pkg/kn/commands/service/human_readable_flags.go Outdated Show resolved Hide resolved

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 15, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 15, 2019

navidshaikh requested changes Aug 16, 2019

View reviewed changes

toVersus added 5 commits August 18, 2019 10:53

Make kn service list --all-namespace print namespace column

b752f15

`kn service list` should print NAMESPACE column when specifying `--all-namespace` option, which follows the kubectl convention.

Put services in default namespace at the top of the list

6967283

Switch output of column definitions by priority field

37354cc

A priority field is handled as follows: - priority 0: print namespace column - priority 1: print default columns - priority 2: print additional columns for wide option

Don't use AddFlags to bind all namespaces flag

33158c3

Update CHANGELOG

3041b97

toVersus force-pushed the feat/all-namespaces branch from 468455f to 3041b97 Compare August 18, 2019 01:54

maximilien reviewed Aug 19, 2019

View reviewed changes

pkg/kn/commands/service/human_readable_flags.go Outdated Show resolved Hide resolved

toVersus added 2 commits August 20, 2019 12:55

Remove commented codes

24b010e

Fix import path

badd4fd

navidshaikh requested changes Aug 20, 2019

View reviewed changes

Clean up

a85c3f0

navidshaikh requested changes Aug 21, 2019

View reviewed changes

pkg/kn/commands/service/service_list_flags.go Outdated Show resolved Hide resolved

pkg/kn/commands/service/service_list_mock_test.go Outdated Show resolved Hide resolved

pkg/kn/commands/service/service_list_test.go Show resolved Hide resolved

toVersus added 2 commits August 21, 2019 23:41

Remove unnecessary return

1b7576c

Swap the order of records

c131194

toVersus force-pushed the feat/all-namespaces branch from 029153e to c131194 Compare August 22, 2019 00:31

navidshaikh approved these changes Aug 22, 2019

View reviewed changes

knative-prow-robot assigned navidshaikh Aug 22, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 22, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2019

knative-prow-robot merged commit d0dc3b7 into knative:master Aug 22, 2019

toVersus deleted the feat/all-namespaces branch August 25, 2019 13:48

coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020

Ignore .gitignore, .gitattributes and AUTHORS on presubmit (knative#366)

2c930fb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(service list): Print NAMESPACE column as the first column when --all-namespaces is specified #366

feature(service list): Print NAMESPACE column as the first column when --all-namespaces is specified #366

toVersus commented Aug 15, 2019

knative-prow-robot left a comment

knative-prow-robot commented Aug 15, 2019

duglin commented Aug 15, 2019

toVersus commented Aug 15, 2019 •

edited

Loading

navidshaikh Aug 16, 2019 •

edited

Loading

toVersus Aug 18, 2019

navidshaikh left a comment

navidshaikh left a comment

navidshaikh left a comment

knative-prow-robot commented Aug 22, 2019

knative-metrics-robot commented Aug 22, 2019

feature(service list): Print NAMESPACE column as the first column when --all-namespaces is specified #366

feature(service list): Print NAMESPACE column as the first column when --all-namespaces is specified #366

Conversation

toVersus commented Aug 15, 2019

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

knative-prow-robot commented Aug 15, 2019

duglin commented Aug 15, 2019

toVersus commented Aug 15, 2019 • edited Loading

navidshaikh Aug 16, 2019 • edited Loading

Choose a reason for hiding this comment

toVersus Aug 18, 2019

Choose a reason for hiding this comment

navidshaikh left a comment

Choose a reason for hiding this comment

navidshaikh left a comment

Choose a reason for hiding this comment

navidshaikh left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Aug 22, 2019

knative-metrics-robot commented Aug 22, 2019

toVersus commented Aug 15, 2019 •

edited

Loading

navidshaikh Aug 16, 2019 •

edited

Loading