-
Notifications
You must be signed in to change notification settings - Fork 226
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
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
Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. |
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:
|
I've help off touching docs etc until we're happy with the command syntax. |
Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. |
What happens to prior users using the Golang/flags syntax? |
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 I can probably check for legacy flags before triggering the Theres some discussion on the Go flag implementation here - https://groups.google.com/forum/#!topic/golang-nuts/3myLL-6mA94 (it is wrong imo) |
How to test John's PR
Mac:
Linux:
|
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? |
@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) |
Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. |
Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. |
@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. |
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. |
It seems that |
@ems5311 the deploy behaviour shouldn't have changed, do you see the same behaviour with the As for the bash completion, I'll be adding some info to the docs tonight, will ping you on Slack when I add that. |
By 0.4.8 cli, do you mean the faas-cli version that builds from the master branch? If so, I see |
Yes the current release from master, thats a regression if its behaving differently for you, i'll try to reproduce and fix. |
@ems5311 ahh I know what this is - the default for the @alexellis regarding |
Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. |
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. |
ok I'll revert to having it default to true - will leave the |
@ems5311 I've reverted to the old replace behaviour, thanks for taking the time to spot this!! |
@johnmccabe Glad we found a resolution! Thanks for making these changes as well. 👍 |
Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. |
Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. |
@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. |
I think I'd like to see the commits squashed into one chunk. Are you OK with that? |
Would merging it down into 3 commits for vendor, docs and cli be useful? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did it need to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GitCommit
variable now lives in the commands
package, in the version command itself, the update was to ensure that gets updated.
@@ -0,0 +1,54 @@ | |||
// Copyright (c) Alex Ellis 2017. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will probably need this on other files too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
// pushCmd handles pushing function container images to a remote repo | ||
var pushCmd = &cobra.Command{ | ||
Use: "push -f YAML_FILE", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that meant to be all caps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...]
expectError bool | ||
}{ | ||
{ | ||
title: "legacy deploy action with all args, no =", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see named test cases
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>
Merging into master. |
This is awesome @johnmccabe, thank you! 😁 Been bugging Alex about |
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 thelegacy_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
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.faas-cli
faas-cli help
faas-cli version
faas-cli build
faas-cli deploy
faas-cli remove
rm
,delete
)faas-cli push
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
Checklist:
git commit -s