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.

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

fix(core): generic solution for issue #111 for all command groups #218

Merged
merged 1 commit into from
Jul 3, 2019

Conversation

maximilien
Copy link
Contributor

@maximilien maximilien commented Jul 1, 2019

BFS exploration of all sub-commands from rootCmd and adds a RunE for all commands that are command groups with child sub-commands and without a RunE.

The added RunE will return the correct errors when the command is called with empty sub-command or with an unknown sub-command. It will also print the command help message first.


I did not add a test for this since the we need to decide on #185 and #214 and #134. Once we do I will add the gotest.tools or Ginkgo test for it.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 1, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien

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 added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 1, 2019
@maximilien
Copy link
Contributor Author

/assign @cppforlife

cc: @rhuss

@knative-prow-robot knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 1, 2019
@maximilien maximilien changed the title fixes(issue #111) generically for all command groups fixes(core) generic solution for issue #111 for all command groups Jul 1, 2019
@maximilien maximilien changed the title fixes(core) generic solution for issue #111 for all command groups fix(core) generic solution for issue #111 for all command groups Jul 1, 2019
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.

Works better than v1 fix of the issue.
@maximilien :

  1. Can we separate the help section and error message printed with a line please? helps make error message more visible / readable.
  2. I tested PR by running ./hack/build.sh, its reporting updates to command docs files docs/cmd/kn_revision.md and docs/cmd/kn_service.md as well, lets also include that with the PR.
  3. Can we add some tests please?

pkg/kn/core/root.go Outdated Show resolved Hide resolved
@rhuss
Copy link
Contributor

rhuss commented Jul 1, 2019

3\. Can we add some tests please?

I think @maximilien wants just to wait with tests until with which testing setup we want to continue. So let's wait until tomorrows WG meeting.

@maximilien maximilien changed the title fix(core) generic solution for issue #111 for all command groups fix(core): generic solution for issue #111 for all command groups Jul 2, 2019
@maximilien maximilien force-pushed the issue111v2 branch 2 times, most recently from bc3484e to 0abdb7d Compare July 2, 2019 01:28
@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 2, 2019
@maximilien maximilien force-pushed the issue111v2 branch 2 times, most recently from 97bd1b4 to bba7abb Compare July 2, 2019 05:41
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 2, 2019
@maximilien
Copy link
Contributor Author

maximilien commented Jul 2, 2019

OK @rhuss, I think I am convinced gotest.tools is better for this project. You were right from the start...

I added a test for root.go that also tests the functionality for issue #111 and I like the result. See latest update here.

cc: @navidshaikh and @cppforlife

@maximilien maximilien force-pushed the issue111v2 branch 2 times, most recently from 0c2141a to 8db55d0 Compare July 2, 2019 06:24
@rhuss
Copy link
Contributor

rhuss commented Jul 2, 2019

@maximilien I think you missed to check in the vendored files (use a build.sh -u to populate the vendor dir)

The PR looks good to me, let's merge it tomorrow if there are no objections against gotest.tools in today's WG call.

@maximilien
Copy link
Contributor Author

maximilien commented Jul 3, 2019

@maximilien I think you missed to check in the vendored files (use a build.sh -u to populate the vendor dir)

The PR looks good to me, let's merge it tomorrow if there are no objections against gotest.tools in today's WG call.

Yes, done, thanks @rhuss, one day I will remember to do this before pushing :) Converting all ginkgo tests now. Best.

Explores all sub-commands from root and adds a RunE for all
commands that are groups with child commands and without a RunE.
The added RunE will return the correct errors when the command
is called with empty sub-command or with an unknown sub-command.
It will also print the command help message first.

Added a gotest.tools test for root.go.
@knative-prow-robot knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 3, 2019
@knative-metrics-robot
Copy link

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/core/root.go Do not exist 52.5%

@rhuss
Copy link
Contributor

rhuss commented Jul 3, 2019

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2019
@rhuss
Copy link
Contributor

rhuss commented Jul 3, 2019

@maximilien cool, thanks !

Now that gotest.tools is in, we have to rebase our other PRs which introduce them (but should be hopefully trivial)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants