Fixes issue #111 by responding with an error for unknown subcommands

[maximilien]

➜  client git:(issue111) ✗ ./kn service unknown
unknown command "unknown" for "kn service"

This works for both the service and revision groups.

[maximilien]

Fixes #111

cc: @sixolet

[maximilien]

/test pull-knative-client-integration-tests

[maximilien]

BTW, FYI, just for kicks...

I implemented this feature for the plugin group in #177 and also added a Ginkgo UT.

The resulting test is so much more readable than what's here--albeit a bit longer (precisely ~25 lines more).

However, the clarity of the test is worth noting. And it also tests all aspects of the plugin command group cleanly, i.e., creation, known and unkown subcommands. Whereas the go test version only tests the bad path. So they might end up being the same length...

Anyhow, I am pasting here (w/o import), so you see:

1. Go test

import (
   ...
)

func fakeService(args []string, response *v1alpha1.ServiceList) (action client_testing.Action, output []string, err error) {
	knParams := &commands.KnParams{}
	cmd, fakeServing, buf := commands.CreateTestKnCommand(NewServiceCommand(knParams), knParams)
	fakeServing.AddReactor("*", "*",
		func(a client_testing.Action) (bool, runtime.Object, error) {
			action = a
			return true, response, nil
		})
	cmd.SetArgs(args)
	err = cmd.Execute()
	if err != nil {
		return
	}
	output = strings.Split(buf.String(), "\n")
	return
}

func TestUnknownSubcommand(t *testing.T) {
	_, _, err := fakeService([]string{"service", "unknown"}, &v1alpha1.ServiceList{})
	if err == nil {
		t.Error(err)
		return
	}

	if err.Error() != "unknown command \"unknown\" for \"kn service\"" {
		t.Errorf("Bad error message %s", err.Error())
	}
}

2. Ginkgo test

import (
   ...
)

var _ = Describe("kn service", func() {
	var (
		serviceCmd *cobra.Command
		knParams  *KnParams
	)

	BeforeEach(func() {
		knParams = &KnParams{}
		serviceCmd = NewServiceCommand(knParams)
	})

	Describe("#NewServiceCommand", func() {
		It("creates a new cobra.Command", func() {
			Expect(serviceCmd).NotTo(BeNil())
			Expect(serviceCmd.Use).To(Equal("service"))
			Expect(serviceCmd.Short).To(Equal("Service command group"))
			Expect(serviceCmd.Long).To(ContainSubstring("Provides utilities for interacting with services."))
			Expect(serviceCmd.Args).NotTo(BeNil())
			Expect(serviceCmd.Run).NotTo(BeNil())
		})

		Context("called with known subcommand", func() {
			var (
				fakeExecuted bool
			)

			BeforeEach(func() {
				serviceCmd.AddCommand(&cobra.Command{
					Use:   "fake",
					Short: "fake subcommand",
					RunE: func(cmd *cobra.Command, args []string) error {
						fakeExecuted = true
						return nil
					},
				})
			})

			It("executes the subcommand RunE func", func() {
				serviceCmd.SetArgs([]string{"fake"})
				err := serviceCmd.Execute()
				Expect(err).NotTo(HaveOccurred())
				Expect(fakeExecuted).To(BeTrue())
			})
		})

		Context("called with unknown subcommand", func() {
			It("fails with 'unknown command' message", func() {
				serviceCmd.SetArgs([]string{"unknown"})
				err := serviceCmd.Execute()
				Expect(err).To(HaveOccurred())
				Expect(err.Error()).To(ContainSubstring("unknown command \"unknown\" for"))
			})
		})
	})
})

[navidshaikh]

➜  client git:(drmax-issue111) ./kn revision unkown
unknown command "unkown" for "kn revision"

fine! but,

➜  client git:(drmax-issue111) ./kn revision       
➜  client git:(drmax-issue111) echo $?
0

only kn revision should also give error/help message

[maximilien]

only kn revision should also give error/help message

I can do that. Was not sure it made sense to error, but I guess we can treat it as invalid "" command/flags.

Will update today. Thx

[maximilien]

Done @navidshaikh. Let me know what you think. Best.

[maximilien]

/test pull-knative-client-integration-tests

[maximilien]

/retest

[navidshaikh]

Works! , but this change should also be applied to other commands as well for eg kn version unkown (or completion) should also prompt the error.

[maximilien]

Works! , but this change should also be applied to other commands as well for eg kn version unkown (or completion) should also prompt the error.

Well this was for group commands. I added it to the plugin WIP #177, which AFAIK is the only other "group" command in works so far.

