Fix kn source -h #846

danielhelfand · 2020-05-18T19:23:35Z

Description

This pull request adds a check for if -h or --help is specified with a plugin command. There should be no need to check for plugin executable availability if --help is specified and will avoid an error being returned that a command does not exist as documented in #845.

Changes

Add check for if -h or --help is specified in HandlePluginCommand
Add test

Reference

Fixes #845

/lint

knative-prow-robot

@danielhelfand: 0 warnings.

In response to this:

Description

This pull request adds a check for if -h or --help is specified with a plugin command. There should be no need to check for plugin executable availability if --help is specified and will avoid an error being returned that a command does not exist as documented in #845.

Changes

Add check for if -h or --help is specified in HandlePluginCommand

Add test

Reference

Fixes #845

/lint

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.

knative-prow-robot · 2020-05-18T19:23:48Z

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

rhuss · 2020-05-19T10:11:24Z

Thanks for the PR ! Could you a bit elaborate on your use case ? I'm asking because I think that a

kn --help admin should be the same as kn admin --help and print out the help message of the plugin itself (i.e. executing kn-admin --help). So I think checking for a valid plugin makes sense and only if no or an invalid plugin is given then print out the help (+ an error message if an unknown command/plugin is specified).

rhuss · 2020-05-19T10:12:07Z

/ok-to-test

danielhelfand · 2020-05-19T12:53:58Z

So I think checking for a valid plugin makes sense and only if no or an invalid plugin is given then print out the help (+ an error message if an unknown command/plugin is specified).

Thanks for the explanation. My apologies for the original attempt as I am not fully familiar with kn's plugin strategy. I pushed up a change that I think addresses this, but let me know if there are use cases I am not considering here.

Basically, instead of returning an error for if len(foundBinaryPath) == 0, move that check to before attempting to execute the plugin. If the plugin is invalid, the execution returns an error.

if len(foundBinaryPath) != 0 {
    err := pluginHandler.Execute(foundBinaryPath, append([]string{foundBinaryPath}, cmdArgs[len(remainingArgs):]...), os.Environ())
    if err != nil {
        return err
    }
}

danielhelfand · 2020-05-19T14:03:04Z

pkg/kn/core/root.go

@@ -87,7 +87,8 @@ func NewDefaultKnCommandWithArgs(rootCmd *cobra.Command,
 				fmt.Fprintf(rootCmd.OutOrStderr(), "Error: unknown command '%s' \nRun 'kn --help' for usage.\n", args[1])
 				os.Exit(1)
 			}
