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

zhangtbj · 2019-07-05T03:09:46Z

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

For issue: #205

knative-prow-robot · 2019-07-05T03:10:00Z

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-05T03:10:19Z

/assign @maximilie

knative-prow-robot · 2019-07-05T03:10:20Z

@zhangtbj: GitHub didn't allow me to assign the following users: maximilie.

Note that only knative members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/assign @maximilie

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.

maximilien · 2019-07-05T03:21:46Z

/ok-to-test

knative-metrics-robot · 2019-07-05T03:22:54Z

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
pkg/kn/commands/route/route_describe.go	Do not exist	79.2%
pkg/kn/commands/route/route_list.go	Do not exist	76.7%
pkg/kn/commands/route/route_list_flags.go	Do not exist	66.7%

maximilien · 2019-07-05T03:28:39Z

+	}
+
+	t.Run("requires the route name", func(t *testing.T) {
+		_, _, err := fakeRouteDescribe([]string{"route", "describe"}, &v1alpha1.Route{})


Why is setup(t) not called here? Perhaps do it for consistency

Hi Max, because this test is only for wihtout route name test. So doesn't need to initialize any fake route in setup. So I skip it. If we hope to be consistent. I can add it back. Thanks!

maximilien · 2019-07-05T03:30:47Z

+		assert.Assert(t, err == nil)
+		assert.Assert(t, action != nil)
+		assert.Assert(t, action.Matches("get", "routes"))
+


Not sure I see the value of JSON conversion here. Why not just parse the YAML returned (it’s the default output)

Yes, in this test, I parse to json format and Unmarshal it to a route object returnedRoute. Then compare it with expectedRoute

maximilien · 2019-07-05T03:32:49Z

Nice job @zhangtbj left a few minor comments. Adding @navidshaikh as reviewer too.

maximilien · 2019-07-05T03:33:11Z

/assign @navidshaikh

knative-prow-robot · 2019-07-05T03:33:12Z

@maximilien: GitHub didn't allow me to assign the following users: navidshaikh.

Note that only knative members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/assign @navidshaikh

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.

rhuss · 2019-07-05T05:59:20Z

@zhangtbj coudl you please rebase on master ? Gotest.tools is already included on master but also part of this PR. This should not be the case.

rhuss · 2019-07-05T06:01:31Z

+			if err != nil {
+				return err
+			}
+			describeRoute.GetObjectKind().SetGroupVersionKind(schema.GroupVersionKind{


Can we please get #134 merged first as this directly is changed in this PR ?

I mark this PR as 'on-hold' if this ok, and remove it if we have #134 in place. 'hope this is ok.

rhuss · 2019-07-05T06:01:59Z

+				return err
+			}
+			describeRoute.GetObjectKind().SetGroupVersionKind(schema.GroupVersionKind{
+				Group:   "knative.dev",


its the wrong group anyway, but that gets fixed with #134.

rhuss

Thanks for the PR ! Technical its ok (except the missing rebase on master), but we should really agree what <noun> describe shoulde:

Should it, like for kubectl, only be targetting human-readable output ? (there is no -o option for 'kubectl describe')
Should -o yaml|json|name be exclusively reserved for kn revision list (with kubectl get being the equivalent for kubectl) ? Or is it ok to have it for kn revision describe to ?
Would be we good to rename describe to show to make the deviation to kubectl more clear ? We already went from get to list so moving from describe to show would be consiquent (and it would be then also easier to deviate from kubectl describe behaviour like adding -o options).

@maximilien @sixolet @cppforlife @navidshaikh any opinions on this ?

rhuss · 2019-07-05T06:06:12Z

+				Group:   "knative.dev",
+				Version: "v1alpha1",
+				Kind:    "Route"})
+			err = printer.PrintObj(describeRoute, cmd.OutOrStdout())


'route describe' (or as I would love to rename it to 'route show', but that's another discussion ;) should imo really be about human readable output. I think its ok to have -o name|json|yaml, too but the main focus is to have a detaile out of the the routes, what revisions they are targetting which with which split and which service the route is targetting.

See #75 for the idea (although this PR is too huge and as soon as #134 is merged it will be broken down).

navidshaikh · 2019-07-05T09:19:38Z

@zhangtbj : Thanks for your PR.
show (or describe verb) for a resource should print better representation of a resource's detailed info as compare to printing yaml for same. This is the reason we have revision describe #206 open.

Please check work done by @rhuss on #75 , we'd like to produce something like that. I'd say lets discuss info we'd like show for route show or describe on the issue.

rhuss · 2019-07-05T15:33:12Z

I opened #235 for discussion the general direction we want to go with kn <noun> show (or kn <noun> describe)

maximilien · 2019-07-08T21:59:29Z

@rhuss are we adding this to discussion tomorrow or do we already have too much?

In any case, let's make sure to document decision(s) in WG doc or here as @zhangtbj is in Beijing and the time for the meeting is 1:30a so it's not clear he can attend in person.

googlebot · 2019-07-09T01:16:13Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

knative-prow-robot · 2019-07-09T01:20:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

zhangtbj · 2019-07-09T11:30:42Z

Sorry I closed this PR by mistake. I created a new PR #251 which is rebased from master and change to use new knClient. Thanks!

…y providing goos and goarch. (knative#228) * add go build with goos and goarch * add a example at help message --------- Co-authored-by: Kaustubh Pande <kaustubhpande.kp@gmail.com>

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

knative-prow-robot requested review from evankanderson and maximilien July 5, 2019 03:09

knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 5, 2019

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 5, 2019

maximilien reviewed Jul 5, 2019

View reviewed changes

zhangtbj mentioned this pull request Jul 5, 2019

Implement kn route describe command #205

Closed

rhuss reviewed Jul 5, 2019

View reviewed changes

rhuss suggested changes Jul 5, 2019

View reviewed changes

googlebot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels Jul 9, 2019

zhangtbj closed this Jul 9, 2019

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 9, 2019

zhangtbj mentioned this pull request Jul 9, 2019

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

Merged

Conversation

zhangtbj commented Jul 5, 2019

Uh oh!

knative-prow-robot commented Jul 5, 2019

Uh oh!

zhangtbj commented Jul 5, 2019

Uh oh!

knative-prow-robot commented Jul 5, 2019

Uh oh!

maximilien commented Jul 5, 2019

Uh oh!

knative-metrics-robot commented Jul 5, 2019

Uh oh!

maximilien Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximilien Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

zhangtbj Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

maximilien commented Jul 5, 2019

Uh oh!

maximilien commented Jul 5, 2019

Uh oh!

knative-prow-robot commented Jul 5, 2019

Uh oh!

rhuss commented Jul 5, 2019

Uh oh!

rhuss Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

rhuss Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

rhuss left a comment

Choose a reason for hiding this comment

Uh oh!

rhuss Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

navidshaikh commented Jul 5, 2019

Uh oh!

rhuss commented Jul 5, 2019

Uh oh!

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

Uh oh!

Uh oh!

googlebot commented Jul 9, 2019

Uh oh!

knative-prow-robot commented Jul 9, 2019

Uh oh!

zhangtbj commented Jul 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

zhangtbj Jul 5, 2019 •

edited

Loading

maximilien commented Jul 8, 2019 •

edited

Loading