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

feat: Add plugin listing to "kn --help" #929

Merged
merged 10 commits into from
Jul 14, 2020

Conversation

rhuss
Copy link
Contributor

@rhuss rhuss commented Jul 13, 2020

This works on all levels. Test needs to be expanded still.

Fixes #266
Fixes #904

This works on all levels. Test needs to be expanded still.

Fixes knative#266
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 13, 2020
@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 13, 2020
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.

Good start. As you say definitely needs more tests. Don’t forget to add some e2e tests and perhaps verify output with expected that you can have in the e2e test.

Happy to take another pass when tests added.

}

commandParts := extractPluginCommandFromFileName(name)
if len(commandParts) != len(commandGroupParts)+1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get some English comments for these conditions... typically not for comments but when conditions are not obvious...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

@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
cmd/kn/main.go 78.1% 75.9% -2.1
pkg/kn/plugin/manager.go 84.1% 87.1% 2.9
pkg/templates/template_engine.go 87.5% 88.6% 1.1

@rhuss
Copy link
Contributor Author

rhuss commented Jul 14, 2020

/retest

@rhuss
Copy link
Contributor Author

rhuss commented Jul 14, 2020

@navidshaikh @maximilien tests are green, I also slurped in #910 so that we can get quicker to the release.

@rhuss
Copy link
Contributor Author

rhuss commented Jul 14, 2020

There is probably still someplace for improvement (e.g. how to get to the description of a plugin, for now just the path is printed). But maybe let's leave that for the next release.

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.

Looks good to me. So let's get this in
/lgtm

➜  client git:(pr/plugin-help) ✗ ./kn -h
kn is the command line interface for managing Knative Serving and Eventing resources

 Find more information about Knative at: https://knative.dev

Serving Commands:
  service     Manage Knative services
  revision    Manage service revisions
  route       List and describe service routes

Eventing Commands:
  source      Manage event sources
  broker      Manage message broker
  trigger     Manage event triggers

Other Commands:
  plugin      Manage kn plugins
  completion  Output shell completion code
  version     Show the version of this client

Plugins:
  curl        /Users/maximilien/.config/kn/plugins/kn-curl
  source-github /Users/maximilien/.config/kn/plugins/kn-source_github

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, 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 490fe57 into knative:master Jul 14, 2020
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugin handling if it retuns with non-zero exit status Consider adding plugin cmds to kn --help
5 participants