Skip to content

Update the analyze databases article #10459

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Sep 28, 2022
Merged

Update the analyze databases article #10459

merged 7 commits into from
Sep 28, 2022

Conversation

aeisenberg
Copy link
Contributor

This change updates the analyze databases article to clarify examples. It reorganizes to put packs examples first and rearranges a few paragraphs.

@aeisenberg
Update the analyze databases article
027365c 
This change updates the analyze databases article to clarify examples.
It reorganizes to put packs examples first and rearranges a few
paragraphs.
@aeisenberg aeisenberg requested a review from jf205 September 16, 2022 21:05
jf205
jf205 reviewed Sep 21, 2022
View reviewed changes
@aeisenberg @jf205
Apply suggestions from code review
7607e67 
Co-authored-by: James Fletcher <42464962+jf205@users.noreply.github.com>
aeisenberg
aeisenberg commented Sep 21, 2022
View reviewed changes
Copy link
Contributor Author

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

Thanks for the review!

aeisenberg
aeisenberg commented Sep 21, 2022
View reviewed changes
@aeisenberg
Copy link
Contributor Author

Got an odd failure when running the docs job

HTTP request sent, awaiting response... 503 Egress is over the account limit.
2022-09-21 16:21:56 ERROR 503: Egress is over the account limit..

Rerunning.

@aeisenberg
Copy link
Contributor Author

Thanks for the fix @jf205. Is this good to merge now?

@jf205 jf205 added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Sep 22, 2022
@jf205
Copy link
Contributor

jf205 commented Sep 22, 2022

LGTM, but let's get a review from the docs team too. I've added the magic label.

@aeisenberg
Copy link
Contributor Author

@github/docs-content is there anyone able to take a look at this? Thank you.

adityasharad
adityasharad reviewed Sep 27, 2022
View reviewed changes
Copy link
Collaborator

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Looks good, some minor suggestions.

docs/codeql/codeql-cli/analyzing-databases-with-the-codeql-cli.rst Outdated
- :doc:`Set up the CodeQL CLI <getting-started-with-the-codeql-cli>` so that it can find the queries
and libraries included in the CodeQL repository.
- :doc:`Set up the CodeQL CLI <getting-started-with-the-codeql-cli>` to run commands locally and
optionally check out the CodeQL repository if you want direct access to the CodeQL core queries
Copy link
Collaborator

Choose a reason for hiding this comment

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

Avoid highlighting the idea of checking out the repo here. If they are using a bundle or packs there is no need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

docs/codeql/codeql-cli/analyzing-databases-with-the-codeql-cli.rst Outdated
Comment on lines 291 to 292
* ``cpp-security-and-quality`` - Security-and-quality queries for C++.
* ``cpp-security-extended`` - Security-extended queries for C++. This suite contains queries that are less precise than the standard security queries, and may find more false-positives.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reorder these: security-extended should go first, and highlight that each includes the suites above it.

@aeisenberg
Re-order query suite descriptions
aefd516 
Add a pull quote and apply some suggestions from code review.
@aeisenberg
Copy link
Contributor Author

I made a slightly bigger change than you suggested, @adityasharad. I created a pull-quote and moved some of the query suite descriptions into the quote.

adityasharad
adityasharad previously approved these changes Sep 27, 2022
View reviewed changes
Copy link
Collaborator

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Good stuff. I'm happy with the content, over to docs team for final editorial review.

subatoi
subatoi reviewed Sep 28, 2022
View reviewed changes
Copy link
Contributor

@subatoi subatoi left a comment

Choose a reason for hiding this comment

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

Hey @aeisenberg 👋 I joined Docs recently, nice to meet you! I've left some comments (mostly minor and related to style), and will be very happy to take another look after you've reviewed, or discuss anything. Thank you!

@subatoi
Copy link
Contributor

subatoi commented Sep 28, 2022

@aeisenberg Felicity mentioned to me in passing that these docs follow an older style (I hadn't realised!) so if some of my comments related to that are introducing too much overhead for you in this PR, we'll be happy to take care of the edits down the road in the docs team 😀

aeisenberg
aeisenberg commented Sep 28, 2022
View reviewed changes
@aeisenberg
Copy link
Contributor Author

@subatoi, I'm having trouble merging your suggestions inline in the PR. It looks like there's some sort of UI but where github is complaining about multiple suggestions on the same line being committed together (which I am not doing). I'm probably going to file an issue.

@subatoi
Copy link
Contributor

subatoi commented Sep 28, 2022

@subatoi, I'm having trouble merging your suggestions inline in the PR. It looks like there's some sort of UI but where github is complaining about multiple suggestions on the same line being committed together (which I am not doing). I'm probably going to file an issue.

@aeisenberg Let me know if you need me to commit anything if you're still having trouble!

@aeisenberg
Copy link
Contributor Author

I raised this issue: https://github.com/github/pull-requests/issues/5684

I think I can move ahead by committing suggestions one-by-one. If not, I'll just reproduce your suggestions locally. Thanks for your review!

@aeisenberg @subatoi
Update docs/codeql/codeql-cli/analyzing-databases-with-the-codeql-cli…
e8a0d07 
….rst

Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>
@aeisenberg aeisenberg force-pushed the aeisenberg/update-analyzing-databases branch from c8dbab9 to e8a0d07 Compare September 28, 2022 16:59
@aeisenberg
Copy link
Contributor Author

I think everything is resolved, but I may have missed something due to the issues I mentioned above. I squashed a bunch of the commits generated from suggestions. Please let me know if there is anything left to discuss.

subatoi
subatoi approved these changes Sep 28, 2022
View reviewed changes
Copy link
Contributor

@subatoi subatoi left a comment

Choose a reason for hiding this comment

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

Thanks @aeisenberg! 🚀

@aeisenberg aeisenberg merged commit ffd5886 into main Sep 28, 2022
@aeisenberg aeisenberg deleted the aeisenberg/update-analyzing-databases branch September 28, 2022 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jf205 jf205 jf205 left review comments

@subatoi subatoi subatoi approved these changes

@adityasharad adityasharad adityasharad left review comments

Assignees
No one assigned
Labels
documentation ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aeisenberg @jf205 @subatoi @adityasharad