Skip to content

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Jan 17, 2022

It is no longer necessary to check out a version of github/codeql as
a sibling directory to the distribution. Instead, users can download
the required packs as needed. using the pack download command orchange
the --download option for codeql database analyze.

Fixes https://github.com/github/codeql-core/issues/1957

Note that this change removes a few paragraphs on how to check out the codeql repository. For most external uses cases, this is not directly necessary.

Though, maybe we need something on local checkouts for how to contribute queries back. Suggestions welcome.

@aeisenberg aeisenberg marked this pull request as draft January 17, 2022 17:50
It is no longer necessary to check out a version of `github/codeql` as
a sibling directory to the distribution. Instead, users can download
the required packs as needed. using the `pack download` command or
the `--download` option for `codeql database analyze`.
Add more information about running packs. Include the `--download` flag.
We are no longer including information about how to check out
github/codeql, so this paragraph doesn't fit any more.
@aeisenberg aeisenberg force-pushed the aeisenberg/getting-started-docs branch from abb3219 to 01b5881 Compare January 18, 2022 23:48
@aeisenberg aeisenberg marked this pull request as ready for review January 19, 2022 00:00
@jf205
Copy link
Contributor

jf205 commented Jan 19, 2022

Hey @aeisenberg. Thanks for making these changes. I've had a very quick glance and it's great to see how simple the set up process now becomes when using CodeQL packages rather than checking out github/codeql.

I think we need to be careful about totally removing the section about checking out github/codeql at the moment though. For example, the examples about Running a single query and Running query suites with the CLI will currently only work if you have checked out github/codeql as i understand it. Additionally, I'm aware of some users who work almost exclusively from the command line rather than in VS Code, so we also need to make sure that we still provide the correct info for users who develop, test and run custom queries using the CLI. It may already be correct, but we should make sure.

Copy link
Contributor

@felicitymay felicitymay left a comment

Choose a reason for hiding this comment

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

Thank you for opening this pull request 💖

It seems like a great idea to simplify the documentation if we can do so without removing information that some users still need. Possibly we need an article for people who just want to get set up to run CodeQL CLI to generate results and another article for people who want to get set up for more complex purposes?

Having suffered various branch problems recently, I'm wondering whether this belongs in main or whether it should be merged into a branch to ensure that the changes are included in the docs updates for the correct release of the CLI.

I'm also wondering if we need to update the closely related articles Installing CodeQL CLI in your CI system and Configuring CodeQL CLI in your CI system. If these articles need to be updated, please open a docs-content issue.

I've made a couple of suggestions for formatting fixes but have not tested these changes.

Comment on lines +56 to +59
- a path to a query file
- a path to a directory containing query files
- a path to a query suite file
- the name of a CodeQL query pack
Copy link
Contributor

Choose a reason for hiding this comment

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

The formatting seems to be off here (at least in the docs generated by the checks on the PR). You may need an empty line before and after these nested bullets.

converted to ``.md`` before running the analysis. For further information,
- ``--sarif-add-query-help``: (supported in version 2.7.1 onwards) adds any custom query help written
in markdown to SARIF files (v2.1.0 or later) generated by the analysis. Query help stored in ``.qhelp`` files must be
converted to ``.md`` before running the analysis. For further information,
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're editing this file. The spacing on lines 70/71 is giving a weird result in the live output: https://codeql.github.com/docs/codeql-cli/analyzing-databases-with-the-codeql-cli/#running-codeql-database-analyze

Comment on lines 75 to 76
- ``--download``: a boolean flag that will allow the CLI to download any referenced CodeQL packages that are not available locally.
If this flag is missing and a referenced CodeQL package is not available locally, the command will fail.
Copy link
Contributor

Choose a reason for hiding this comment

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

This suffers from the same problem as lines 70/71 in the original article.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like there's an extra space at the start of the line. Fixing.


Afterwards, you can run the pack on a specific database::
codeql database analyze --download <database> microsoft/coding-standards@1.0.0 github/secutiry-queries --format=sarifv2.1.0 --output=query-results.sarif
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd tend to put the download flag later in the command to reduce the tendency for people to see it as connected in anyway to <database> but maybe this is something that no developer would think 🤔

it. Legacy packs ensure that custom queries and libraries created using older
products are compatible with your version of CodeQL.
default in your CodeQL CLI package.
- (Optional) You can download some ":ref"`CodeQL packs <about-codeql-packs>` containing pre-compiled queries you would like to run.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in the link here, probably should be:

Suggested change
- (Optional) You can download some ":ref"`CodeQL packs <about-codeql-packs>` containing pre-compiled queries you would like to run.
- (Optional) You can download some ":ref:`CodeQL packs <about-codeql-packs>`" containing pre-compiled queries you want to run.


Alternatively, you can download query packs during the analysis by using the `--download` flag of the `codeql database analyze`
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
Alternatively, you can download query packs during the analysis by using the `--download` flag of the `codeql database analyze`
Alternatively, you can download query packs during the analysis by using the ``--download`` flag of the ``codeql database analyze``

@aeisenberg
Copy link
Contributor Author

Possibly we need an article for people who just want to get set up to run CodeQL CLI to generate results and another article for people who want to get set up for more complex purposes?

Yes, I think it's best to add that information back somewhere.

Having suffered various branch problems recently, I'm wondering whether this belongs in main or whether it should be merged into a branch to ensure that the changes are included in the docs updates for the correct release of the CLI.

Perhaps retargeting to 2.8.0 is best.

I'm also wondering if we need to update the closely related articles Installing CodeQL CLI in your CI system and Configuring CodeQL CLI in your CI system. If these articles need to be updated, please open a docs-content issue.

Thanks for pointing out the two linked articles. I think the only section that could use some tidying up is analyzing-a-codeql-database. It's not exactly wrong bug could be cleared up in two ways:

  1. Explicitly calling pack download is optional. It might be easier to suggest running database analyze with the --download flag
  2. The description of <packs,queries>, can probably be made more explicit or at least changing the link so that it points to the examples in analyzing-databases-with-the-codeql-cli.rst.

I'll create an issue for this.

@aeisenberg
Copy link
Contributor Author

@aeisenberg
Copy link
Contributor Author

@jf205 That makes sense that we don't want to remove the old way of doing things. I can add back the old steps and make it clear that this is an optional, alternative approach. It is useful when you want to interact with the codeql query sources directly.

@felicitymay The precise timing of the next release is still being discussed and so no release branches have been created yet. I'll keep this PR targeted towards main. If a release branch is opened before this is merged, I will re-target.

Adds a second getting started, specifically for checking out the
codeql repo as a way to get the core queries.

This ensures that people wanting to work in the traditional way still
have the old docs available.
Co-authored-by: Felicity Chapman <felicitymay@github.com>
felicitymay
felicitymay previously approved these changes Jan 20, 2022
Copy link
Contributor

@felicitymay felicitymay left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. I think this is ready to merge from the text point of view, but I have not tested the instructions 😞

@aeisenberg
Copy link
Contributor Author

Thanks for the review @felicitymay.

@jf205, would you be able to take another look at this and see if this is sufficient from your perspective?

jf205
jf205 previously approved these changes Jan 21, 2022
Copy link
Contributor

@jf205 jf205 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 good to merge, pending a couple of small fixes 👍🏻

Thanks for adding the packaging information to the documentation @aeisenberg. It'll definitely be helpful for our users. I think @felicitymay's suggestion of perhaps separating out the different setup procedures for the different use cases is sensible and is something we should think about doing in the future, but no need to rush through it now.

@felicitymay
Copy link
Contributor

Thanks for spotting those issues James 🙈

@aeisenberg aeisenberg dismissed stale reviews from jf205 and felicitymay via aee9eb5 January 21, 2022 19:35
Co-authored-by: James Fletcher <42464962+jf205@users.noreply.github.com>
@aeisenberg
Copy link
Contributor Author

Thanks, James. Suggestions applied.

@aeisenberg
Copy link
Contributor Author

I need one more final review, and then I will merge.

@aeisenberg
Copy link
Contributor Author

v2.7.6 is now released. Can I get another review so I can merge this?

@felicitymay
Copy link
Contributor

This seems to have picked up some conflicts that would be good to fix before you get the final 👍🏻, otherwise fixing them will dismiss the 👍🏻

@aeisenberg
Copy link
Contributor Author

Merge conflicts addressed.

Copy link
Contributor

@felicitymay felicitymay left a comment

Choose a reason for hiding this comment

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

Thanks for these updates 🚀

@aeisenberg aeisenberg merged commit e722121 into main Jan 25, 2022
@aeisenberg aeisenberg deleted the aeisenberg/getting-started-docs branch January 25, 2022 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants