-
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
Improve handling of CLI versions #707
Conversation
1b88c79
to
e70ec1d
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.
A few flags that are only available in recent CodeQL CLIs are moved to be behind a version check, so we can be compatible with older versions. @henrymercer I'd be very keen to have your eyes on this commit to let me know if you foresee any issues with not passing those flags (we don't need full functionality, but we shouldn't break either).
For --print-metrics-summary
and --print-diagnostics-summary
, these summaries are only printed when there are results from metrics or diagnostics queries respectively, so we already need to handle the situation where they're not printed.
For --sarif-group-rules-by-pack
, Code Scanning should be able to handle receiving both SARIF that does and SARIF that doesn't group rules by the query pack they're from. However I'm unsure whether --sarif-category
works without --sarif-group-rules-by-pack
— ccing @cannist to confirm since I think you worked on the implementation of this.
I would be very surprised if these had an effect on each other, so I think this change is fine. |
This PR bundles together a few commits that improve how the Action handles what CLI version is being used. In particular:
codeql version
to avoid spawning the process many times. This will be important since the later changes will result in us using the version much more frequently.CODEQL_MINIMUM_VERSION
that records the oldest version we support. This is currently set to 2.3.1 when some breaking changes to how the tracing environment is configured occurred.I think the integration tests workflow is in need of some refactoring now that I have added this additional complexity to it, but I propose delaying this to a later PR.