Stop pushing QL pack as top level folder to avoid confusing the user by elenatanasoiu · Pull Request #2310 · github/vscode-codeql

elenatanasoiu · 2023-04-13T09:34:11Z

In the original flow for creating skeleton packs, we were starting out
by choosing a database (e.g. github/codeql) and having the extension
create the QL pack for us.

At that point, we were storing the QL pack together with the database in
the extension storage because we weren't interested in committing it to
the repo.

This means we weren't able to see it in the file explorer so in order to
make it visible, we made a decision to push it as a top-level folder in the
workspace.

Hindsight is 20/20.

Let's change this original flow by just creating the folder in the
in the repo instead of the extension storage (which will make it
visible in the file explorer) and stop pushing it as an extra top level folder
in the workspace.

This makes it consistent with what we're doing for the skeleton query
wizard where we start by:

picking a language
creating a QL pack in the repo
downloading a database to the extension storage

For this original flow we:

still download the database to the extension storage
we're now creating the QL pack in the repo

Bonus: For both flows we're not pushing the QL pack as a top folder
so it won't show up twice if you use the skeleton wizard flow, as Shati
discovered.

So that we can re-use it in the QLPackGenerator.

In the original flow for creating skeleton packs, we were starting out by choosing a database (e.g. github/codeql) and having the extension create the QL pack for us. At that point, we were storing the QL pack together with the database in the extension storage because we weren't interested in committing it to the repo. This means we weren't able to see it in the file explorer so in order to make it visible, we decided to push it as a top-level folder in the workspace. Hindsight is 20/20. Let's change this original flow by just creating the folder in the workspace storage instead of the extension storage (which will make it visible in the file explorer) and stop pushing it as an extra top level folder in the workspace. NB: For this flow, we exit early in the `createSkeletonPacks` method if the folder already exists so we don't need to check this again.

shati-patel

Looks good! Though I think there's still some confusion around naming (left comments inline). Let me know if they don't make sense! 🙃

💭 Rough overview of my understanding of the 3 kinds of "storage":

The folders that you have open in your workspace (shown in the sidebar)
A folder generated/managed by the extension, which stores databases and general "state" about a particular workspace. There's a different folder per workspace.
- This is what we typically call "workspace storage".
- Looks something like User\workspaceStorage\e261f5bb3c3fc94ac91b18302744f6ad\GitHub.vscode-codeql\...
A folder generated/managed by the extension, which stores the CodeQL CLI executable and various other "global" things.
- This is what we typically call "global storage".
- Looks something like User\globalStorage\github.vscode-codeql\...

This PR is about adding QL packs to 1, rather than 2, which is why "workspace storage" is a slightly confusing name! 🗳️

shati-patel

Cool! 😎 Just a note from testing: now that we no longer add codeql-custom-queries-xxx as a top-level folder, we get into a clash with the previous "skeleton pack generator" ☠️ 😄

E.g. When running Create Query in the codespaces-codeql repo, it successfully creates codeql-custom-queries-xxx as a subfolder of my first workspace folder, and then adds a database.
After the database gets added, I get the message

We've noticed you don't have a CodeQL pack available to analyze this database. Can we set up a query pack for you?

which would try to create another pack.

Can we turn off this prompt, or change isFolderAlreadyInWorkspace to also check for subfolders?

elenatanasoiu · 2023-04-13T16:01:42Z

E.g. When running Create Query in the codespaces-codeql repo, it successfully creates codeql-custom-queries-xxx as a subfolder of my first workspace folder, and then adds a database.
After the database gets added, I get the message

We've noticed you don't have a CodeQL pack available to analyze this database. Can we set up a query pack for you?
which would try to create another pack.

That's weird. When I tested this I didn't get that behaviour. This line should stop it from trying to generate again. 🤔

I'll do some more testing on this. Thanks!

elenatanasoiu · 2023-04-13T16:09:11Z

Oh I see what you mean by subfolders 👍

adityasharad · 2023-04-13T16:20:14Z

Are you planning to make the choice of workspace folder configurable in future, or unconditionally create the pack within the first workspace folder?

elenatanasoiu · 2023-04-13T16:49:11Z

Are you planning to make the choice of workspace folder configurable in future, or unconditionally create the pack within the first workspace folder?

@adityasharad Not at the moment. The main target is people that are using the Codespace template. Once we get to implementing the new Queries panel in the extension, we can revisit. Do you think it's something we should consider earlier?

adityasharad · 2023-04-13T18:21:16Z

Are you planning to make the choice of workspace folder configurable in future, or unconditionally create the pack within the first workspace folder?

@adityasharad Not at the moment. The main target is people that are using the Codespace template. Once we get to implementing the new Queries panel in the extension, we can revisit. Do you think it's something we should consider earlier?

In an arbitrary workspace I would find it somewhat confusing as a user to have a new pack quietly written to my first workspace folder; for the Codespaces template we have more control and know exactly what the first folder is so it makes sense. Should not hold up this PR, but I think it's worth considering a follow-up of having the unconditional first-folder insertion only in the template workspace, and a user choice of folder in other workspaces.

When running Create Query in the codespaces-codeql repo, it successfully creates codeql-custom-queries-xxx as a subfolder of the first workspace folder, and then adds a database. After the database gets added, we get prompted with this message: ``` We've noticed you don't have a CodeQL pack available to analyze this database. Can we set up a query pack for you? ``` which would try to create another QL pack. Since we're no longer pushing QL packs as top level folders in the workspace when we use the new "Create Query" flow, we also need to adapt the original flow to take into account subfolders. Just as a reminder, the original flow is: - Be in the codespace template - Download a database from GitHub - The extension will offer to create a QL pack for the database The new flow: - Run the "Create Query" command - Choose a language - Create a QL pack - Download a database for it In the new flow the last step of downloading a database would trigger the extension to offer to create a QL pack. Let's fix this by detecting subfolders as well and exiting early.

We've made an exception to fetch the parent folder when we're in the vscode-codeql-starter workspace. We'd like to make this more specific so that it doesn't interfere with other repos.

elenatanasoiu · 2023-04-14T11:19:23Z

Thanks for your input everyone! This PR is ready for review again.

I have some time to look into being able to configure the workspace folder. Discussed this with Shati, we want to prompt non-codespace users for a workspace folder and then save it as config.

shati-patel

One more comment from me 😎

(I haven't got round to testing the "Don't offer to create skeleton pack again" change yet, though the code looks good 💻 )

shati-patel · 2023-04-14T11:21:56Z

+  // For the vscode-codeql-starter repo, the first folder will be a ql pack
+  // so we need to get the parent folder
+  if (
+    firstFolderFsPath.includes("vscode-codeql-starter/codeql-custom-queries")


This might need to use platform-specific separators, e.g. with path.join or path.sep 🪟

Thanks Shati! You'd think I would've learned my lesson by now!

The separator character is different on Windows.

aeisenberg

Nice

elenatanasoiu · 2023-04-14T20:24:38Z

Thanks @aeisenberg !

Offer non-codespace users the option to configure their storage folder for skeleton packs. Suggested here: #2310 (comment) At the moment we're choosing to create our skeleton packs in the first folder in the workspace. This is fine for the codespace template because we can control the folder structure in that repo. For users outside of this we'd like to offer them the option to choose where to save their skeleton packs.

elenatanasoiu added 2 commits April 13, 2023 09:07

Move getFirstStoragePath method into helpers

9a2de39

So that we can re-use it in the QLPackGenerator.

elenatanasoiu requested a review from a team as a code owner April 13, 2023 09:34

elenatanasoiu requested a review from a team April 13, 2023 09:44

elenatanasoiu added the secexp label Apr 13, 2023

koesie10 reviewed Apr 13, 2023

View reviewed changes

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

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

shati-patel reviewed Apr 13, 2023

View reviewed changes

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

Comment thread extensions/ql-vscode/src/local-databases.ts Outdated

elenatanasoiu force-pushed the yer-a-workspace-query branch from c8d6611 to dbcbaa5 Compare April 13, 2023 14:49

elenatanasoiu added 2 commits April 13, 2023 14:58

Rename getFirstStoragePath -> getFirstWorkspaceFolder

5634c1e

Add a JSDoc description for the method

b83d54f

elenatanasoiu force-pushed the yer-a-workspace-query branch from dbcbaa5 to 669c724 Compare April 13, 2023 14:59

shati-patel reviewed Apr 13, 2023

View reviewed changes

Comment thread extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts Outdated

elenatanasoiu added 2 commits April 13, 2023 15:57

Use getOnDiskWorkspaceFolders to fetch folders

14d8593

Rename workspaceStorage -> firstWorkspaceFolder

b6eaf93

elenatanasoiu force-pushed the yer-a-workspace-query branch from 669c724 to b6eaf93 Compare April 13, 2023 15:58

aeisenberg reviewed Apr 13, 2023

View reviewed changes

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

elenatanasoiu added 2 commits April 14, 2023 09:55

Check specifically for the starter workspace

bfc5f49

We've made an exception to fetch the parent folder when we're in the vscode-codeql-starter workspace. We'd like to make this more specific so that it doesn't interfere with other repos.

shati-patel reviewed Apr 14, 2023

View reviewed changes

Make check platform agnostic

70d533f

The separator character is different on Windows.

elenatanasoiu force-pushed the yer-a-workspace-query branch from b9e734a to 70d533f Compare April 14, 2023 12:31

Merge branch 'main' into yer-a-workspace-query

9c79799

aeisenberg approved these changes Apr 14, 2023

View reviewed changes

elenatanasoiu merged commit 9b647ff into main Apr 14, 2023

elenatanasoiu deleted the yer-a-workspace-query branch April 14, 2023 20:24

elenatanasoiu mentioned this pull request Apr 17, 2023

Prompt non-codespace users for storage path #2341

Merged

Conversation

elenatanasoiu commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shati-patel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shati-patel 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!

elenatanasoiu commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elenatanasoiu commented Apr 13, 2023

Uh oh!

adityasharad commented Apr 13, 2023

Uh oh!

Uh oh!

elenatanasoiu commented Apr 13, 2023

Uh oh!

adityasharad commented Apr 13, 2023

Uh oh!

elenatanasoiu commented Apr 14, 2023

Uh oh!

shati-patel left a comment

Choose a reason for hiding this comment

Uh oh!

shati-patel Apr 14, 2023

Choose a reason for hiding this comment

Uh oh!

elenatanasoiu Apr 14, 2023

Choose a reason for hiding this comment

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

elenatanasoiu commented Apr 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

elenatanasoiu commented Apr 13, 2023 •

edited

Loading

shati-patel left a comment •

edited

Loading

elenatanasoiu commented Apr 13, 2023 •

edited

Loading