-
Notifications
You must be signed in to change notification settings - Fork 1.6k
cli: avoid os.Exit() in Run() funcs #7788
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| // Copyright 2025 The OPA Authors. All rights reserved. | ||
| // Use of this source code is governed by an Apache2 | ||
| // license that can be found in the LICENSE file. | ||
|
|
||
| package cmd | ||
|
|
||
| import ( | ||
| "fmt" | ||
| ) | ||
|
|
||
| type ExitError struct { | ||
| Exit int | ||
| wrapped error | ||
| } | ||
|
|
||
| func newExitError(exit int) error { | ||
| return &ExitError{Exit: exit} | ||
| } | ||
|
|
||
| func newExitErrorWrap(exit int, err error) error { | ||
| return &ExitError{Exit: exit, wrapped: err} | ||
| } | ||
|
|
||
| func (c *ExitError) Error() string { | ||
| return fmt.Sprintf("exit %d", c.Exit) | ||
| } | ||
|
|
||
| func (c *ExitError) Unwrap() error { | ||
| return c.wrapped | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,8 +28,10 @@ func init() { | |
| PreRunE: func(cmd *cobra.Command, _ []string) error { | ||
| return env.CmdFlags.CheckEnvironmentVariables(cmd) | ||
| }, | ||
| Run: func(_ *cobra.Command, _ []string) { | ||
| generateCmdOutput(os.Stdout, check) | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| cmd.SilenceErrors = true | ||
| cmd.SilenceUsage = true | ||
| return generateCmdOutput(os.Stdout, check) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we shouldn't wrap the error?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really, it's not evaluating user-controlled rego code... |
||
| }, | ||
| } | ||
|
|
||
|
|
@@ -39,7 +41,7 @@ func init() { | |
| RootCommand.AddCommand(versionCommand) | ||
| } | ||
|
|
||
| func generateCmdOutput(out io.Writer, check bool) { | ||
| func generateCmdOutput(out io.Writer, check bool) error { | ||
| fmt.Fprintln(out, "Version: "+version2.Version) | ||
| fmt.Fprintln(out, "Build Commit: "+version2.Vcs) | ||
| fmt.Fprintln(out, "Build Timestamp: "+version2.Timestamp) | ||
|
|
@@ -62,9 +64,10 @@ func generateCmdOutput(out io.Writer, check bool) { | |
| err := checkOPAUpdate(out) | ||
| if err != nil { | ||
| fmt.Fprintf(out, "Error: %v\n", err) | ||
| os.Exit(1) | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func checkOPAUpdate(out io.Writer) error { | ||
|
|
||
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.
There is still an
os.Exit(1)ininit(). Dropping it wouldn't impact what you're doing here, from what I understand, but it raises the question: do we need that? If we fail to deprecate a flag, maybe we don't really care and should prioritize a non-bricked build(?).