Use `--codescanning-config` flag of CLI #830

edoardopirovano · 2021-11-23T14:46:05Z

The CLI now supports a --codescanning-config flag to pass in the user-supplied input and use this to configure the run. This leads to a bunch of duplication between the Action (which does its own handling of these files) and the CLI. We can't remove the Action's own handling of these files yet as we still have to support old versions of the CLI, but for the time being let's start having it use the new flag when it can and then we can remove the duplicate code at a later date.

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Confirm the readme has been updated if necessary.
Confirm the changelog has been updated if necessary.

edoardopirovano · 2021-11-23T15:22:01Z

Looks like a small CLI fix is needed to get this working. Assuming that gets in the next release, we can enable this for CodeQL 2.7.3+.

aeisenberg

So, to be clear, whenever we run tests with a CLI >= 2.7.2, we will automatically be running with the --codescanning-config. Are there any other, new integration tests we should be writing here?

edoardopirovano · 2021-11-23T17:50:50Z

So, to be clear, whenever we run tests with a CLI >= 2.7.2, we will automatically be running with the --codescanning-config. Are there any other, new integration tests we should be writing here?

That's right, yes. And indeed this already highlighted the issue I fixed in the internal PR (e.g. this run). I think our coverage of different config files is already pretty good, but I'm open to adding more if you think we need to.

There is a slight annoyance that the packaging tests run with old CLIs (deliberately, since these are known to be compatible with the test packs), so those won't hit the new code path and we're relying on the integration tests in semmle-code to catch issues with specifying packages in the config files. The solution there would be to re-release the test packs with the next CLI version, then we can run those tests with that CLI so that we hit the new code path there too.

aeisenberg · 2021-11-23T18:30:02Z

There is a slight annoyance that the packaging tests run with old CLIs (deliberately, since these are known to be compatible with the test packs), so those won't hit the new code path and we're relying on the integration tests in semmle-code to catch issues with specifying packages in the config files. The solution there would be to re-release the test packs with the next CLI version, then we can run those tests with that CLI so that we hit the new code path there too.

