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.

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

fix: change flag parse error handling to return errors instead of exiting #107

Merged
merged 2 commits into from
Jun 19, 2022

Conversation

carlpett
Copy link
Contributor

@carlpett carlpett commented Apr 23, 2022

Currently, if flag parsing fails, kubeconform exits with status 2 but no error message. This is because ExitOnError means flags.Parse actually calls os.Exit directly, rather than returning the error to the caller, which seems to have been what the rest of the program assumes. In combination with flags.SetOutput, the error message is written to a buffer which is never printed to the user.

This PR changes to ContinueOnError, which instead returns the error, leading to the buffer with the error message to be printed before exiting.

As a note, it also means the exit code will be 1 instead of 2 on flag parse errors, since that is what is currently done in realMain on errors from config.FromFlags. Not sure if this is something that would be considered a breaking change?

…ting

Having ExitOnError in combination with SetOutput to a buffer instead of
stdout/stderr means flags.Parse output is swallowed and kubeconform silently
exits directly with exit code 2 instead of returning the error.

Setting ContinueOnError instead returns the error, and writes usage help to
the buffer, so error handling code in main is reached.
@sklirg
Copy link

sklirg commented Jun 17, 2022

This also happens if a flag is input incorrectly, such as for a typo (-ignore-missing-schema vs. -ignore-missing-schema*s*). It's weird that the program exits with no information.

I actually dug to the same code and changed it before I realised a PR was already open, but here you can see the difference in behaviour with the same change I did:

$ git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
$ go build -o kubeconform cmd/kubeconform/main.go
$ ./kubeconform -x
$ echo $?
2

$ git checkout -
Switched to branch 'fix/dont-crash-without-error-on-non-existing-flag'
$ go build -o kubeconform cmd/kubeconform/main.go
$ ./kubeconform -x
flag provided but not defined: -x
Usage: ./kubeconform [OPTION]... [FILE OR FOLDER]...
  -cache string
        cache schemas downloaded via HTTP to this folder
  -cpu-prof string
        debug - log CPU profiling to file
  -exit-on-error
        immediately stop execution when the first error is encountered
  -h    show help information
  -ignore-filename-pattern value
        regular expression specifying paths to ignore (can be specified multiple times)
  -ignore-missing-schemas
        skip files with missing schemas instead of failing
  -insecure-skip-tls-verify
        disable verification of the server's SSL certificate. This will make your HTTPS connections insecure
  -kubernetes-version string
        version of Kubernetes to validate against, e.g.: 1.18.0 (default "master")
  -n int
        number of goroutines to run concurrently (default 4)
  -output string
        output format - json, junit, tap, text (default "text")
  -reject string
        comma-separated list of kinds to reject
  -schema-location value
        override schemas location search path (can be specified multiple times)
  -skip string
        comma-separated list of kinds to ignore
  -strict
        disallow additional properties not in schema
  -summary
        print a summary at the end (ignored for junit output)
  -v    show version information
  -verbose
        print results for all resources (ignored for tap and junit output)

$ echo $?
1

@yannh
Copy link
Owner

yannh commented Jun 19, 2022

Awesome thanks :) I had the same issue in another project of mine a few months ago and didnt port the fix here 🙈 yannh/redis-dump-go@d5bfe02

@yannh yannh merged commit 7bf1e01 into yannh:master Jun 19, 2022
@carlpett carlpett deleted the fix-flag-parse-error branch June 20, 2022 11:52
yannh added a commit that referenced this pull request Oct 16, 2022
…ting (#107)

* fix: change flag parse error handling to return errors instead of exiting

Having ExitOnError in combination with SetOutput to a buffer instead of
stdout/stderr means flags.Parse output is swallowed and kubeconform silently
exits directly with exit code 2 instead of returning the error.

Setting ContinueOnError instead returns the error, and writes usage help to
the buffer, so error handling code in main is reached.

* Add test for parsing incorrect flags

Co-authored-by: Yann Hamon <yann@mandragor.org>
yannh added a commit that referenced this pull request Oct 16, 2022
…ting (#107)

* fix: change flag parse error handling to return errors instead of exiting

Having ExitOnError in combination with SetOutput to a buffer instead of
stdout/stderr means flags.Parse output is swallowed and kubeconform silently
exits directly with exit code 2 instead of returning the error.

Setting ContinueOnError instead returns the error, and writes usage help to
the buffer, so error handling code in main is reached.

* Add test for parsing incorrect flags

Co-authored-by: Yann Hamon <yann@mandragor.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants