Migrate CLI to Cobra and add initial bash completion : fixes #17 #50

johnmccabe · 2017-08-26T11:01:39Z

Description

This PR ports the existing faas-cli across to the spf13/cobra library and adds an initial bash completion script.

Support for the legacy go flag based cli is maintained via the translateLegacyOpts(args []string) function in the legacy_cli.go file. It attempts to translate the old syntax in the new Cobra based verb/noun syntax, if successful it displays a warning to the user along with the translated new syntax command which it proceeds to execute. The opts/flags it supports translation for are set to those the cli supports at this point in time, we should ensure the new syntax is used for future documentation.

Motivation and Context

I have raised an issue to propose this change (required)

This PR moves to the Cobra CLI library in order to provide a richer command line experience using the verb/noun based approach that has seen wide adoption in the cloud/container space.

It also includes a first attempt at a bash completion file to aid use on the command line. It aims to address #17

New commands

The following commands are added, see the in-built faas-cli help command or --help flag for more detailed info on usage.

command	short description
`faas-cli`	Displays help
`faas-cli help`	Displays help
`faas-cli version`	Displays version information
`faas-cli build`	Builds OpenFaaS function images
`faas-cli deploy`	Deploys OpenFaaS function containers
`faas-cli remove`	Removes OpenFaaS function containers (has aliases `rm`, `delete`)
`faas-cli push`	Pushes OpenFaaS function images (currently to Docker Hub)

Bash completion

An initial bash completion file is provided in contrib/bash/faas-cil this can be sourced or added to your systems bash-completions directory (for example /usr/local/etc/bash_completion.d/ for a Brew installed completion on OSX).

