Allow input parameter output-format and output report to be used
#187
Conversation
|
@maartenh could you rebase this against main? |
|
@kzantow rebase done, as requested. |
|
Hi @maartenh sorry to ask again, but could you rebase against main once more? I added a check that validates the built action is correct so we don't have a situation like this where the built action gets committed with changes not in the source. |
|
I've had a better look at this change and I'm wondering if there is a bit of a different approach we might want to take here. There is another related PR: #135 to get table output. I think maybe both of these could be combined with a One question I have is: are there situations where anyone wants more than one format (e.g. a table printed and a SARIF file generated)? |
| @@ -124,6 +132,7 @@ async function runScan({ source, failBuild, acsReportEnable, severityCutoff }) { | |||
| } | |||
|
|
|||
| const SEVERITY_LIST = ["negligible", "low", "medium", "high", "critical"]; | |||
| const FORMAT_LIST = ["sarif", "json"]; | |||
There was a problem hiding this comment.
I think we should probably check that if outputFormat !== "sarif" && acsReportEnable, it should be an error.
There was a problem hiding this comment.
Sounds good. Basically the acs-report-enable flag would be an alias for setting the output format to sarif, and setting both an error unless they match.
And I actually see no reason not to support the other output formats available on the Grype command line, excepting template which gets a lot more complicated (and a potential code injection point depending on what Go templates can do, I'm not familiar with them).
There was a problem hiding this comment.
Agree -- the other PR I referenced is basically asking for table output, so we'll make some changes there for aligning this stuff.
|
Ahh -- sorry for the spam. I realize now that this is a follow-on to a partially merged change, which is already in the docs. Sorry I didn't have all the context loaded before. I've added a comment about one thing I think we should add, and with another rebase we can probably get this merged 👍 |
|
@maartenh if you'd like I could make these changes -- just let me know; thanks! |
|
I agree that it's a bit messy to have 2 parameters controlling the output, but it does allow the action to retain compatibility with existing usages, so no major version bump is needed yet. For supporting multiple output formats, that would either require updates to the underlying Grype tool to support that, or require running it multiple times. And at that point you might want another flag to prevent updating the vulnerability db for the second run, just to keep the different reports consistent. It seems to get slightly complex, and except for the consistency thing, can simply be covered by users running the action multiple times. |
Signed-off-by: Maarten Hazewinkel <maarten.hazewinkel@gmail.com>
Signed-off-by: Maarten Hazewinkel <maarten.hazewinkel@gmail.com>
In PR anchore#184 the ./dist/index.js was updated with new functionality, but the source file ./index.js was not updated. So the next build run would have overwritten the new output-format functionality. Migrated the above changes into ./index.js and updated relevant test cases to make them run properly again. Signed-off-by: Maarten Hazewinkel <maarten.hazewinkel@gmail.com>
Check for conflicts between the acs-report-enable parameter and the new output-format parameters. Signed-off-by: Maarten Hazewinkel <maarten.hazewinkel@gmail.com>
|
(Didn't expect resetting my source branch to auto-close the PR, reopening now) @kzantow Rebased the PR and added the parameter check as suggested. I'll add the other output formats I suggested above in a new PR after this gets merged. |
This fixes issue #186.
The PR #184 missed out on updating the
action.ymlto describe the new input parameter and output field for the action.Secondly, in that PR only the
./dist/index.jsfile was updated with the new feature, but the source file./index.jswas left untouched. Any subsequent run and commit ofnpm run buildwould have erased the changes.I've moved the new code into
./index.jsand regenerated./dist/index.js. That also looks to have picked up a few changes due to library updates that had not been integrated into thedistoutput yet.Finally I've updated a few tests to make them work properly with the new feature.