Add ability to run query with data extensions by aeisenberg · Pull Request #2065 · github/vscode-codeql

aeisenberg · 2023-02-10T23:38:18Z

Add a new config that toggles between using all data extensions or none.
If using all data extensions, then before a query evaluation, run a codeql resolve qlpacks command with the new --kind option. This will return a list of extension packs in the workspace.
Pass these packs to the cli before evaluation/
This will only work with CLI v2.12.3 (not yet released) or later.
Also include some CLI tests to ensure this works.

Note that the new integration tests will be skipped until 2.12.3 is released.

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
[Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

- Add a new config that toggles between using all data extensions or none. - If using all data extensions, then before a query evaluation, run a `codeql resolve qlpacks` command with the new `--kind` option. This will return a list of extension packs in the workspace. - Pass these packs to the cli before evaluation/ - This will only work with CLI v2.12.3 (not yet released) or later. - Also include some CLI tests to ensure this works.

robertbrignull

I have a couple of comments, but these changes make sense to me. I'm mostly reviewing this in isolation and I don't have much context yet of the whole data extensions work.

robertbrignull · 2023-02-13T16:18:06Z

+  describe("extension packs", () => {
+    skipIfTrue(qs.cliServer.cliConstraints.supportsQlpacksKind());


I'm unfortunately not sure this use of async code is safe.

This is calling beforeEach to add a skip, but what guarantees it does this before the its are called to add tests. I'm guessing that Jest waits for the full describe block to finish executing before it starts running any tests, but I'm not sure it has any way to know that this async code is running in the background. To the best of my knowledge, the block passed to describe must not be async.

Could you also link to docs about how beforeEach.skip(() => {}) works? I'm trying to understand it but the docs I can find don't include any examples like this.

Hmmm...I will have to rework this. Good catch.

robertbrignull · 2023-02-13T16:23:03Z

@@ -1142,19 +1142,26 @@ export class CodeQLCliServer implements Disposable {
   *   the default CLI search path is used.


Docs for this method are now outdated

Also, update jsdoc for `resolveQlpacks`.

aeisenberg · 2023-02-14T05:06:32Z

Hmmm...linting is no longer working in the editor, so I keep missing linting errors. Ah...It was a configuration problem. Working now.

robertbrignull

A couple more tiny points but then this looks good to me. Thanks for explaining bits. The use of the CLI now makes sense to me.

robertbrignull · 2023-02-14T09:28:31Z

@@ -1138,23 +1138,31 @@ export class CodeQLCliServer implements Disposable {
  /**
   * Gets information about available qlpacks
   * @param additionalPacks A list of directories to search for qlpacks before searching in `searchPath`.


Thanks for updating the doc comment. Sorry but seachPath is also mentioned in this line.

robertbrignull · 2023-02-14T09:34:29Z

+
+export function skipIfTrue(condition: () => Thenable<boolean>) {
+  return beforeEach(async () => {
+    if (await condition) {


Should be

Suggested change

if (await condition) {

if (await condition()) {

as otherwise this will always be true, assuming a function object is truthy.

Or convert condition back into a Thenable<boolean>. I don't think the extra layer of being function is necessary to make this work.

As an extra side point, what benefit does using Thenable give over using Promise? I'm curious to know which one we should use in general.

There's no reason I used Thenable<boolean> instead of Promise, other than I was just looking at the typings of beforeEach and copying that. I can change it to Promise.

I've fixed the implementation and it is working locally now in various forms.

Actually, I don't think the function is really doing much. I'm moving to vanilla beforeEach...pending.

…-all-data-extensions

Also, add a comment to the vscode settings file and describe how to use it.

aeisenberg · 2023-02-15T02:45:48Z

OK...all tests are passing, but the new cli integration test is not running since it relies on CLI version 2.12.3.

aeisenberg · 2023-02-28T04:05:47Z

v2.11.3 is now available. I am taking this PR out of draft. @robertbrignull, would you be able to take another look?

robertbrignull

One final comment. Sorry I think it was missed last time.

Other than that this look good to me. I've done a quick test too that everything still works as before when the config option is not set and this won't change behaviour for users that don't opt-in.

Co-authored-by: Robert <robertbrignull@github.com>

aeisenberg · 2023-02-28T16:29:45Z

Thanks for the review. Since we will be releasing the extension tomorrow, I will hold off on merging this PR until after the release.

…-all-data-extensions

aeisenberg · 2023-03-09T17:43:21Z

@robertbrignull now that the MRVA release is out, can you take another look? I'm hoping to merge this soon.

aeisenberg requested a review from a team as a code owner February 10, 2023 23:38

aeisenberg marked this pull request as draft February 10, 2023 23:38

aeisenberg force-pushed the aeisenberg/run-with-all-data-extensions branch from 194c28c to 6f43530 Compare February 10, 2023 23:51

robertbrignull reviewed Feb 13, 2023

View reviewed changes

Fix skipIfTrue

d8435d1

Also, update jsdoc for `resolveQlpacks`.

Make linter happy

39e6b27

aeisenberg force-pushed the aeisenberg/run-with-all-data-extensions branch from 072646f to 39e6b27 Compare February 14, 2023 05:11

robertbrignull reviewed Feb 14, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into aeisenberg/run-with…

c0c30b4

…-all-data-extensions

github-advanced-security AI found potential problems Feb 14, 2023

View reviewed changes

Comment thread extensions/ql-vscode/test/vscode-tests/cli-integration/queries.test.ts Fixed

aeisenberg force-pushed the aeisenberg/run-with-all-data-extensions branch from 208bd1e to a35c92f Compare February 14, 2023 21:36

Remove skipIfTrue

256b806

Also, add a comment to the vscode settings file and describe how to use it.

aeisenberg force-pushed the aeisenberg/run-with-all-data-extensions branch from a35c92f to 256b806 Compare February 14, 2023 22:47

aeisenberg marked this pull request as ready for review February 28, 2023 04:04

aeisenberg requested a review from robertbrignull February 28, 2023 04:05

aeisenberg enabled auto-merge February 28, 2023 04:19

robertbrignull reviewed Feb 28, 2023

View reviewed changes

Comment thread extensions/ql-vscode/src/cli.ts Outdated

Remove mention of search path in coment

5843c40

Co-authored-by: Robert <robertbrignull@github.com>

aeisenberg added 2 commits March 6, 2023 15:10

Merge remote-tracking branch 'upstream/main' into aeisenberg/run-with…

e07208b

…-all-data-extensions

Fix test comments

0ecde78

aeisenberg force-pushed the aeisenberg/run-with-all-data-extensions branch from 2d577de to 0ecde78 Compare March 7, 2023 02:07

aeisenberg disabled auto-merge March 9, 2023 17:41

Merge branch 'main' into aeisenberg/run-with-all-data-extensions

790a152

aeisenberg requested a review from robertbrignull March 9, 2023 17:42

robertbrignull approved these changes Mar 10, 2023

View reviewed changes

aeisenberg merged commit 0d8df9a into main Mar 10, 2023

aeisenberg deleted the aeisenberg/run-with-all-data-extensions branch March 10, 2023 16:09

		describe("extension packs", () => {
		skipIfTrue(qs.cliServer.cliConstraints.supportsQlpacksKind());

		@@ -1142,19 +1142,26 @@ export class CodeQLCliServer implements Disposable {
		* the default CLI search path is used.

Conversation

aeisenberg commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

robertbrignull left a comment

Choose a reason for hiding this comment

Uh oh!

robertbrignull Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

aeisenberg Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

robertbrignull Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aeisenberg commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertbrignull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbrignull Feb 14, 2023

Choose a reason for hiding this comment

Uh oh!

robertbrignull Feb 14, 2023

Choose a reason for hiding this comment

Uh oh!

aeisenberg Feb 14, 2023

Choose a reason for hiding this comment

Uh oh!

aeisenberg Feb 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aeisenberg commented Feb 15, 2023

Uh oh!

aeisenberg commented Feb 28, 2023

Uh oh!

robertbrignull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aeisenberg commented Feb 28, 2023

Uh oh!

aeisenberg commented Mar 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aeisenberg commented Feb 10, 2023 •

edited

Loading

aeisenberg commented Feb 14, 2023 •

edited

Loading

robertbrignull left a comment •

edited

Loading