FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Improve handling of CLI versions by edoardopirovano · Pull Request #707 · github/codeql-action · GitHub

Improve handling of CLI versions - #707

Merged
edoardopirovano merged 6 commits into
github:mainfrom
edoardopirovano:cli-version
Aug 25, 2021
Merged

edoardopirovano merged 6 commits into
github:mainfrom
edoardopirovano:cli-version

Conversation

edoardopirovano commented Aug 18, 2021
edited
Loading

Copy link
Copy Markdown
Contributor

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.

edoardopirovano requested a review from a team as a code owner August 18, 2021 10:38

henrymercer left a comment
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread src/codeql.ts Outdated
Comment thread src/codeql.ts
Comment thread src/codeql.ts

starcke commented Aug 25, 2021

Copy link
Copy Markdown

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 merged commit a44b61d into github:main Aug 25, 2021
edoardopirovano deleted the cli-version branch August 25, 2021 14:52
github-actions Bot mentioned this pull request Aug 26, 2021
5 tasks
github-actions Bot mentioned this pull request Sep 6, 2021
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

4 participants


Back | FazBrowse Home | New Git URL