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

atlasexec: expose atlas exit code #22

Merged
merged 2 commits into from
Sep 28, 2023
Merged

atlasexec: expose atlas exit code #22

merged 2 commits into from
Sep 28, 2023

Conversation

dorav
Copy link
Contributor

@dorav dorav commented Sep 28, 2023

This is part of a multi stage change

  • Changing the runCommand, removing validJson from it. This makes things simpler for the next change
  • Changing the runCommand so it won't hide the exitCode and remove the special case for error code 1
  • Add the --context parameter to MigrateLint so it can be used from atlas-action

@dorav dorav requested a review from a team September 28, 2023 08:30
atlasexec/atlas.go Outdated Show resolved Hide resolved
atlasexec/atlas.go Outdated Show resolved Hide resolved
atlasexec/atlas.go Outdated Show resolved Hide resolved
require.NoError(t, err)
require.ErrorContains(t, err, "atlas command exited with 1")
Copy link
Member

Choose a reason for hiding this comment

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

This is suspicions. No error before -> now there is an error. Are you sure this is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there was a branch in the code that was hiding it (inside runCommand)

We don't want to expose it as an error in MigrateLint for backwards compatibility, but MigrateLintError is new and it's the only way we can tell that the lint failed when running in CI

@dorav dorav changed the title atlasexec/runCommand: expose atlas exit code atlasexec: expose atlas exit code Sep 28, 2023
atlasexec/atlas.go Outdated Show resolved Hide resolved
Comment on lines 323 to 327
ok := errors.As(err, &cliErr)
if ok && cliErr.hasLintErrors() {
r = strings.NewReader(cliErr.detail)
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.

@dorav dorav requested review from a8m and masseelch September 28, 2023 09:22
Comment on lines +332 to 339
if cliErr := (cliError{}); errors.As(err, &cliErr) && cliErr.summary == "" {
r = strings.NewReader(cliErr.detail)
}
if params.Writer != nil {
_, err = io.Copy(params.Writer, r)
if params.Writer != nil && r != nil {
if _, ioErr := io.Copy(params.Writer, r); ioErr != nil {
err = errors.Join(err, ioErr)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

cliError can have stdout and stderr. These summary/details are confusing.

Then, in the linting code, copy stdout to the writer, and return the error.

return jsonDecode[SummaryReport](r, err)
}

// 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))
if err != nil {
return err
if cliErr := (cliError{}); errors.As(err, &cliErr) && cliErr.summary == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if the () are needed.

@dorav
Copy link
Contributor Author

dorav commented Sep 28, 2023

@rotemtam I'm afraid that this change introduces backwards incompatible changes, let's discuss verbally please

@dorav dorav merged commit ef784ee into master Sep 28, 2023
2 checks passed
@dorav dorav deleted the dont_hide_exit_code branch September 28, 2023 12:26
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