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

Refactoring Commands to exit with non-0 status codes on failure. #207

Merged
merged 5 commits into from Nov 14, 2017

Conversation

Projects
None yet
4 participants
@tommysolsen
Contributor

tommysolsen commented Nov 6, 2017

Signed-off-by: Tommy Stigen Olsen tommysolsen@gmail.com

As discussed in #183, I have gone through the commands and made all of the cobra commands
use RunE, returning errors which triggers os.Exit(1).

Description

Refactors all cobra commands except version (which never generates errors). This lets all of the commands exit with status-code 1 if an error occurs.
I have also cleaned up some of the different ways each function handles failure to a standard fmt.Println and return err style to be consistent across the modules.

Since RunE will print any error it receives, i've suppressed them so the software doesnt repeat itself.

Motivation and Context

See #183

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

How Has This Been Tested?

Tested through compiling and running a few functions that failed.

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.
@alexellis

Looks sensible. Please can you show with console output how you've tested each scenario edited?

@tommysolsen

This comment has been minimized.

Show comment
Hide comment
@tommysolsen

tommysolsen Nov 7, 2017

Contributor

I've pushed some new code. I've started going through the commands one more time to capture the output and found a bug that slipped me last time. If a user would try to deploy with a malformed label not including a =, you would get a index out of range panic. Added a catch for that.

I'll be a little more thorough and retest everything to make sure everything is as its supposed to. Will post the results when I'm done.

Contributor

tommysolsen commented Nov 7, 2017

I've pushed some new code. I've started going through the commands one more time to capture the output and found a bug that slipped me last time. If a user would try to deploy with a malformed label not including a =, you would get a index out of range panic. Added a catch for that.

I'll be a little more thorough and retest everything to make sure everything is as its supposed to. Will post the results when I'm done.

@tommysolsen

This comment has been minimized.

Show comment
Hide comment
@tommysolsen

tommysolsen Nov 7, 2017

Contributor

build

Built one function with no parameters

$ ./faas-cli build -f samples.yml
[0] > Building: url-ping.
Please provide a valid -lang or 'Dockerfile' for your function.
[0] < Builder done.

Built one function with lang parameter, but without handler or image

./faas-cli build -f samples.yml
[0] > Building: url-ping.
Clearing temporary build folder: ./build/url-ping/
Preparing / ./build/url-ping/function
2017/11/07 13:05:52 open : no such file or directory

built one with lang and handler parameters, not image

./faas-cli build -f samples.yml
[0] > Building: url-ping.
Clearing temporary build folder: ./build/url-ping/
Preparing ./sample/url-ping/ ./build/url-ping/function
Building:  with python template. Please wait..
invalid argument "" for t: invalid reference format
See 'docker build --help'.

I assume these two last ones should have failed with proper error messages,
but it seems that stack.ParseYAMLFile returns an instance for a service, even if both handler and image are missing.

deploy

$ ./faas-cli deploy
Please provide a --image to be deployed.

Here is tested on a file with some giberish in it.

Other functions that parse yaml(like list) giberish will produce same output

$ ./faas-cli deploy -f test.yml
Error with YAML file
2017/11/07 12:50:19 yaml: unmarshal errors:
  line 1: cannot unmarshal !!str `sefojrh...` into stack.Services

With both update and replace

$ ./faas-cli deploy --update --replace
Cannot specify --update and --replace at the same time.
  --replace    removes an existing deployment before re-creating it
  --update     provides a rolling update to a new function image or configuration
$ ./faas-cli deploy               
Please provide a --image to be deployed.
$ ./faas-cli deploy --image v2m    
Please provide a --name for your function as it will be deployed on FaaS

All label related error messages.
Including one bugfix that would panic the program

$ ./faas-cli deploy --image v2m --name k --label somethingiswr 
Error parsing labels: Label format is not correct, needs key=value

$ ./faas-cli deploy --image v2m --name k --label somethingiswr=
Error parsing labels: Empty label value: [somethingiswr=]

$ ./faas-cli deploy --image v2m --name k --label " =test"      
Error parsing labels: Empty label name: [ =test]

invoke

No name supplied

$ ./faas-cli invoke     
Please provide a name for the function

list

Gateway is unreachable, triggering error from proxy.GetFunctions

$ ./faas-cli list --gateway http://127.0.0.1:8080

Get http://127.0.0.1:8080/system/functions: dial tcp 127.0.0.1:8080: getsockopt: connection refused

login

$ ./faas-cli login 
Must provide --username or -u

$ ./faas-cli login --username test
Must provide a non-empty password via --password or --password-stdin

logout

Trying to logout without any valid logins registered(No config file exists)

$ ./faas-cli logout
config file not found

new

$ ./faas-cli new   
Please provide a name for the function

$ ./faas-cli new test  
You must supply a function language with the --lang flag

$ faas-cli new --list
Languages available as templates:
- node
- python
- python3
- ruby
- csharp
- Dockerfile
- go

Or alternatively create a folder containing a Dockerfile, then pick
the "Dockerfile" lang type in your YAML file.

$ echo $?
0

validTemplate() would return true, even for languages that did not have
templates. With that fixed

$ ./faas-cli new test --lang COBOL
COBOL is unavailable or not supported.

Trying to create a function that already exists

$ ./faas-cli new test --lang go   
Folder: test already exists

push

Changed samples to not have valid image keys.

Currently the go routines does not return anything

$ ./faas-cli push -f samples.yml
[0] > Pushing: url-ping.
Please provide a valid Image value in the YAML file.
[0] > Pushing: nodejs-echo.
Please provide a valid Image value in the YAML file.
[0] > Pushing: shrink-image.
Please provide a valid Image value in the YAML file.
[0] > Pushing: ruby-echo.
Please provide a valid Image value in the YAML file.
[0] < Pushing done.

$ echo $?
0

remove

$ ./faas-cli remove --gateway http://localhost:8080             
Please provide the name of a function to delete

version

Doesn't give any errors

Missing tests

  • invoke - Read from stdio. I don't know how to trigger this error.
  • bash completion
  • login - My stack doesnt run the login part yet so all messages that depend on server input responses I'll be unable to test until later this week.
  • new_function Im not sure how i can trigger the gitignore error or writing to file errors. Even when creating the folder/files without read/write permissions they get created just fine.
Contributor

tommysolsen commented Nov 7, 2017

build

Built one function with no parameters

$ ./faas-cli build -f samples.yml
[0] > Building: url-ping.
Please provide a valid -lang or 'Dockerfile' for your function.
[0] < Builder done.

Built one function with lang parameter, but without handler or image

./faas-cli build -f samples.yml
[0] > Building: url-ping.
Clearing temporary build folder: ./build/url-ping/
Preparing / ./build/url-ping/function
2017/11/07 13:05:52 open : no such file or directory

built one with lang and handler parameters, not image

./faas-cli build -f samples.yml
[0] > Building: url-ping.
Clearing temporary build folder: ./build/url-ping/
Preparing ./sample/url-ping/ ./build/url-ping/function
Building:  with python template. Please wait..
invalid argument "" for t: invalid reference format
See 'docker build --help'.

I assume these two last ones should have failed with proper error messages,
but it seems that stack.ParseYAMLFile returns an instance for a service, even if both handler and image are missing.

deploy

$ ./faas-cli deploy
Please provide a --image to be deployed.

Here is tested on a file with some giberish in it.

Other functions that parse yaml(like list) giberish will produce same output

$ ./faas-cli deploy -f test.yml
Error with YAML file
2017/11/07 12:50:19 yaml: unmarshal errors:
  line 1: cannot unmarshal !!str `sefojrh...` into stack.Services

With both update and replace

$ ./faas-cli deploy --update --replace
Cannot specify --update and --replace at the same time.
  --replace    removes an existing deployment before re-creating it
  --update     provides a rolling update to a new function image or configuration
$ ./faas-cli deploy               
Please provide a --image to be deployed.
$ ./faas-cli deploy --image v2m    
Please provide a --name for your function as it will be deployed on FaaS

All label related error messages.
Including one bugfix that would panic the program

$ ./faas-cli deploy --image v2m --name k --label somethingiswr 
Error parsing labels: Label format is not correct, needs key=value

$ ./faas-cli deploy --image v2m --name k --label somethingiswr=
Error parsing labels: Empty label value: [somethingiswr=]

$ ./faas-cli deploy --image v2m --name k --label " =test"      
Error parsing labels: Empty label name: [ =test]

invoke

No name supplied

$ ./faas-cli invoke     
Please provide a name for the function

list

Gateway is unreachable, triggering error from proxy.GetFunctions

$ ./faas-cli list --gateway http://127.0.0.1:8080

Get http://127.0.0.1:8080/system/functions: dial tcp 127.0.0.1:8080: getsockopt: connection refused

login

$ ./faas-cli login 
Must provide --username or -u

$ ./faas-cli login --username test
Must provide a non-empty password via --password or --password-stdin

logout

Trying to logout without any valid logins registered(No config file exists)

$ ./faas-cli logout
config file not found

new

$ ./faas-cli new   
Please provide a name for the function

$ ./faas-cli new test  
You must supply a function language with the --lang flag

$ faas-cli new --list
Languages available as templates:
- node
- python
- python3
- ruby
- csharp
- Dockerfile
- go

Or alternatively create a folder containing a Dockerfile, then pick
the "Dockerfile" lang type in your YAML file.

$ echo $?
0

validTemplate() would return true, even for languages that did not have
templates. With that fixed

$ ./faas-cli new test --lang COBOL
COBOL is unavailable or not supported.

Trying to create a function that already exists

$ ./faas-cli new test --lang go   
Folder: test already exists

push

Changed samples to not have valid image keys.

Currently the go routines does not return anything

$ ./faas-cli push -f samples.yml
[0] > Pushing: url-ping.
Please provide a valid Image value in the YAML file.
[0] > Pushing: nodejs-echo.
Please provide a valid Image value in the YAML file.
[0] > Pushing: shrink-image.
Please provide a valid Image value in the YAML file.
[0] > Pushing: ruby-echo.
Please provide a valid Image value in the YAML file.
[0] < Pushing done.

$ echo $?
0

remove

$ ./faas-cli remove --gateway http://localhost:8080             
Please provide the name of a function to delete

version

Doesn't give any errors

Missing tests

  • invoke - Read from stdio. I don't know how to trigger this error.
  • bash completion
  • login - My stack doesnt run the login part yet so all messages that depend on server input responses I'll be unable to test until later this week.
  • new_function Im not sure how i can trigger the gitignore error or writing to file errors. Even when creating the folder/files without read/write permissions they get created just fine.

@alexellis alexellis requested a review from ericstoekl Nov 7, 2017

@alexellis

LGTM

@ericstoekl

This comment has been minimized.

Show comment
Hide comment
@ericstoekl

ericstoekl Nov 7, 2017

Contributor

