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 --json-file-output option for snyk test #1107

Merged
merged 1 commit into from
May 28, 2020

Conversation

maxjeffos
Copy link
Contributor

@maxjeffos maxjeffos commented May 7, 2020

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

What does this PR do?

This allows a user to directly save JSON format output to a file as specified by the --json-file-output= parameter, regardless of whether they are using the human-readable or json output. The primary use-case of this is to enable users (and Snyk CI plugins) to simultaneously get the human readable output while saving a JSON format output to a file for other purposes (for example, generating a report).

Where should the reviewer start?

Please take a look at the code and advise as to testing best-practices. I will then include appropriate tests for this PR.

How should this be manually tested?

Say you want to save the JSON output to a file called "snyk-output.json" in the current directory. You could then run snyk test --json-file-output=snyk-output.json which should do a normal snyk test and generate the human-readable output to stdout but at the same time save the JSON format output to snyk-output.json.

Any background context you want to provide?

We need this for #1027 and snyk/snyk-azure-pipelines-task#30 as well as for another CI plugin we're currently working on.
This PR replaces a #1059 which I will close once this gets traction

What are the relevant tickets?

Additional questions

@maxjeffos maxjeffos requested review from gitphill and orsagie May 7, 2020 03:26
@maxjeffos maxjeffos requested a review from a team as a code owner May 7, 2020 03:26
@maxjeffos maxjeffos force-pushed the feat/add-json-file-output-option branch 2 times, most recently from 449d5cc to aea2b8c Compare May 7, 2020 03:53
@maxjeffos maxjeffos force-pushed the feat/add-json-file-output-option branch 7 times, most recently from 8474f14 to f5f4829 Compare May 12, 2020 03:00
@maxjeffos maxjeffos requested review from JackuB and pavel-snyk May 12, 2020 03:01
@maxjeffos maxjeffos force-pushed the feat/add-json-file-output-option branch from f5f4829 to d50e296 Compare May 12, 2020 03:14
@aarlaud
Copy link
Contributor

aarlaud commented May 12, 2020

@maxjeffos can you also make sure the output of --print-deps is in that file too if the option is specified. I'm using the depgraph for something and that'd be great to have in this function. Thanks!

src/cli/index.ts Outdated
try {
writeFileSync(jsonOutputFile, stringifiedJson);
} catch (err) {
console.error(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

potentially 'ugly' for user

for example: snyk test --json-file-output (no path provided) produces:

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string or an instance of Buffer or URL. Received type boolean (true)
    at Object.openSync (fs.js:450:10)
    at Object.writeFileSync (fs.js:1279:35)
    at saveJsonResultsToFile (/Users/phill/code/snyk/src/cli/index.ts:156:5)
    at handleError (/Users/phill/code/snyk/src/cli/index.ts:111:5)
    at main (/Users/phill/code/snyk/src/cli/index.ts:273:28)
    at processTicksAndRejections (internal/process/task_queues.js:97:5) {
  code: 'ERR_INVALID_ARG_TYPE'
}

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe some validation of argument passed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is resolved now and I added some tests for it. Thanks.

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.

can we update help text please?

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.

should we support a path to json?

for example: snyk test ---json-file-output=path/to/ouput.json at the moment this causes ENOENT error

If we don't want to support this, we should validate input and display friendly message explaining arg should be filename only (no path).

@maxjeffos maxjeffos force-pushed the feat/add-json-file-output-option branch from d50e296 to 3d7afdc Compare May 14, 2020 12:33
package.json Outdated Show resolved Hide resolved
@maxjeffos maxjeffos force-pushed the feat/add-json-file-output-option branch from 3d7afdc to 8a84ae2 Compare May 14, 2020 12:43
src/cli/index.ts Outdated
}

if (jsonOutputFile.constructor.name !== String.name) {
console.error("jsonOutputFile is invalid type");
Copy link
Contributor

Choose a reason for hiding this comment

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

"--json-output-file should be a filename path"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@maxjeffos maxjeffos force-pushed the feat/add-json-file-output-option branch 6 times, most recently from 51bce38 to c5c52ad Compare May 18, 2020 13:34
@maxjeffos maxjeffos deleted the feat/add-json-file-output-option branch May 28, 2020 14:25
@snyksec
Copy link

snyksec commented May 28, 2020

🎉 This PR is included in version 1.332.0 🎉

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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants