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

Feat/add snyk code as plugin for test #1664

Merged
merged 1 commit into from
Mar 4, 2021

Conversation

j-sp4
Copy link
Contributor

@j-sp4 j-sp4 commented Feb 26, 2021

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

What does this PR do?

This introduces an mpv usage for snyk code.

Where should the reviewer start?

you should have the snykcode cli's ff,
and run it with snyk code test or snyk code test <project_path>

How should this be manually tested?

snyk code test or snyk code test <project_path>

Any background context you want to provide?

we will be adding more functionality around this flow, more error handling, analytics, and output functionality, later on

What are the relevant tickets?

https://snyksec.atlassian.net/browse/COD-123

Screenshots

image

Additional questions

@github-actions
Copy link
Contributor

github-actions bot commented Feb 26, 2021

Warnings
⚠️

Looks like you added a new Tap test. Consider making it a Jest test instead. See files like test/*.spec.ts for examples. Files found:

  • test/fixtures/sast/sample-analyze-folders-response.json
  • test/fixtures/sast/sample-sarif.json
  • test/fixtures/sast/test-output-windows.txt
  • test/fixtures/sast/test-output.txt

Generated by 🚫 dangerJS against cba65a3

@j-sp4 j-sp4 force-pushed the feat/add-snyk-code-as-plugin-for-test branch from 097247c to 5f26a44 Compare February 26, 2021 17:50
@ArturSnyk ArturSnyk force-pushed the feat/add-snyk-code-as-plugin-for-test branch from 8deacf5 to a9fbf6e Compare March 1, 2021 07:33
@ArturSnyk ArturSnyk force-pushed the feat/add-snyk-code-as-plugin-for-test branch 9 times, most recently from eb558d5 to df57e31 Compare March 2, 2021 09:03
@ArturSnyk ArturSnyk self-assigned this Mar 2, 2021
@ArturSnyk ArturSnyk mentioned this pull request Mar 2, 2021
3 tasks
@ArturSnyk ArturSnyk force-pushed the feat/add-snyk-code-as-plugin-for-test branch 5 times, most recently from 50065dc to 3e42519 Compare March 2, 2021 12:57
@ArturSnyk ArturSnyk mentioned this pull request Mar 2, 2021
3 tasks
if (error instanceof Error) {
throw error;
} else {
throw new Error(error);
Copy link
Contributor

@ArturSnyk ArturSnyk Mar 2, 2021

Choose a reason for hiding this comment

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

we sometimes throw an Object which is an error(but not as an Error obj), so we would like to wrap it as an actual error

Copy link

Choose a reason for hiding this comment

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

Ideally we should only be throwing Error objects to avoid this inconsistency but that can be fixed outside this PR (ideally with an eslint rule). If error is a non-Error Object, passing it into the Error constructor is odd as that's a message string. But again, that seems to be existing behaviour so not blocking this PR.

@ArturSnyk ArturSnyk marked this pull request as ready for review March 2, 2021 13:25
@ArturSnyk ArturSnyk requested review from a team as code owners March 2, 2021 13:25
@ArturSnyk ArturSnyk requested a review from darscan March 3, 2021 11:24
Copy link
Contributor

@aviadhahami aviadhahami left a comment

Choose a reason for hiding this comment

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

lgtm

@darscan
Copy link
Contributor

darscan commented Mar 3, 2021

Please interactively rebase and structure your commit messages so that they make sense as Release Notes - the preview release notes do not make sense right now ( #1664 (comment) )

.github/CODEOWNERS Outdated Show resolved Hide resolved
src/lib/ecosystems/plugins.ts Show resolved Hide resolved
src/lib/errors/unsupported-feature-for-org-error.ts Outdated Show resolved Hide resolved
src/lib/plugins/code/analysis.ts Outdated Show resolved Hide resolved
src/lib/plugins/code/analysis.ts Outdated Show resolved Hide resolved
src/lib/plugins/code/format/output-format.ts Outdated Show resolved Hide resolved
src/lib/plugins/code/format/output-format.ts Outdated Show resolved Hide resolved
src/lib/plugins/code/index.ts Outdated Show resolved Hide resolved
src/lib/plugins/code/index.ts Outdated Show resolved Hide resolved
src/lib/plugins/code/index.ts Outdated Show resolved Hide resolved
@ArturSnyk
Copy link
Contributor

Please interactively rebase and structure your commit messages so that they make sense as Release Notes - the preview release notes do not make sense right now ( #1664 (comment) )

@darscan i will squash the commits before merging. I've split it so it would be easier to review :)

src/lib/plugins/sast/utils/testEmitter.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@orkamara orkamara left a comment

Choose a reason for hiding this comment

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

👏 Great job @ArturSnyk and @Spoor2709 !

@ArturSnyk ArturSnyk force-pushed the feat/add-snyk-code-as-plugin-for-test branch 5 times, most recently from 1455c59 to 4169a74 Compare March 4, 2021 11:19
* we are using code-clint lib to analyze a project and expecting
to get a sarif typed response.
* creating new formating schema for snyk code scanning
* adding `snyk code` functionality as an internal plugin
* adding `snyk code` behind FF
* adding support for the currect exit code (1) when there
are vulnerabilities.
* we currently have circular import issue. to temporary solve
it in our case, we will dynamicly import a module.
@ArturSnyk ArturSnyk force-pushed the feat/add-snyk-code-as-plugin-for-test branch from 4169a74 to cba65a3 Compare March 4, 2021 11:20
@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2021

Expected release notes (by @ArturSnyk)

features:
add snyk code support, as plugin, for test flow (cba65a3)

  • I hereby acknowledge these release notes are 🥙 AWESOME 🥙

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.

6 participants