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: propagate failed scans with --all-projects #1301

Merged
merged 1 commit into from
Aug 4, 2020

Conversation

lili2311
Copy link
Contributor

@lili2311 lili2311 commented Jul 31, 2020

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

  • Collect failed dependency extractions with --all-projects and propagate it higher up.
  • For monitor these are now propagated all the way and will appear in output + json output. This may affect the existing error code as previously we silently skipped failure like these but not failures to get vulnerabilities during monitor or when a path test failed. So it was inconsistent.
  • For test more work / refactor needed to get these to the output & json so for now warning the user & providing instructions how to see the error with -d

Where should the reviewer start?

https://github.com/snyk/snyk/compare/fix/throw-erorr-if-all-tests-failed?expand=1#diff-e4ec2ebbae5004d31be7a5b680580a46R42

How should this be manually tested?

You can use cd snyk/test/acceptance/workspaces && snyk test --all-projects and cd snyk/test/acceptance/workspaces && snyk monitor --all-projects to see the output.

Any background context you want to provide?

Sometimes users see a blank screen when 100% of -all-projects scans fail and we do not flag about other failures as it was too noisy unless you run with -d

Screenshots

Screen Shot 2020-07-31 at 14 27 22

Screen Shot 2020-07-31 at 14 42 06

Screen Shot 2020-07-31 at 14 49 03

Screen Shot 2020-07-31 at 16 45 33

@lili2311 lili2311 added the 🚧 WIP Work In Progress label Jul 31, 2020
@lili2311 lili2311 self-assigned this Jul 31, 2020
@lili2311 lili2311 force-pushed the fix/throw-erorr-if-all-tests-failed branch from 87ef9d9 to c8598d3 Compare July 31, 2020 13:52
analytics.add('pluginName', inspectResult.plugin.name);

const postingMonitorSpinnerLabel =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved a little further down unchanged

@lili2311 lili2311 force-pushed the fix/throw-erorr-if-all-tests-failed branch 3 times, most recently from 93e7e73 to b2954d6 Compare July 31, 2020 15:09
allProjects: true,
});
let result;
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these were not previously propagated up if dependencies failed to be detected, but we do fail if one of the paths fails to be scanned so this aligns that behaviour.

This may however start failing some pipelines that previously were skipping the failures. How can this be communicated & is this the desired behaviour right? You want to know something you monitored failed and it did not pass.

@lili2311 lili2311 force-pushed the fix/throw-erorr-if-all-tests-failed branch 4 times, most recently from af49e17 to 7e9c7ef Compare July 31, 2020 16:32
@lili2311 lili2311 marked this pull request as ready for review July 31, 2020 16:59
@lili2311 lili2311 requested review from a team as code owners July 31, 2020 16:59
@ghost ghost requested review from gitphill and MegaBean July 31, 2020 16:59
@lili2311 lili2311 added ☠️ tech services and removed 🚧 WIP Work In Progress labels Jul 31, 2020
.gitignore Outdated Show resolved Hide resolved
@lili2311 lili2311 force-pushed the fix/throw-erorr-if-all-tests-failed branch from 7e9c7ef to 5cc6eee Compare August 3, 2020 08:13
- failed monitor paths generate an error but not failed
attempts to get dependencies, these were skipped and shown
only with -d output. Propagate full status back to the user.

- this may affect the expected exit code for some users where
previous failures were skipped in this manner

- log failed snyk test scans with --all-projects
@lili2311 lili2311 force-pushed the fix/throw-erorr-if-all-tests-failed branch from 5cc6eee to 7ef59ed Compare August 3, 2020 08:14
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2020

Expected release notes (by @lili2311)

fixes:
propagate failed monitor scans all the way to the user (7ef59ed)

others (will not be included in Semantic-Release notes):
ignore node diagnostic report (2a204e2)
disably flaky Windows tests (6d2d76a)
remove irrelevant comment (7d8dccc)

  • I hereby acknowledge these release notes are 🥙 AWESOME 🥙

Copy link
Contributor

@gitphill gitphill left a comment

Choose a reason for hiding this comment

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

looks good, tested locally

@lili2311 lili2311 merged commit 5ad17d8 into master Aug 4, 2020
@lili2311 lili2311 deleted the fix/throw-erorr-if-all-tests-failed branch August 4, 2020 09:01
@snyksec
Copy link

snyksec commented Aug 4, 2020

🎉 This PR is included in version 1.369.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants