Download databases from GitHub by shati-patel · Pull Request #1229 · github/vscode-codeql

shati-patel · 2022-03-22T16:16:02Z

Will fix #1123.

Adds a new command to download a database directly from GitHub. This borrows heavily from existing functionality like the "Download database from LGTM" command 😅

The main difference is that we need to pass in credentials to authenticate to the GitHub API. We could use octokit directly for downloading the DB, but then we wouldn't be able to re-use all the nice functionality from databaseArchiveFetcher. So instead we get the octokit token (as previously done in #952) and pass in some custom API headers.

Still TODO:

Get a GitHub logo for the database UI (any ideas where I can find this in light/dark versions?)
- See Download databases from GitHub #1229 (comment)
Add some tests

(probably in a follow-up PR)

Give the imported database a more helpful name
Allow GitHub repo URLs as well as NWOs in the input box (like we do for LGTM): Allow GitHub URL as well as NWO #1241

Checklist

N/A, since this is now also internal-only 🔐

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
[Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

adityasharad

Code changes look good! Minor suggestions only. Would it be possible to add unit tests for the new command, based on any we have for the existing LGTM command? (EDIT: I see those are in progress, reviewed too quickly!)

adityasharad · 2022-03-22T18:37:55Z

For logos, we currently use icons from both https://github.com/primer/octicons and https://github.com/microsoft/vscode-icons in this repo, and both have a GitHub icon. Give those a try :)

From https://github.com/microsoft/vscode-icons

shati-patel · 2022-03-23T16:06:25Z

Thanks for the helpful review! 😍 Addressed the comments, and made another small change to hide this command behind the canary flag.

Currently working on unit tests! 🧪 😅

For logos, we currently use icons from both https://github.com/primer/octicons and https://github.com/microsoft/vscode-icons in this repo, and both have a GitHub icon. Give those a try :)

Thank you! Found in https://github.com/microsoft/vscode-icons. Only thing is that some of the other logos have a small green "plus" ➕

I don't think it matters too much that we don't have that for the GitHub (and general "cloud") logos, especially since it's hidden behind canary.

shati-patel · 2022-03-24T12:37:03Z

Added some tests in d7a1e42!
It's not ideal, since we can't easily make any real octokit requests, but I've tested with a few mock API responses instead. Re-reviews/suggestions welcome 🌅 🙏🏽

aeisenberg · 2022-03-24T16:50:38Z

+    const [owner, repo] = githubRepo.split('/');
+
+    const octokit = await credentials.getOctokit();
+    const response = await octokit.request('GET /repos/:owner/:repo/code-scanning/codeql/databases', { owner, repo });


I'm pretty sure we're ok here, but can you make sure that we can handle misplaced cApS? Remember the boinc/BOINC repo that gave us trouble for LGTM?

Also, does this support anonymous downloading of public repos?

The API currently does not allow anonymous requests because it is only staff shipped. Even in the private beta it will still require logging in. Once we get to public beta it will likely start accepting anonymous requests for public repos.

However for the extension I'd argue we still want to always make the user log in. It'll simplify our lives because we only have to handle one case. And I would expect the requirement to log in is not a blocked to users here.

Thanks for the explanation. I think that's fine for now. It does seem like a small barrier for users who are not using any other github features. This is something we can discuss later when the API endpoint starts accepting anonymous requests.

I'm pretty sure we're ok here, but can you make sure that we can handle misplaced cApS? Remember the boinc/BOINC repo that gave us trouble for LGTM?

Good point, I hadn't thought of that 💪🏽 From a bit of manual testing, it looks like the API is case-insensitive (e.g. GitHub/CodeQL returns the same as github/codeql). So I think we're okay here!

Also, does this support anonymous downloading of public repos?

Not yet. The entire MRVA API is still staff-only, so we need to pass in credentials for now 🔒

aeisenberg · 2022-03-24T16:56:18Z

  });
  if (!languages.length) {
-    return;
+    throw new Error('No databases found');


This is also a change to downloading LGTM databases. I can't remember if there was a reason why we didn't throw when there were no languages found for an LGTM project. Can you just make sure that nothing awkward happens when downloading LGTM dbs?

I wasn't able to find an LGTM project that doesn't have any languages, so I suspect that's a sufficiently rare case that we've never needed to handle it previously 😅

aeisenberg · 2022-03-24T16:58:11Z

 * - `repo` is made up of alphanumeric characters, hyphens, or underscores
 */
-const REPO_REGEX = /^(?:[a-zA-Z0-9]+-)*[a-zA-Z0-9]+\/[a-zA-Z0-9-_]+$/;
+export const REPO_REGEX = /^(?:[a-zA-Z0-9]+-)*[a-zA-Z0-9]+\/[a-zA-Z0-9-_]+$/;


Probably good to move this to a new file. I don't think databaseFetcher should be depending on run-remote-query. Maybe helpers or helpers-pure.

Done! ✔️

adityasharad

Good stuff. I think you've addressed my comments, and I'll leave @aeisenberg to approve when he's happy.

adityasharad · 2022-03-24T18:14:55Z

@@ -0,0 +1,3 @@
+<svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg">


Consider an HTML comment (if SVG allows them) or a README file stating where you got these icons from.

adityasharad · 2022-03-24T18:15:34Z

+  }
+
+  const databaseUrl = await convertGithubNwoToDatabaseUrl(githubRepo, credentials, progress);
+  if (databaseUrl) {


Minor: You can do the same early-exit trick here:

if (!databaseUrl) { return } // continue

adityasharad · 2022-03-24T18:16:30Z

+     * }
+     * We only need the actual token string.
+     */
+    const octokitToken = await octokit.auth() as { token: string };


Minor: Could this be (await octokit.auth())?.token, and then you don't need to use .token everywhere below?

Makes sense! I still had to keep as { token: string }, since await octokit.auth() is of type "unknown" otherwise 🤷🏽

adityasharad · 2022-03-24T18:19:18Z

+      await commands.executeCommand('codeQLDatabases.focus');
+      void showAndLogInformationMessage('Database downloaded and imported successfully.');
+    }
+    return item;


Minor: Move this up into the if(item) case, so it's more obvious what's going on. The return at the end of the function will handle all the fall-through cases where we didn't return early.

aeisenberg

Thanks for looking into all of my comments. I think you've addressed Aditya's as well. So... 🚢.

shati-patel added 2 commits March 22, 2022 15:52

Download dbs from GitHub

45d805d

Pass in credentials for authenticating to GitHub API

bc727d5

shati-patel marked this pull request as ready for review March 22, 2022 17:26

shati-patel requested review from a team as code owners March 22, 2022 17:26

adityasharad reviewed Mar 22, 2022

View reviewed changes

Comment thread extensions/ql-vscode/src/databaseFetcher.ts Outdated

Comment thread extensions/ql-vscode/src/authentication.ts Outdated

Comment thread extensions/ql-vscode/src/databaseFetcher.ts

shati-patel added 3 commits March 23, 2022 12:51

Use GitHub icon in Databases UI

18bfd67

From https://github.com/microsoft/vscode-icons

Get octokit token using .auth()

9c21b66

Hide "Download db from GH" behind canary flag 🤫

b5a0ee6

Add tests

d7a1e42

shati-patel mentioned this pull request Mar 24, 2022

Support importing databases directly from github #1123

Closed

aeisenberg reviewed Mar 24, 2022

View reviewed changes

Move REPO_REGEX into helper file

ed7b285

adityasharad approved these changes Mar 24, 2022

View reviewed changes

Add source to SVG files + address code comments

1bf8cc6

aeisenberg approved these changes Mar 25, 2022

View reviewed changes

shati-patel merged commit 90d636a into main Mar 25, 2022

shati-patel deleted the shati-patel/download-gh-db branch March 25, 2022 15:24

shati-patel mentioned this pull request Mar 28, 2022

Allow GitHub URL as well as NWO #1241

Merged

3 tasks

		@@ -0,0 +1,3 @@
		<svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg">

Conversation

shati-patel commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

adityasharad left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adityasharad commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shati-patel commented Mar 23, 2022

Uh oh!

shati-patel commented Mar 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityasharad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shati-patel commented Mar 22, 2022 •

edited

Loading

adityasharad left a comment •

edited

Loading

adityasharad commented Mar 22, 2022 •

edited

Loading