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

Use --codescanning-config flag of CLI #830

Merged
merged 1 commit into from
Feb 16, 2022

Conversation

edoardopirovano
Copy link
Contributor

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 edoardopirovano requested a review from a team as a code owner November 23, 2021 14:46
@edoardopirovano
Copy link
Contributor Author

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+.

@edoardopirovano edoardopirovano marked this pull request as draft November 23, 2021 15:22
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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 edoardopirovano marked this pull request as ready for review December 9, 2021 15:31
@edoardopirovano
Copy link
Contributor Author

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
Copy link
Contributor Author

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!

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}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines +706 to +718
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}`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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

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

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

@edoardopirovano
Copy link
Contributor Author

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

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

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

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

🚢

@edoardopirovano edoardopirovano merged commit c4e058a into github:main Feb 16, 2022
@edoardopirovano edoardopirovano deleted the cli-config-files branch February 16, 2022 17:08
@github-actions github-actions bot mentioned this pull request Feb 17, 2022
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.

2 participants