-		} else if foundCmd.HasSubCommands() {
+		}
+		if foundCmd.HasSubCommands() {


This can either be kept as an else if and checking would need to be done in the if block so an error is still returned if the subcommand is invalid, or it can be checked each time as its own block.

The change here would result in command suggestions being shown in addition to the error, which I think is helpful, but I don't have a strong opinion on it:

$ ./kn rev Error: unknown subcommand 'rev' for 'kn'. Available subcommands: completion, help, plugin, revision, route, service, source, trigger, version Run 'kn --help' for usage.

Thanks. I actually also had a fix in my pending PR for allowing extending all subcommands groups. But thanks for submitting this. I’ll merge mine.

rhuss · 2020-05-19T17:23:27Z

Thanks! I will have a look tomorrow but maybe we should wait until #834 is merged as I think it can conflict.

maximilien

Looks good. I would also add a small e2e test for help with -h With and without the source subcommand. Otherwise LGTM

maximilien · 2020-05-20T21:23:55Z

pkg/kn/core/root.go

@@ -87,7 +87,8 @@ func NewDefaultKnCommandWithArgs(rootCmd *cobra.Command,
 				fmt.Fprintf(rootCmd.OutOrStderr(), "Error: unknown command '%s' \nRun 'kn --help' for usage.\n", args[1])
 				os.Exit(1)
 			}
-		} else if foundCmd.HasSubCommands() {
+		}
+		if foundCmd.HasSubCommands() {


Thanks. I actually also had a fix in my pending PR for allowing extending all subcommands groups. But thanks for submitting this. I’ll merge mine.

knative-metrics-robot · 2020-05-26T13:27:51Z

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
pkg/kn/core/root.go	51.7%	52.8%	1.1

rhuss · 2020-05-27T05:32:53Z

@danielhelfand I think there is still a mismatch in the assertion of the e2e test (see test failure). Could you have a look please ?

rhuss · 2020-06-01T09:45:38Z

@danielhelfand would it be possible that we get this PR merged until tomorrow? I'm asking because I'm planning a bigger refactoring in this area (mostly because for implementing #579 and plugin help messages).

If not, it could be that we later have to change this PR considerably because of conflicts which are likely.

Also, tomorrow the next release will be cut, so would be cool if we could get that feature in.

danielhelfand · 2020-06-01T16:53:11Z

@rhuss Should be fixed with some minor tweaks I'll point out in review message. Let me know if there's any additional changes you would like, and I can address today. Thanks!

danielhelfand · 2020-06-01T16:56:11Z

test/e2e/basic_workflow_test.go

@@ -77,12 +77,11 @@ func TestWrongCommand(t *testing.T) {
 	r := test.NewKnRunResultCollector(t, it)
 	defer r.DumpIfFailed()

-	out := test.Kn{}.Run("source", "apiserver", "noverb", "--tag=0.13")


Seems with changes that flag validation is taking place, so error was coming back saying --tag is not a valid flag for kn source apiserver, which is valid since there is no --tag flag. I removed --tag for now.

rhuss

thanks, let's get that merge. Also to avoid conflicts later when I work on refactoring the help message for grouping.

/lgtm
/approve

knative-prow-robot · 2020-06-02T08:14:03Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~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 May 18, 2020

knative-prow-robot requested review from maximilien and navidshaikh May 18, 2020 19:23

knative-prow-robot reviewed May 18, 2020

View reviewed changes

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 May 18, 2020

knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 18, 2020

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 May 19, 2020

danielhelfand force-pushed the source_help branch from 4ecc502 to b9e123c Compare May 19, 2020 12:48

danielhelfand force-pushed the source_help branch from b9e123c to 287c591 Compare May 19, 2020 14:00

danielhelfand commented May 19, 2020

View reviewed changes

danielhelfand force-pushed the source_help branch from 287c591 to 0fdbe07 Compare May 19, 2020 14:14

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 May 19, 2020

danielhelfand force-pushed the source_help branch from 0fdbe07 to 02af555 Compare May 19, 2020 16:13

maximilien suggested changes May 20, 2020

View reviewed changes

danielhelfand force-pushed the source_help branch 2 times, most recently from 4f2beb7 to 1124015 Compare June 1, 2020 15:54

do not check for plugin executable if none on path

a5cb0a4

danielhelfand force-pushed the source_help branch from 1124015 to a5cb0a4 Compare June 1, 2020 16:16

danielhelfand commented Jun 1, 2020

View reviewed changes

rhuss approved these changes Jun 2, 2020

View reviewed changes

knative-prow-robot assigned rhuss Jun 2, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 2, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 2, 2020

knative-prow-robot merged commit 834ee79 into knative:master Jun 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix kn source -h #846

Fix kn source -h #846

danielhelfand commented May 18, 2020

knative-prow-robot left a comment

knative-prow-robot commented May 18, 2020

rhuss commented May 19, 2020

rhuss commented May 19, 2020

danielhelfand commented May 19, 2020 •

edited

Loading

danielhelfand May 19, 2020 •

edited

Loading

maximilien May 20, 2020

rhuss commented May 19, 2020

maximilien left a comment •

edited

Loading

maximilien May 20, 2020

knative-metrics-robot commented May 26, 2020

rhuss commented May 27, 2020

rhuss commented Jun 1, 2020

danielhelfand commented Jun 1, 2020 •

edited

Loading

danielhelfand Jun 1, 2020 •

edited

Loading

rhuss left a comment

knative-prow-robot commented Jun 2, 2020

Fix kn source -h #846

Fix kn source -h #846

Conversation

danielhelfand commented May 18, 2020

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

knative-prow-robot commented May 18, 2020

rhuss commented May 19, 2020

rhuss commented May 19, 2020

danielhelfand commented May 19, 2020 • edited Loading

danielhelfand May 19, 2020 • edited Loading

Choose a reason for hiding this comment

maximilien May 20, 2020

Choose a reason for hiding this comment

rhuss commented May 19, 2020

maximilien left a comment • edited Loading

Choose a reason for hiding this comment

maximilien May 20, 2020

Choose a reason for hiding this comment

knative-metrics-robot commented May 26, 2020

rhuss commented May 27, 2020

rhuss commented Jun 1, 2020

danielhelfand commented Jun 1, 2020 • edited Loading

danielhelfand Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jun 2, 2020

danielhelfand commented May 19, 2020 •

edited

Loading

danielhelfand May 19, 2020 •

edited

Loading

maximilien left a comment •

edited

Loading

danielhelfand commented Jun 1, 2020 •

edited

Loading

danielhelfand Jun 1, 2020 •

edited

Loading