-
Notifications
You must be signed in to change notification settings - Fork 191
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
Remove support for CodeQL CLI versions older than v2.16.6 #3728
Conversation
* CLI version where there is support for multiple queries on the pack create command. | ||
*/ | ||
public static CLI_VERSION_WITH_MULTI_QUERY_PACK_CREATE = new SemVer("2.16.1"); | ||
public static OLDEST_SUPPORTED_CLI_VERSION = new SemVer("2.16.6"); |
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 a bit confused about the version number syntax. We use version numbers with v
in the supported_cli_versions.json
, but here we have historically used no v
. In the code I removed we used SemVer("2.16.1")
, so without the v
. Which one is correct?
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.
https://www.npmjs.com/package/semver says "A leading "=" or "v" character is stripped off and ignored." so either is fine. Personally I'd go with no "v" here.
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.
Thanks!
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.
To explain more...
From a behaviour point of view I don't think it makes any difference. From a code readability point of view it makes sense to remove unnecessary boilerplate. From a consistency point of view I suppose you might want them to match the json file, but I think any human who reads them would easily correlate the numbers and not get confused.
@@ -362,7 +354,7 @@ describe("Variant Analysis Manager", () => { | |||
qlPackRootPath, | |||
qlPackFilePath, | |||
expectedPackName: "codeql/java-queries", | |||
filesThatExist: [queryToRun, ...extraQueries], | |||
filesThatExist: [queryToRun, ...["Telemetry/ExtractorInformation.ql"]], |
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.
filesThatExist: [queryToRun, ...["Telemetry/ExtractorInformation.ql"]], | |
filesThatExist: [queryToRun], |
We actually don't want to check for this file anymore.
Recent versions of the CLI don't preserve queries with extensible predicates in MRVA packs,
The preservesExtensiblePredicatesInMrvaPack
method returns true when the CodeQL CLI version is older than 2.16.1
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.
🤦♀️ thank's for spotting that!
Remove support for CodeQL CLI versions older than v2.16.6