Starting in December, we will release new versions of the packs with every CLI release. Either, we can add a GitHub action that periodically releases updated versions of the test packs. OR (and this is preferable IMO) we can rely on database downgrade scripts that we are planning to start including with all of our extractors. We would need a one time upgrade of the test packs to ensure they are compatible with the first dbscheme that includes a downgrade, and then things should work forever (and if they stop working, then it's a bug).

edoardopirovano · 2021-11-23T18:35:45Z

Right, all of that makes sense and I agree downgrade scripts are the way to go. Until the Glorious Future you describe above, though, we do perhaps want to do a release of the test packs for the purposes of more thoroughly testing this PR.

Let's leave that for in a couple of weeks when we actually have the CLI release with the fix and this is ready to ship.

edoardopirovano · 2021-12-09T15:34:37Z

Bumped the version flag in this to the now-released 2.7.3 which includes the necessary fix for this PR. This is possibly ready to merge as-is, unless we want to re-release the test packs so that the packaging-related tests can run with 2.7.3 to improve our coverage of this.

edoardopirovano · 2022-01-04T14:16:48Z

Rebased once again. I think it would be good to get this in - while it may seem low priority, getting merged will mean that we can start deprecating the codeql-action doing its own reading of Code Scanning files. This could save us some work down the line - if we finish that deprecation before we next need to change the format of those files, we'll only need to update the CLI code instead of both the CLI and Action!

aeisenberg · 2022-01-07T17:06:03Z

src/codeql.ts

+      const extraArgs = config.languages.map(
+        (language) => `--language=${language}`
+      );
+      if (config.languages.filter(isTracedLanguage).length > 0) {
        extraArgs.push("--begin-tracing");
        if (processName !== undefined) {
          extraArgs.push(`--trace-process-name=${processName}`);
        } else {
          extraArgs.push(`--trace-process-level=${processLevel || 3}`);


I know this is unchanged, but a comment as to why 3 would be nice.

Seems sensible, I've added a comment explaining this default.

aeisenberg · 2022-01-07T17:11:55Z

src/codeql.ts

+      if (await util.codeQlVersionAbove(codeql, CODEQL_VERSION_CONFIG_FILES)) {
+        const configLocation = path.resolve(config.tempDir, "user-config.yaml");
+        fs.writeFileSync(configLocation, yaml.dump(config.originalUserInput));
+        extraArgs.push(`--codescanning-config=${configLocation}`);
+      }


Do we also need to create a user config for (non-clustered) database init?

We do not. Note that we use clustered databases for all versions of the CLI versions from 2.7.0 whereas we are using the CLI's parsing of the config files for CLI versions from 2.7.3. So, if we added that code path it would be perfectly valid but dead code because we would never actually be in a situation where we have a non-clustered DB and we want to let the CLI parse the config file.

Maybe we should mark this path as dead code for CLI < 2.7.0. So that when we stop supporting those versions, we k now we can safely delete the code.

I like the idea, but I'm not quite sure how you're suggesting marking it. As we stop supporting CLI versions I expect we'll look for usages of the CODEQL_VERSION_* that becomes no longer relevant and then delete true true or false path as appropriate. I don't think any additional marking is needed to achieve this beyond the fact that we've used this variable and we know what branch of the if will eventually become the one we always go down.

edoardopirovano · 2022-01-07T17:36:02Z

Thanks for the review @aeisenberg! Have rebased again and addressed your comment and question.

aeisenberg · 2022-02-02T17:11:07Z

@edoardopirovano Sorry...this dropped off my notifications page. Is this still a relevant change?

edoardopirovano · 2022-02-02T18:21:16Z

@edoardopirovano Sorry...this dropped off my notifications page. Is this still a relevant change?

Yep, it would still be nice to get this in.

aeisenberg

🚢

edoardopirovano requested a review from a team as a code owner November 23, 2021 14:46

edoardopirovano marked this pull request as draft November 23, 2021 15:22

aeisenberg reviewed Nov 23, 2021

View reviewed changes

edoardopirovano force-pushed the cli-config-files branch from 1705961 to 101fb2e Compare December 9, 2021 15:31

edoardopirovano marked this pull request as ready for review December 9, 2021 15:31

edoardopirovano force-pushed the cli-config-files branch from 101fb2e to e29e255 Compare December 13, 2021 16:08

edoardopirovano requested a review from aeisenberg December 13, 2021 16:08

edoardopirovano force-pushed the cli-config-files branch from e29e255 to b1f2035 Compare January 4, 2022 14:14

aeisenberg reviewed Jan 7, 2022

View reviewed changes

edoardopirovano force-pushed the cli-config-files branch from b1f2035 to 5a8f51c Compare January 7, 2022 17:31

edoardopirovano force-pushed the cli-config-files branch 2 times, most recently from d07595a to 696ba6f Compare January 12, 2022 18:26

edoardopirovano force-pushed the cli-config-files branch 2 times, most recently from 682e7e7 to 8a78944 Compare February 16, 2022 13:27

Use --codescanning-config flag of CLI

0d87b8c

edoardopirovano force-pushed the cli-config-files branch from 8a78944 to 0d87b8c Compare February 16, 2022 13:34

aeisenberg approved these changes Feb 16, 2022

View reviewed changes

edoardopirovano merged commit c4e058a into github:main Feb 16, 2022

edoardopirovano deleted the cli-config-files branch February 16, 2022 17:08

github-actions bot mentioned this pull request Feb 17, 2022

Merge main into v1 #934

Merged

5 tasks

edoardopirovano mentioned this pull request Feb 17, 2022

Revert "Use --codescanning-config flag of CLI" #936

Merged

3 tasks

github-actions bot mentioned this pull request Feb 17, 2022

Merge main into v1 #939

Merged

5 tasks

edoardopirovano mentioned this pull request Mar 1, 2022

Second attempt to use --codescanning-config flag of CLI #957

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `--codescanning-config` flag of CLI #830

Use `--codescanning-config` flag of CLI #830

edoardopirovano commented Nov 23, 2021

edoardopirovano commented Nov 23, 2021

aeisenberg left a comment

edoardopirovano commented Nov 23, 2021

aeisenberg commented Nov 23, 2021

edoardopirovano commented Nov 23, 2021

edoardopirovano commented Dec 9, 2021

edoardopirovano commented Jan 4, 2022

aeisenberg Jan 7, 2022

edoardopirovano Jan 7, 2022

aeisenberg Jan 7, 2022

edoardopirovano Jan 7, 2022

aeisenberg Jan 7, 2022

edoardopirovano Jan 12, 2022

edoardopirovano commented Jan 7, 2022

aeisenberg commented Feb 2, 2022

edoardopirovano commented Feb 2, 2022

aeisenberg left a comment

Use --codescanning-config flag of CLI #830

Use --codescanning-config flag of CLI #830

Conversation

edoardopirovano commented Nov 23, 2021

Merge / deployment checklist

edoardopirovano commented Nov 23, 2021

aeisenberg left a comment

Choose a reason for hiding this comment

edoardopirovano commented Nov 23, 2021

aeisenberg commented Nov 23, 2021

edoardopirovano commented Nov 23, 2021

edoardopirovano commented Dec 9, 2021

edoardopirovano commented Jan 4, 2022

aeisenberg Jan 7, 2022

Choose a reason for hiding this comment

edoardopirovano Jan 7, 2022

Choose a reason for hiding this comment

aeisenberg Jan 7, 2022

Choose a reason for hiding this comment

edoardopirovano Jan 7, 2022

Choose a reason for hiding this comment

aeisenberg Jan 7, 2022

Choose a reason for hiding this comment

edoardopirovano Jan 12, 2022

Choose a reason for hiding this comment

edoardopirovano commented Jan 7, 2022

aeisenberg commented Feb 2, 2022

edoardopirovano commented Feb 2, 2022

aeisenberg left a comment

Choose a reason for hiding this comment

Use `--codescanning-config` flag of CLI #830

Use `--codescanning-config` flag of CLI #830