-
Notifications
You must be signed in to change notification settings - Fork 11
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
atlasexec/migrate-lint: added --context and handle exit status code #20
Conversation
This change adds --context to MigrateLint. It's only useful in combination with Web=true, which exposed a new edge case where Atlas returns status code 1 when linting fails. Indication of linting failure can be observed using the summary when using MigrateLint, but when using Web=true, there is no summary, only a URL. So this PR also exposes the status code of Migrate Lint when runnning from MigrateLintError
@@ -384,7 +400,7 @@ func (c *Client) Version(ctx context.Context) (*Version, error) { | |||
} | |||
|
|||
// runCommand runs the given command and returns its output. | |||
func (c *Client) runCommand(ctx context.Context, args []string, vs ...validator) (io.Reader, error) { | |||
func (c *Client) runCommand(ctx context.Context, args []string, vs ...validator) (io.Reader, int, 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.
Error are values: https://go.dev/blog/errors-are-values
Define one and use it with errors.Is
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.
For example:
if stderr > 0, returns its content. Otherwise, return if exit-code <> 1, return errExitCode = errors.New("atlasexec: atlas command exited code 1")
. Then, catch that in the caller and handle it accordingly.
} | ||
|
||
// ErrLint is returned by MigrateLintError when linting errors are detected | ||
var ErrLint = errors.New("atlasexec: lint errors exist") |
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.
No need for this error. Compare the text instead.
Do you plan to consume this error somewhere? Maybe, to keep this error generic for other purposes, we can simply return exec.ExitError
// MigrateLintError runs the 'migrate lint' command, the output is written to params.Writer | ||
func (c *Client) MigrateLintError(ctx context.Context, params *MigrateLintParams) error { | ||
r, err := c.runCommand(ctx, lintArgs(params)) | ||
r, exitCode, err := c.runCommand(ctx, lintArgs(params)) |
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.
See my previous comment, and use errors.Is
here.
if err != nil { | ||
return err | ||
} | ||
if exitCode > 1 { |
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.
0?
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.
exit code of 0 is considered non-error, or am I missing something?
This change adds --context to MigrateLint.
It's only useful in combination with Web=true, which exposed a new edge case where Atlas returns status code 1 when linting fails.
Indication of linting failure can be observed using the summary when using MigrateLint, but when using Web=true, there is no summary, only a URL.
So this PR also exposes the status code of Migrate Lint when runnning from MigrateLintError