Fix issue #205 to add route describe feature and test by zhangtbj · Pull Request #251 · knative/client

zhangtbj · 2019-07-09T09:19:20Z

1, Implement kn route describe command with required parameters:

✔ $ go run cmd/kn/main.go route describe --help
Describe available route.

Usage:
  kn route describe NAME [flags]

Flags:
      --allow-missing-template-keys   If true, ignore any errors in templates when a field or map key is missing in the template. Only applies to golang and jsonpath output formats. (default true)
  -h, --help                          help for describe
  -n, --namespace string              List the requested object(s) in given namespace.
  -o, --output string                 Output format. One of: json|yaml|name|go-template|go-template-file|template|templatefile|jsonpath|jsonpath-file. (default "yaml")
      --template string               Template string or path to template file to use when -o=go-template, -o=go-template-file. The template format is golang templates [http://golang.org/pkg/text/template/#pkg-overview].

Global Flags:
      --config string       config file (default is $HOME/.kn/config.yaml)
      --kubeconfig string   kubectl config file (default is $HOME/.kube/config)

2, Add gotest.tools tests for kn route describe commands:

test without route name
test route describe with the default output
test route describe with the YAML output
test route describe with the JSON output
test route describe with the name output

Also rebased from master and change to use new knClient.

For issue: #205

knative-prow-robot · 2019-07-09T09:19:34Z

Hi @zhangtbj. 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.

Details

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.

zhangtbj · 2019-07-09T11:41:03Z

Hi @maximilien , @rhuss and @navidshaikh ,

Sorry I closed the previous PR #228 by mistake, this is the new one which is rebased from master and change to use new knClient.

I think for the discussion for #206 and #235, it should be related to command Printer, should not change in each sub describe command.

For example:
1, without parameter, should show common properties, like resource name, ns, status....
2, with --all, show all info like now
3, with --color, no change for showing

So I think these should be the same for all subcommands, only change the implementation in Printer code.

I am not sure if we could accept this PR first then change or refine the describe/show format for all subcommands.

Thanks all!:)

rhuss

Thanks for the PR and for adapting it to the lates refactorings.

As mentioned in #235 , the kn route describe is primarly (if not exclusively) for human readable output. If I understand the PR correctly, it returns the route as yaml (machine readable) presentation.

I'm not sure why the renaming of the files was needed wrt/ this PR. Could you please revert ?

As an intermediate step, I'm happy to include but we should go to a more human readable output as it is under the way for kn service describe.

rhuss · 2019-07-09T13:58:50Z

/ok-to-test

zhangtbj · 2019-07-09T16:37:29Z

Thanks for the review @rhuss

About I'm not sure why the renaming of the files was needed wrt/ this PR. Could you please revert ?

Do you mean the change like this in PR?:
pkg/kn/commands/route/list.go → pkg/kn/commands/route/route_list.go

I took a look the other command file styles, they are all format like this <noun>_<verb>.go, like revision_describe_test.go, in route folder, there is only list subcommand and named as list.go directly.

I think it makes people confusing (what list?) and not sync with other commands.

If this is not a good place to fix it in this PR, I can revert it in here and just for route describe function.

rhuss · 2019-07-09T17:21:33Z

Do you mean the change like this in PR?:
pkg/kn/commands/route/list.go → pkg/kn/commands/route/route_list.go

Yes. Tbh, we already have a PR pending which does it the other way round for the other files #245 . So please revert that change and we can discuss that in another context.

zhangtbj · 2019-07-10T03:53:50Z

Sure thing @rhuss . I have reverted the renaming change. Pls review again. Thanks!

maximilien · 2019-07-10T16:49:34Z

As an intermediate step, I'm happy to include but we should go to a more human readable output as it is under the way for kn service describe.

Agree. Default output should be for us humans :)

maximilien · 2019-07-10T16:59:05Z

