-
Notifications
You must be signed in to change notification settings - Fork 333
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
Use github merge-results
command for merging SARIF files
#2199
Conversation
a924694
to
66fcc6a
Compare
89a77d5
to
e20c273
Compare
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.
This looks generally great. Some initial comments:
// This is guarded by a `supportsFeature` check rather than by a version check. | ||
minimumVersion: undefined, |
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.
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.
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.
Are you suggesting that I add a toolsFeature
field to this object in this PR?
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'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.
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'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.
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'm not sure if it makes sense for this specific feature flag though since we usually do not have a
CodeQL
object available in theupload-sarif
action, so we wouldn't be able to check thetoolsFeature
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.
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.
Looks good to me — just a couple of final comments.
github?: { | ||
"*"?: Options; | ||
"merge-results"?: Options; | ||
}; |
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.
Not for you to fix, but I don't think this addition does anything.
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.
Great, thanks!
This will use the
github merge-results
command when thecli_sarif_merge_enabled
feature flag is enabled and all SARIF files were produced by CodeQL.Merge / deployment checklist