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

Ensure qlconfig file is created when config parsing in cli is on #1527

Merged
merged 7 commits into from
Feb 27, 2023

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Feb 6, 2023

Previously, with the config parsing in the cli feature flag turned on, the CLI was not able to download packs from other registries. This PR adds the codeql-action changes required for this. The CLI changes will be in a separate, internal PR.

This is a fix for a potential bug that has not yet been raised, so no changelog post is necessary.

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.

@aeisenberg aeisenberg requested a review from a team as a code owner February 6, 2023 21:28
@aeisenberg
Copy link
Contributor Author

I expect that init-with-registries.yml will fail since in this case the config.yml was not being used. It should pass if the CODEQL_PASS_CONFIG_TO_CLI is explicitly set to false. It will also pass after the corresponding CLI work is merged.

@aeisenberg aeisenberg force-pushed the aeisenberg/qlconfig-in-cli branch 2 times, most recently from b00e4e4 to 328773c Compare February 6, 2023 21:37
@aeisenberg aeisenberg marked this pull request as draft February 6, 2023 21:37
@aeisenberg aeisenberg removed the request for review from a team February 6, 2023 21:37
@aeisenberg aeisenberg force-pushed the aeisenberg/qlconfig-in-cli branch 3 times, most recently from 75fa980 to 826edd9 Compare February 6, 2023 23:44
@aeisenberg
Copy link
Contributor Author

Note that even though the PR Check - Packaging: Download using registries jobs are passing, they are not actually testing what they should be. Because we are not using extra private PATs in this repository, we can't test on other registries. I will be creating an internal PR with an integration test.

This PR also depends on a change to the CLI where we allow the --qlconfig-file option to be passed to database init.

Previously, with the config parsing in the cli feature flag turned on,
the CLI was not able to download packs from other registries. This PR
adds the codeql-action changes required for this. The CLI changes will
be in a separate, internal PR.
@aeisenberg aeisenberg marked this pull request as ready for review February 7, 2023 20:11
@aeisenberg
Copy link
Contributor Author

This is ready for review, even though it is not complete until the associated change in the codeql cli is available.

Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

This looks mostly good. Comments are mainly around maintainability and filling a few gaps in my knowledge.

src/codeql.test.ts Outdated Show resolved Hide resolved
src/codeql.test.ts Outdated Show resolved Hide resolved
src/codeql.test.ts Outdated Show resolved Hide resolved
src/codeql.test.ts Outdated Show resolved Hide resolved
src/codeql.test.ts Show resolved Hide resolved
src/config-utils.ts Outdated Show resolved Hide resolved
src/config-utils.ts Outdated Show resolved Hide resolved
src/config-utils.ts Show resolved Hide resolved
src/init.ts Outdated Show resolved Hide resolved
src/init.ts Outdated Show resolved Hide resolved
src/codeql.test.ts Outdated Show resolved Hide resolved
src/codeql.test.ts Show resolved Hide resolved
src/codeql.ts Outdated Show resolved Hide resolved
src/config-utils.test.ts Outdated Show resolved Hide resolved
src/config-utils.test.ts Outdated Show resolved Hide resolved
src/config-utils.ts Outdated Show resolved Hide resolved
src/config-utils.ts Outdated Show resolved Hide resolved
src/config-utils.ts Show resolved Hide resolved
src/init.ts Outdated Show resolved Hide resolved
Co-authored-by: Henry Mercer <henry.mercer@me.com>
@aeisenberg
Copy link
Contributor Author

aeisenberg commented Feb 9, 2023

@henrymercer can you take another look? There is a minor semantic change. If a user explicitly sets a CODEQL_REGISTRIES_AUTH env var, then this will override the generated env var based on inputs.

I think this provides a nice back door so that in case there are issues with the action not generating or passing the qlconfig or CODEQL_REGISTRIES_AUTH, a workflow can do that explicitly.

@aeisenberg aeisenberg force-pushed the aeisenberg/qlconfig-in-cli branch 2 times, most recently from a5f7338 to 60afa75 Compare February 9, 2023 21:30
henrymercer
henrymercer previously approved these changes Feb 10, 2023
@aeisenberg
Copy link
Contributor Author

I'm going to hold off on merging this until the related PR on the CLI side is merged as well. The assumption here is that the feature will be enabled officially for v2.12.3, but we won't be certain until the CLI PR is merged.

src/codeql.ts Outdated Show resolved Hide resolved
@aeisenberg
Copy link
Contributor Author

v2.12.3 is now available. I am picking this PR up again.

@aeisenberg
Copy link
Contributor Author

@henrymercer, can you take another look? Nothing has changed since your last approval.

henrymercer
henrymercer previously approved these changes Feb 27, 2023
src/codeql.ts Outdated Show resolved Hide resolved
lib/codeql.js Outdated
@@ -98,6 +98,10 @@ exports.CODEQL_VERSION_BETTER_RESOLVE_LANGUAGES = "2.10.3";
* Versions 2.11.1+ of the CodeQL Bundle include a `security-experimental` built-in query suite for each language.
*/
exports.CODEQL_VERSION_SECURITY_EXPERIMENTAL_SUITE = "2.12.1";
/**
* Versions 2.12.3+ of the CodeQL CLI support the `--qlconfig` flag in calls to `database init`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Versions 2.12.3+ of the CodeQL CLI support the `--qlconfig` flag in calls to `database init`.
* Versions 2.12.4+ of the CodeQL CLI support the `--qlconfig` flag in calls to `database init`.

@aeisenberg
Copy link
Contributor Author

Thanks for the review. Addressed your comments and need another approvel.

Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

:shipit:

@aeisenberg aeisenberg merged commit a589d40 into main Feb 27, 2023
@aeisenberg aeisenberg deleted the aeisenberg/qlconfig-in-cli branch February 27, 2023 18:26
@henrymercer henrymercer mentioned this pull request Mar 6, 2023
3 tasks
@github-actions github-actions bot mentioned this pull request Mar 10, 2023
6 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.

None yet

2 participants