+	})
+
+	t.Run("describe a valid route with special output", func(t *testing.T) {
+		t.Run("describe a valid route with YAML output", func(t *testing.T) {


to simplify and make the test run output easier, I'd avoid duplicating the descriptions when nesting these t.Run("...") so for instance, I'd name these as follows:

[...] t.Run("describe a valid route with special output", func(t *testing.T) { t.Run("YAML", func(t *testing.T) { [...] }) }) [...]

See output of test runs to see what I mean. Try to do the same in subsequent nested t.Run(...)

Thanks Max, I have fixed in my PR

maximilien · 2019-07-10T17:07:46Z

+		validateGroupVersionKind(t, route)
+	})
+
+	t.Run("get unknown route name returns error", func(t *testing.T) {


Is there a need for a check for invalid route names?

Hi Max, I think it is better for us to cover the invalid failure case and I found we also have this invalid cases for service and revision.

No I was thinking about user’s passing a route name like @#$Q!$# or anything with invalid HTTP char will be invalid. The API might have a specification as to what route names are valid.

The key for me is not so much that we guard against the valid or invalid name but that we tell user a message that’s easy to understand.

Hi @maximilien , got it. I have tried, no matter it is a non-exist or invalid name, kn client can report routes.serving.knative.dev "xxx" not found correctly without any http error. such as:

jordan at jordandembp.cn.ibm.com in ~/Work/workspace/go/src/github.com/knative/client [master|✔] ✘-1 $ go run cmd/kn/main.go route describe he$low@rld routes.serving.knative.dev "he@rld" not found exit status 1 jordan at jordandembp.cn.ibm.com in ~/Work/workspace/go/src/github.com/knative/client [master|✔] ✘-1 $ go run cmd/kn/main.go route describe hello routes.serving.knative.dev "hello" not found exit status 1

So I also change the test route name to r@ute-that-d$es-n#t-exist for spceial case.

I think we can rely on the error message from the API client to point in the right direction. I don't think we need an extra validation of the name on our side.

rhuss

Some minor comments around an unrealistic test. Otherwise looks good to me.

rhuss · 2019-07-12T08:36:30Z

+	t.Run("describe a valid route with default output", func(t *testing.T) {
+		setup(t)
+
+		action, output, err := fakeRouteDescribe([]string{"route", "describe", "test-foo"}, &expectedRoute)


So you are requesting a route with name "test-foo" but return a route with Route.Name == "foo". IMO the name you are are requesting and the returned name should be the same.

Hi @rhuss , I have refined it. Thanks!

rhuss · 2019-07-12T08:39:28Z

+		validateGroupVersionKind(t, route)
+	})
+
+	t.Run("get unknown route name returns error", func(t *testing.T) {


I think we can rely on the error message from the API client to point in the right direction. I don't think we need an extra validation of the name on our side.

knative-metrics-robot · 2019-07-12T09:14:11Z

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/route/route_describe.go	Do not exist	78.3%
pkg/serving/v1alpha1/client.go	86.0%	86.0%	-0.0

rhuss

/approve
/lgtm

knative-prow-robot · 2019-07-12T12:06:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhuss, zhangtbj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhuss]

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

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

knative-prow-robot requested review from cppforlife and rhuss July 9, 2019 09:19

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 Jul 9, 2019

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

zhangtbj mentioned this pull request Jul 9, 2019

Fix issue #205 to add route describe feature and test #228

Closed

rhuss suggested changes Jul 9, 2019

View reviewed changes

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 9, 2019

maximilien reviewed Jul 10, 2019

View reviewed changes

Comment thread pkg/kn/commands/route/route_describe_test.go

maximilien reviewed Jul 10, 2019

View reviewed changes

rhuss suggested changes Jul 12, 2019

View reviewed changes

add route describe feature and test

7d42857

rhuss approved these changes Jul 12, 2019

View reviewed changes

Comment thread pkg/kn/commands/route/route_describe_test.go

knative-prow-robot assigned rhuss Jul 12, 2019

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

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

knative-prow-robot merged commit 385f848 into knative:master Jul 12, 2019

Conversation

zhangtbj commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-prow-robot commented Jul 9, 2019

Uh oh!

zhangtbj commented Jul 9, 2019

Uh oh!

rhuss left a comment

Choose a reason for hiding this comment

Uh oh!

rhuss commented Jul 9, 2019

Uh oh!

zhangtbj commented Jul 9, 2019

Uh oh!

rhuss commented Jul 9, 2019

Uh oh!

zhangtbj commented Jul 10, 2019

Uh oh!

maximilien commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhuss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Jul 12, 2019

Uh oh!

rhuss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

knative-prow-robot commented Jul 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zhangtbj commented Jul 9, 2019 •

edited

Loading

maximilien commented Jul 10, 2019 •

edited

Loading