Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable non-zero-exit in case of an error while running "build" #195

Merged
merged 3 commits into from
Oct 31, 2017
Merged

Enable non-zero-exit in case of an error while running "build" #195

merged 3 commits into from
Oct 31, 2017

Conversation

nenadilic84
Copy link
Contributor

@nenadilic84 nenadilic84 commented Oct 29, 2017 •
edited
Loading

Description

Motivation and Context

How Has This Been Tested?

$ ./faas-cli build
Error: please provide a valid -image name for your Docker image
# checking the exit code
$ echo $?
1

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.

@nenadilic84
Enable non-zero exit codes for error conditions
0d0cc35 
Signed-off-by: Nenad Ilic <nenadilic84@gmail.com>
alexellis
alexellis reviewed Oct 29, 2017
View reviewed changes
Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see the errors triggered in the comments here. Also does this work if the Docker build fails.

@alexellis alexellis changed the title Enable non-zero-exit in case of an error while running a command Enable non-zero-exit in case of an error while running "build" Oct 29, 2017
@nenadilic84
Copy link
Contributor Author

@alexellis, not sure if I know what you mean by

I'd like to see the errors triggered in the comments here.

@alexellis
Copy link
Member

You tested one of the error conditions. I'd like you to run each error scenario and show that it triggers the exit code we want. @itscaro may also be able to help show to extend the tests to pick up the exit code automatically.

alexellis
alexellis reviewed Oct 30, 2017
View reviewed changes
commands/build.go Outdated
@@ -80,27 +79,26 @@ func runBuild(cmd *cobra.Command, args []string) {
}

if pullErr := PullTemplates(""); pullErr != nil {
log.Fatalln("Could not pull templates for OpenFaaS.", pullErr)
log.Fatalf("Could not pull templates for OpenFaaS.")
return pullErr
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log.Fatalf will probably cause an exit here. Let's use a combined error message with fmt.Errorf of pullErr and our custom message and return it as the error.

alexellis
alexellis reviewed Oct 30, 2017
View reviewed changes
commands/faas.go
@@ -44,7 +45,9 @@ func init() {
func Execute(customArgs []string) {
faasCmd.SilenceUsage = true
faasCmd.SetArgs(customArgs[1:])
faasCmd.Execute()
if err := faasCmd.Execute(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

alexellis
alexellis reviewed Oct 30, 2017
View reviewed changes
Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a great start, we just need a few tweaks and I'll merge. When do you think you can have it done by?

@alexellis
Copy link
Member

Pinging @itscaro to suggest how to test the exit code from the CLI integration tests.

@nenadilic84
Copy link
Contributor Author

Will push the changes probably till the end of the day...

@nenadilic84
Forward error for a build command
05875f8 
Signed-off-by: Nenad Ilic <nenadilic84@gmail.com>
@nenadilic84
Added build_test
b1ccf9a 
Signed-off-by: Nenad Ilic <nenadilic84@gmail.com>
@nenadilic84
Copy link
Contributor Author

@alexellis, update the PR and added some tests... Let me know if you have something else in mind

@alexellis
Copy link
Member

I think we can merge this now. Not 100% on the tests. I think it's hard to test but we only have the negative cases not a positive one. Ideally want both.

@nenadilic84
Copy link
Contributor Author

@alexellis, not sure what you mean? Should I add more tests for the build command?

@alexellis
Copy link
Member

There is only a test that shows the negative case (exit code non-zero) - we need a test to show positive case of (exit code zero).

@alexellis alexellis merged commit 8b12765 into openfaas:master Oct 31, 2017
@alexellis
Copy link
Member

Merging work so far, thank you 👍

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

@alexellis alexellis alexellis left review comments

Assignees
No one assigned
Labels
hacktoberfest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nenadilic84 @alexellis