Note that a bashcompletion command is also present but hidden pending an upstream fix (spf13/cobra#520) so that it works on OSX (which ships with an old version of Bash).

The completion will likely require need to be updated by hand, with the Docker implementation looking like a good template to follow (it works much better than the autogenerated Cobra file - which kubectl also uses and isn't great).

How Has This Been Tested?

Have run all the provided commands against a local FaaS platform, also added a suite of tests for the translation from the legacy cli to the new Cobra based syntax.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have signed-off my commits with git commit -s
I have added tests to cover my changes.
All new and existing tests passed.

ghost · 2017-08-26T11:01:41Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off.
That's something we need before your Pull Request can be merged. Please see our contributing guide.

johnmccabe · 2017-08-26T12:38:08Z

Summary breakdown of the changes to aid reviewing, as the inclusion of the vendored dependencies in the PR make it look more complicated than it actually is:

app.go has had its action logic broken out into the commands in /command/ and now just calls the commands.Execute() function trigger Cobra
command/faas.go is the root command that all the sub commands hang off and prints the help output if called on its own
command/build.go pulls the build logic from app.go into its own file along with flag handling etc
command/deploy.go same as above for deploy
command/remove.go same as above for delete (includes aliases for rm and delete)
command/version.go same as above for version
command/push.go same as above for push
commands/bash_completion.go generates a bash completion file but is hidden from help as mentioned in the PR description
commands/fetchTemplates.go just moved up into the commands package where its called from
Dockerfile updated to set the GitCommit in the commands package rather than main
vendor.conf pins cobra and its transitive dependency pflag added (its the bulk of the changes in the PR)
vendor/ vndr'd dependencies as mentioned above
legacy_cli.go/legacy_cli_tests.go adds a function to allow a translation from the old go flag based cli syntax to the new Cobra based syntax, it should allow old commands from blog posts etc to run successfully while prompting the user with the new syntax.

johnmccabe · 2017-08-26T12:38:55Z

I've help off touching docs etc until we're happy with the command syntax.

ghost · 2017-08-26T12:45:00Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off.
That's something we need before your Pull Request can be merged. Please see our contributing guide.

alexellis · 2017-08-26T12:48:10Z

What happens to prior users using the Golang/flags syntax?

johnmccabe · 2017-08-26T12:53:16Z

They currently don't work.. I've yet to figure out a way of bridging both worlds - I'll continue to dig into that.

Cobra takes the standard POSIX/GNU approach to flags, with - denoting single character flags so you can do stuff like -zxf ie setting 3 flags at once not a long flag called zxf, and it uses -- for long flags.

I can probably check for legacy flags before triggering the commands.Exec, either prompting the user to use the new format, or mapping old flags to new ones (maybe?).

Theres some discussion on the Go flag implementation here - https://groups.google.com/forum/#!topic/golang-nuts/3myLL-6mA94 (it is wrong imo)

alexellis · 2017-08-26T13:58:22Z

How to test John's PR

$ git fetch origin pull/50/head:switch-to-cobra
$ git checkout switch-to-cobra
$ ./build_redist.sh

Mac:

$ sudo cp ./faas-cli-darwin /usr/local/bin/faas-cli

Linux:

$ sudo cp ./faas-cli /usr/local/bin/faas-cli

alexellis · 2017-08-26T13:59:29Z

Here is an idea.. can we get the flag string before Cobra does and if it looks legacy print a warning instead? What if we take "-action" as a verb and flag (re-routing it to a different command) and deprecate it later?

johnmccabe · 2017-08-26T15:09:38Z

@alexellis already on that, and I think I may have a more graceful solution that prints a deprecation warning and automatically maps old flags to new ones, I've started working on that hopefully have a better idea as to whether its a good solution later tonight (only around intermittently this evening)

ghost · 2017-08-27T20:14:35Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off.
That's something we need before your Pull Request can be merged. Please see our contributing guide.

ghost · 2017-08-28T14:00:00Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off.
That's something we need before your Pull Request can be merged. Please see our contributing guide.

johnmccabe · 2017-08-28T14:31:16Z

@alexellis thats the PR rebased and with the legacy cli syntax translation added.

Can you have a look, and if you're happy I'll sort out doc changes.

alexellis · 2017-08-28T18:56:18Z

Thank you for the demo today. I'm very much looking forward to merging this 💯

Perhaps we could also add some notes on usage in the FaaS repo under the /guide/ folder and link there from the README? (Does not need to hold up testing/merging of this PR)

I'll also get a pre-release build together from your PR.

ericstoekl · 2017-08-29T02:19:26Z

It seems that faas-cli deploy -f <_filename_> does not remove existing functions before deploying, resulting in the function not updating. Is this by design? Also, do I need to install some packages to get tab completion to work?

johnmccabe · 2017-08-29T19:03:19Z

@ems5311 the deploy behaviour shouldn't have changed, do you see the same behaviour with the 0.4.8 cli?

As for the bash completion, I'll be adding some info to the docs tonight, will ping you on Slack when I add that.

ericstoekl · 2017-08-29T19:39:32Z

By 0.4.8 cli, do you mean the faas-cli version that builds from the master branch? If so, I see faas-cli -action deploy -f <filename> removes the function before deploying, wheras the version that this PR builds does not remove before deploying. However, I can issue faas-cli remove prior to deploy, and the same functionality is achieved. This is why I was wondering if it's by design.

johnmccabe · 2017-08-29T19:43:58Z

Yes the current release from master, thats a regression if its behaving differently for you, i'll try to reproduce and fix.

johnmccabe · 2017-08-29T20:05:28Z

@ems5311 ahh I know what this is - the default for the --replace flag changed from true to false. If you pass that flag it it'll work for you.

@alexellis regarding --replace having it default to true - the current cli behaviour - seems counter-intuitive, should this perhaps be --keep if you want the behaviour without the flag specified to remain the same?

ghost · 2017-08-29T20:13:13Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off.
That's something we need before your Pull Request can be merged. Please see our contributing guide.

alexellis · 2017-08-29T20:16:21Z

I'm ok with keeping things the same. Happy to revisit as I'd like to get to an update option further down the line too.

johnmccabe · 2017-08-29T20:20:20Z

ok I'll revert to having it default to true - will leave the replace flag in for the moment (it can be disabled with --replace=false ofc)

johnmccabe · 2017-08-29T20:26:51Z

@ems5311 I've reverted to the old replace behaviour, thanks for taking the time to spot this!!

ericstoekl · 2017-08-29T20:54:52Z

@johnmccabe Glad we found a resolution! Thanks for making these changes as well. 👍

ghost · 2017-08-29T21:42:27Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off.
That's something we need before your Pull Request can be merged. Please see our contributing guide.

ghost · 2017-08-30T12:21:33Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off.
That's something we need before your Pull Request can be merged. Please see our contributing guide.

johnmccabe · 2017-08-30T12:25:30Z

@alexellis I grabbed some time at lunch to add some info to the README on the bash completion.

I think this is now good to consider for merge.

alexellis · 2017-08-31T07:44:21Z

I think I'd like to see the commits squashed into one chunk. Are you OK with that?

johnmccabe · 2017-08-31T08:01:47Z

Would merging it down into 3 commits for vendor, docs and cli be useful?

alexellis

I've cut a second pre-release build as beta over here - https://github.com/alexellis/faas-cli/releases/tag/0.4.9-beta

I'd like to get the changes squashed for bringing into master too

Thanks for all your work on this

alexellis · 2017-08-26T12:49:05Z

Dockerfile

@@ -4,7 +4,7 @@ WORKDIR /go/src/github.com/alexellis/faas-cli
 COPY . .
 RUN go get -d -v

-RUN CGO_ENABLED=0 GOOS=linux go build --ldflags "-X main.GitCommit=${GIT_COMMIT}" -a -installsuffix cgo -o faas-cli .


Why did it need to change?

The GitCommit variable now lives in the commands package, in the version command itself, the update was to ensure that gets updated.

alexellis · 2017-08-26T12:51:18Z

commands/faas.go

@@ -0,0 +1,54 @@
+// Copyright (c) Alex Ellis 2017. All rights reserved.


Will probably need this on other files too

I'd gotten all the Go files as part of this, were you thinking about the templates etc? I'd be inclined to deal with that in a separate PR.

alexellis · 2017-08-27T20:40:15Z

commands/push.go

+
+// pushCmd handles pushing function container images to a remote repo
+var pushCmd = &cobra.Command{
+	Use:   "push -f YAML_FILE",


Is that meant to be all caps?

I've taken a lead from the Docker/Kubectl clis and their handling of args in the help text.

Usage: kubectl label [--overwrite] (-f FILENAME | TYPE NAME) KEY_1=VAL_1 ... KEY_N=VAL_N [--resource-version=version] [options] Usage: docker rm [OPTIONS] CONTAINER [CONTAINER...]

alexellis · 2017-08-27T20:41:10Z

legacy_cli_test.go

+	expectError  bool
+}{
+	{
+		title:        "legacy deploy action with all args, no =",


Great to see named test cases

alexellis · 2017-08-31T08:07:03Z

Whatever is easier for you @johnmccabe - I'd think one commit keeps everything together.

This adds the following commands: - faas-cli - faas-cli help - faas-cli build - faas-cli deploy - faas-cli remove (alias: rm) - faas-cli version - faas-cli push Note that the following is also added but hidden from help pending a more robust bash completion solution, initially using the Cobra generated bash completion but needs spf13/cobra#520 to merge before it'll work on the OSX default Bash 3.x. - faas-cli bashcompletion This commit intercepts the command line args passed to `faas-cli` and attempts to translate them from the deprecated go flag based syntax (`faas-cli -action xxx`) to the new Cobra verb/noun based syntax (`faas-cli xxx`), it also translates a frozen set of legacy flags (with the go-style single-dash) into a GNU style double-dash. Note that some special cases are included: - changing the delete action to remove - passing the function name as a noun to remove rather than as an arg to `-name` - it also handles the legacy format where args are passed after = (`-name=fnname`). If the translation results in a new set of args then a message is displayed to the user (stderr) telling warning that they are using the deprecated cli syntax and also prints the new syntax command that is being executed and which they should use going forward. Any errors thrown during translation result in the command failing with it printing the error cause to stderr. This renames the `fetchTemplates.go` file to use snake case. The convention appears to be for snakecase - as observed in both the Go and Kubernetes source. For example heres a random selection of source files. - https://github.com/kubernetes/kubernetes/blob/master/pkg/kubeapiserver/default_storage_factory_builder.go - https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/bash_comp_utils.go - https://github.com/golang/go/blob/master/src/compress/bzip2/move_to_front.go Note that the language spec does not set a hard rule for source file names, only for package names, but making this change for consistency. Note that this file was initially generated by Cobra, but has been tweaked to include some fixes. It it an experimental initial version. This commit adds some instructions on enabling the `faas-cli` bash auto-completion support. Instructions for Linux users are very light as it differs per-distro and the assumption is that Linux users should be capable of following their Distros instructions on enabling bash completion support. Signed-off-by: John McCabe <john@johnmccabe.net>

alexellis · 2017-08-31T14:57:05Z

Merging into master.

developius · 2017-08-31T20:49:51Z

This is awesome @johnmccabe, thank you! 😁 Been bugging Alex about faas-cli <verb> for a while 🎉

ghost added no-dco and removed no-dco labels Aug 26, 2017

alexellis mentioned this pull request Aug 27, 2017

Update fwatchdog to 0.6.1 and correct help message for "action" option #57

Merged

8 tasks

ghost added no-dco and removed no-dco labels Aug 27, 2017

ghost added no-dco and removed no-dco labels Aug 28, 2017

ghost added no-dco and removed no-dco labels Aug 29, 2017

ghost added no-dco and removed no-dco labels Aug 30, 2017

alexellis requested changes Aug 31, 2017

View reviewed changes

alexellis approved these changes Aug 31, 2017

View reviewed changes

alexellis merged commit 706761e into openfaas:master Aug 31, 2017

johnmccabe mentioned this pull request Sep 2, 2017

GitCommit in commands package not being updated at build time #62

Closed

		@@ -0,0 +1,54 @@
		// Copyright (c) Alex Ellis 2017. All rights reserved.

Migrate CLI to Cobra and add initial bash completion : fixes #17 #50

Migrate CLI to Cobra and add initial bash completion : fixes #17 #50

Conversation

johnmccabe commented Aug 26, 2017 • edited Loading

Description

Motivation and Context

New commands

Bash completion

How Has This Been Tested?

Types of changes

Checklist:

ghost commented Aug 26, 2017

johnmccabe commented Aug 26, 2017 • edited Loading

johnmccabe commented Aug 26, 2017

ghost commented Aug 26, 2017

alexellis commented Aug 26, 2017

johnmccabe commented Aug 26, 2017 • edited Loading

alexellis commented Aug 26, 2017

How to test John's PR

alexellis commented Aug 26, 2017 • edited Loading

johnmccabe commented Aug 26, 2017

ghost commented Aug 27, 2017

ghost commented Aug 28, 2017

johnmccabe commented Aug 28, 2017

alexellis commented Aug 28, 2017

ericstoekl commented Aug 29, 2017 • edited Loading

johnmccabe commented Aug 29, 2017

ericstoekl commented Aug 29, 2017

johnmccabe commented Aug 29, 2017

johnmccabe commented Aug 29, 2017

ghost commented Aug 29, 2017

alexellis commented Aug 29, 2017

johnmccabe commented Aug 29, 2017 • edited Loading

johnmccabe commented Aug 29, 2017

ericstoekl commented Aug 29, 2017

ghost commented Aug 29, 2017

ghost commented Aug 30, 2017

johnmccabe commented Aug 30, 2017

alexellis commented Aug 31, 2017

johnmccabe commented Aug 31, 2017

alexellis left a comment

Choose a reason for hiding this comment

alexellis Aug 26, 2017

Choose a reason for hiding this comment

johnmccabe Aug 31, 2017

Choose a reason for hiding this comment

alexellis Aug 26, 2017

Choose a reason for hiding this comment

johnmccabe Aug 31, 2017

Choose a reason for hiding this comment

alexellis Aug 27, 2017

Choose a reason for hiding this comment

johnmccabe Aug 31, 2017

Choose a reason for hiding this comment

alexellis Aug 27, 2017

Choose a reason for hiding this comment

alexellis commented Aug 31, 2017

alexellis commented Aug 31, 2017

developius commented Aug 31, 2017

johnmccabe commented Aug 26, 2017 •

edited

Loading

johnmccabe commented Aug 26, 2017 •

edited

Loading

johnmccabe commented Aug 26, 2017 •

edited

Loading

alexellis commented Aug 26, 2017 •

edited

Loading

ericstoekl commented Aug 29, 2017 •

edited

Loading

johnmccabe commented Aug 29, 2017 •

edited

Loading