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

chore: Fix IaC test exit code when issues are found #3501

Merged
merged 1 commit into from
Jul 28, 2022

Conversation

ofekatr
Copy link
Contributor

@ofekatr ofekatr commented Jul 27, 2022

What does this PR do?

  • Fixes the exit code for the new IaC test flow to be 1 instead of 0, as is expected by the Snyk CLI (source).

Where should the reviewer start?

src/lib/iac/test/v2/output.ts

How should this be manually tested?

  • Run snyk iac test with a directory/file with issues, in all three formats: text, JSON, SARIF.
  • Ensure the exist code is 1.
  • Run snyk iac test --experimental with a directory/file with issues, in all three formats: text, JSON, SARIF.
  • Ensure the exist code is 1.
  • Run snyk iac test with a directory/file without issues, in all three formats: text, JSON, SARIF.
  • Ensure the exist code is 0.
  • Run snyk iac test --experimental with a directory/file without issues, in all three formats: text, JSON, SARIF.
  • Ensure the exist code is 0.

Any background context you want to provide?

  • At the moment when the new test flow returns issues, the exit code is 0, but based on the status codes list in the CLI’s help doc:
Exit codes
  Possible exit codes and their meaning:

  0: success, no vulnerabilities found
  1: action_needed, vulnerabilities found
  2: failure, try to re-run command
  3: failure, no supported projects detected

We should receive 1 when issues were found.
Running snyk iac test in all output formats confirms it, as it currently returns exist code 1.

Additional information

@ofekatr ofekatr requested a review from YairZ101 July 27, 2022 16:07
@ofekatr ofekatr force-pushed the fix/fix-iac-test-exit-code branch from e8cadfb to 8320f44 Compare July 27, 2022 16:08
@ofekatr ofekatr marked this pull request as ready for review July 27, 2022 16:08
@ofekatr ofekatr requested a review from a team as a code owner July 27, 2022 16:08
Copy link
Contributor

@YairZ101 YairZ101 left a comment

Choose a reason for hiding this comment

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

LGTM
the rest of the exit codes are working as expected?

@ofekatr ofekatr force-pushed the fix/fix-iac-test-exit-code branch from 8320f44 to 7712b57 Compare July 28, 2022 08:23
@ofekatr
Copy link
Contributor Author

ofekatr commented Jul 28, 2022

the rest of the exit codes are working as expected?

@YairZ101 The exit code for failures is 0, which is incorrect.
IIUC, we intend to have additional work on failures, after which we ensure the correct exit code, which is 2

@ofekatr ofekatr force-pushed the fix/fix-iac-test-exit-code branch 4 times, most recently from 3d74c5d to f9e2aee Compare July 28, 2022 08:33
@YairZ101
Copy link
Contributor

IIUC, we intend to have additional work on failures, after which we ensure the correct exit code, which is 2

What stops us from fixing the other 2 error codes now as well?

@ofekatr
Copy link
Contributor Author

ofekatr commented Jul 28, 2022

  • When there are no issues, we get 0 as expected.
  • With this PR, when there are issues, we would get 1 as expected.
  • When the CLI fails, we automatically get 2 as expected. However, in case all of the tested files failed, we should likely return 2 like in local-execution, but we should discuss with the team about how this should be first.
  • When only scanning unsupported files, we need to return 3, which we currently don't.

In general I would separate these to their own PRs to ensure we review these cautiously and decouple the changes.

@ofekatr ofekatr force-pushed the fix/fix-iac-test-exit-code branch from f9e2aee to 99b8ee0 Compare July 28, 2022 08:42
@YairZ101
Copy link
Contributor

In general I would separate these to their own PRs

okay, please make sure to open a ticket for this bug so we could track it.

@ofekatr
Copy link
Contributor Author

ofekatr commented Jul 28, 2022

In general I would separate these to their own PRs

cc @YairZ101

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.

2 participants