Compiled this PR and did some cursory checks, looks good so far. The only suggestion I have is that we could use some tests for this particular functionality (test error code result when a command fails, make sure it's 0 when a command succeeds). @alexellis , do you think tests are necessary or not for this PR?

Contributor

ericstoekl commented Nov 7, 2017

Compiled this PR and did some cursory checks, looks good so far. The only suggestion I have is that we could use some tests for this particular functionality (test error code result when a command fails, make sure it's 0 when a command succeeds). @alexellis , do you think tests are necessary or not for this PR?

Show outdated Hide outdated commands/bash_completion.go Outdated
Show outdated Hide outdated commands/bash_completion.go Outdated
Show outdated Hide outdated commands/build.go Outdated
@@ -227,6 +231,9 @@ func parseMap(envvars []string, keyName string) (map[string]string, error) {
result := make(map[string]string)
for _, envvar := range envvars {
s := strings.SplitN(strings.TrimSpace(envvar), "=", 2)
if len(s) != 2 {

This comment has been minimized.

@johnmccabe

johnmccabe Nov 7, 2017

Member

Good catch.

@johnmccabe

johnmccabe Nov 7, 2017

Member

Good catch.

Show outdated Hide outdated commands/list.go Outdated
Show outdated Hide outdated commands/new_function.go Outdated
Show outdated Hide outdated commands/faas.go Outdated
@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Nov 9, 2017

Member

@johnmccabe did you have any concerns about error handling, or is this good to go to merge?

Member

alexellis commented Nov 9, 2017

@johnmccabe did you have any concerns about error handling, or is this good to go to merge?

Show outdated Hide outdated commands/new_function.go Outdated
Show outdated Hide outdated commands/new_function.go Outdated
Show outdated Hide outdated commands/remove.go Outdated
Show outdated Hide outdated commands/faas.go Outdated
@johnmccabe

This comment has been minimized.

Show comment
Hide comment
@johnmccabe

johnmccabe Nov 9, 2017

Member

@Razesdark @alexellis just some minor comments then I'm 👍 to merge.

This is looking nice, thanks @Razesdark for the fast and responsive turnaround 🎈

Member

johnmccabe commented Nov 9, 2017

@Razesdark @alexellis just some minor comments then I'm 👍 to merge.

This is looking nice, thanks @Razesdark for the fast and responsive turnaround 🎈

Show outdated Hide outdated commands/faas.go Outdated

tommysolsen added some commits Nov 6, 2017

Change cobra Run to RunE and return errors
Signed-off-by: Tommy Stigen Olsen <tommysolsen@gmail.com>

Fix for a bug where the software would panic if the user tried to
supply a --label without a = sign in it

Fix for a bug where the `validTemplate(string) bool` function would return true for
any string not equal to "dockerfile"
Move error printing out of each subcommand
Instead of having fmt.Println(err) like statements scattered
around the each individual function, the error message returned
by each sub command will now be printed on screen.

Signed-off-by: Tommy Stigen Olsen <tommysolsen@gmail.com>
Error msg linting & new capitalisation func.
Uses @johnmccabe's solution for capitalisation

Signed-off-by: Tommy Stigen Olsen <tommysolsen@gmail.com>
Fix tests that relies on stdout instead of emsgs
Some of the tests tested if the respective functions
stdout for the correct error messages.

After making them silent and returning errors instead
these tests would fail, since we test them directly.

These tests now test the error messages instead of stdout

Signed-off-by: Tommy Stigen Olsen <tommysolsen@gmail.com>
@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Nov 13, 2017

Member

Hi, is this ready to merge? There were some comments from @johnmccabe 4 days ago. Have you reviewed these yet?

Member

alexellis commented Nov 13, 2017

Hi, is this ready to merge? There were some comments from @johnmccabe 4 days ago. Have you reviewed these yet?

@tommysolsen

This comment has been minimized.

Show comment
Hide comment
@tommysolsen

tommysolsen Nov 13, 2017

Contributor

As far as I have gathered, the only thing that seems left up for discussion is the capitaliseString function needed to show error messages the way we have been showing them prior to this PR.
The golang way seems to be something along the lines of Error: %s. Instead of just printing out the error message like we have been doing up until now.

Personally, I think the current style of error messages are just fine, since we are exiting the program anyways, prepending "Error" to the last error message or obvious instruction doesn't really do anything.

Contributor

tommysolsen commented Nov 13, 2017

As far as I have gathered, the only thing that seems left up for discussion is the capitaliseString function needed to show error messages the way we have been showing them prior to this PR.
The golang way seems to be something along the lines of Error: %s. Instead of just printing out the error message like we have been doing up until now.

Personally, I think the current style of error messages are just fine, since we are exiting the program anyways, prepending "Error" to the last error message or obvious instruction doesn't really do anything.

Show outdated Hide outdated commands/faas.go Outdated
@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Nov 13, 2017

Member

Thanks for the update.

Member

alexellis commented Nov 13, 2017

Thanks for the update.

@alexellis

LGTM

Use strings.ToUpper instead of capitaliseFirst
Also change proxy.Listfunction to return and not print
error messages to remove edge case where we returned
empty error message in list.go

Signed-off-by: Tommy Stigen Olsen <tommysolsen@gmail.com>

@alexellis alexellis merged commit 2932d48 into openfaas:master Nov 14, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Nov 14, 2017

Member

Merging. Thank you for this contribution @Razesdark I know it'll make a big difference for a lot of people. 🥇 💯 👍

Member

alexellis commented Nov 14, 2017

Merging. Thank you for this contribution @Razesdark I know it'll make a big difference for a lot of people. 🥇 💯 👍

@tommysolsen tommysolsen deleted the tommysolsen:run_errors branch Nov 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment