-
Notifications
You must be signed in to change notification settings - Fork 77
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 output-file option, default to random directory output in temp #346
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Keith Zantow <[email protected]>
dist/index.js
Outdated
"output", | ||
); | ||
} | ||
cmdArgs.push("--file", outputFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this interact with output
? I had the impression we were moving towards output
and way from file
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We support --output <format> --file <output-file>
in both Syft and Grype. This action doesn't allow multiple outputs, so it should work fine. I don't think there is a concrete plan to remove the --file
option, since it's a part of Syft 1.0 already. If you feel strongly this should be using the --output
flag, I can update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take this back, allowing for multiple outputs would be nice, so a user could get both a table printed to the logs and a SARIF report uploaded. I'll update this a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kzantow did you have additional updates to make here? I think you were going to update to use --output
or something?
discussed with @kzantow offline - there are still a few things that might be changed before the merge relating to multiple outputs. I'm moving this back to "in progress" and will re-review later. |
This PR makes a couple changes to the
scan-action
, one of which may be breaking for users:$TEMP
, instead of outputting directly in the workspaceoutput-file
parameter to allow users to specify an output file explicitlyFixes #216
Fixes #238