Use github merge-results command for merging SARIF files - #2199
koesie10 merged 14 commits into
Conversation
Sorry, something went wrong.
henrymercer
left a comment
There was a problem hiding this comment.
This looks generally great. Some initial comments:
Sorry, something went wrong.
| // This is guarded by a `supportsFeature` check rather than by a version check. | ||
| minimumVersion: undefined, |
There was a problem hiding this comment.
Going forwards, this will probably be the common case as we're preferring checking CLI features to checking version numbers. We should consider adding a toolVersion field to this record so callees can just check the feature.
Sorry, something went wrong.
There was a problem hiding this comment.
Are you suggesting that I add a toolsFeature field to this object in this PR?
Sorry, something went wrong.
There was a problem hiding this comment.
I'm happy if you'd rather do this in a separate PR, but I think if we rely on checking manually we'll probably make a mistake soon enough.
Sorry, something went wrong.
There was a problem hiding this comment.
I'm not sure if it makes sense for this specific feature flag though since we usually do not have a CodeQL object available in the upload-sarif action, so we wouldn't be able to check the toolsFeature anyway. We probably also don't want to let the future deprecation behavior depend on which version of CodeQL is being used since the deprecation will happen in the Code Scanning API rather than in CodeQL.
Sorry, something went wrong.
There was a problem hiding this comment.
I'm not sure if it makes sense for this specific feature flag though since we usually do not have a CodeQL object available in the upload-sarif action, so we wouldn't be able to check the toolsFeature anyway.
That makes sense — if the feature flag is disabled via the API, there's no point downloading CodeQL.
We probably also don't want to let the future deprecation behavior depend on which version of CodeQL is being used since the deprecation will happen in the Code Scanning API rather than in CodeQL.
I think we'd probably want to time the deprecation such that the CLIs that can't merge SARIF runs are no longer supported by the time we deprecate the API.
Sorry, something went wrong.
henrymercer
left a comment
There was a problem hiding this comment.
Looks good to me — just a couple of final comments.
Sorry, something went wrong.
| github?: { | ||
| "*"?: Options; | ||
| "merge-results"?: Options; | ||
| }; |
There was a problem hiding this comment.
Not for you to fix, but I don't think this addition does anything.
Sorry, something went wrong.
henrymercer
left a comment
There was a problem hiding this comment.
Great, thanks!
Sorry, something went wrong.
This will use the github merge-results command when the cli_sarif_merge_enabled feature flag is enabled and all SARIF files were produced by CodeQL.
Merge / deployment checklist