The other commands, not under the current group commands, version and completion are leaf commands. I think we should discuss with broader team as to whether silently ignoring extra params or whether outputting an error is warranted.

Mainly because if the leaf command takes a param then it will deal with it naturally in the implementation of the command. If it does not, like for version and completion, then the question becomes to ignore or not.

Worth noting that kubectl version which is similarly a leaf command with no params silently ignores extra params:

kubectl version blah
Client Version: version.Info{Major:"1", Minor:"8", GitVersion:"v1.8.0", GitCommit:"0b9efaeb34a2fc51ff8e4d34ad9bc6375459c4a4", GitTreeState:"clean", BuildDate:"2017-09-29T05:56:06Z", GoVersion:"go1.9", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"13", GitVersion:"v1.13.6+IKS", GitCommit:"ac5f7341d5d0ce8ea8f206ba5b030dc9e9d4cc97", GitTreeState:"clean", BuildDate:"2019-05-09T13:26:51Z", GoVersion:"go1.11.5", Compiler:"gc", Platform:"linux/amd64"}

[sixolet]

Can we print the usage to stdout and make it an error?

[maximilien]

Can we print the usage to stdout and make it an error?

Do you mean for:

kn service lol
Manage your Knative building blocks:

Serving: Manage your services and release new software to them.
Build: Create builds and keep track of their results.
Eventing: Manage event subscriptions and channels. Connect up event sources.

Usage:
  kn [command]

Available Commands:
  completion  Output shell completion code (default Bash)
  help        Help about any command
  revision    Revision command group
  service     Service command group
  version     Prints the client version

Flags:
  -h, --help                help for kn
      --kubeconfig string   kubectl config file (default is $HOME/.kube/config)

Use "kn [command] --help" for more information about a command.

unknown command "lol" for "kn service"

For both service and revision. Confirm and I will update.

[sixolet]

... For both service and revision. Confirm and I will update.

Yeah, exactly 👍

[sixolet]

... actually, not exactly. Should print the equivalent for the subcommand.

[maximilien]

... actually, not exactly. Should print the equivalent for the subcommand.

Yup, got you. Makes sense. Will do that.

[maximilien]

OK done. Updated the commit message but worth repeating here for clarity.

➜  client git:(issue111) ✗ ./kn service unknown
Service command group

Usage:
  kn service [flags]
  kn service [command]

Available Commands:
  create      Create a service.
  delete      Delete a service.
  describe    Describe available services.
  list        List available services.
  update      Update a service.

Flags:
  -h, --help   help for service

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

Use "kn service [command] --help" for more information about a command.
unknown command "unknown" for "kn service"

➜  client git:(issue111) ✗ ./kn revision
Revision command group

Usage:
  kn revision [flags]
  kn revision [command]

Available Commands:
  describe    Describe revisions.
  list        List available revisions.

Flags:
  -h, --help   help for revision

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

Use "kn revision [command] --help" for more information about a command.
please provide a valid sub-command for "kn revision"

This works for both the service and revision groups when called with empty and unknown sub-command.

Note also that the tests do not check for the usage output. Complicated to do with our current UT framework. Will update that when #161 is done and we start moving to Ginkgo. I'll have an example in #177 for plugin group.

[maximilien]

/retest

[maximilien]

OK so pull-knative-client-build-tests is failing with:

2019/06/20 22:25:45 Error creating license classifier: cannot register licenses: EOF

Not clear why. I've checked header files for licenses and our own ./build.sh does that. Dependencies are updated and all UTs and e2e locals are running fine.

Perhaps this is infra or intermittent. Trying again one more time today:

/test pull-knative-client-build-tests

[rhuss]

@maximilien you can reproduce the error locally with

go install github.com/knative/test-infra/tools/dep-collector
dep-collector -check $(go list ./...)

(btw, I could add this to the build script, too, when checking for licenses).

For me this doesn't yield any error, though, so it looks a bit like a flake for me. Let's retest it again.

[rhuss]

/retest

[adrcunha]

/retest

[googlebot]

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]

[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:

• OWNERS [maximilien]

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

[knative-metrics-robot]

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/service/service.go	100.0%	91.7%	-8.3

[rhuss]

@maximilien you probably rebase and squash your commits as this PR currently involves also commits from other contributors (thats probably the reason why the CLA check fails).

[maximilien]

OK since the new code is vastly different from this one. I am closing this PR. We will still have the good discussions but can use new up coming PR for new discussion on the new more general PR.

[knative-prow-robot]

@maximilien: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-knative-client-integration-tests	f7b4eb1	link	/test pull-knative-client-integration-tests

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

[maximilien]

See #218