Skip to content
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

Merged
merged 6 commits into from
Aug 25, 2021

Conversation

edoardopirovano
Copy link
Contributor

@edoardopirovano edoardopirovano commented Aug 18, 2021

This PR bundles together a few commits that improve how the Action handles what CLI version is being used. In particular:

  1. We can cache the result of 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.
  2. 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).
  3. The Action now checks that the CodeQL version is above a constant of 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.
  4. The PR checks now also test against the oldest supported version in addition to the version in the toolcache, the latest release, and the latest nightly build.

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.

Copy link
Contributor

@henrymercer henrymercer left a 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.

src/codeql.ts Outdated Show resolved Hide resolved
src/codeql.ts Show resolved Hide resolved
src/codeql.ts Show resolved Hide resolved
@starcke
Copy link

starcke commented Aug 25, 2021

However I'm unsure whether --sarif-category works without --sarif-group-rules-by-pack

I would be very surprised if these had an effect on each other, so I think this change is fine.

@edoardopirovano edoardopirovano merged commit a44b61d into github:main Aug 25, 2021
@edoardopirovano edoardopirovano deleted the cli-version branch August 25, 2021 14:52
@github-actions github-actions bot mentioned this pull request Aug 26, 2021
5 tasks
@github-actions github-actions bot mentioned this pull request Sep 6, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants