-
Notifications
You must be signed in to change notification settings - Fork 189
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
Add "pack install" and "pack download" commands #1076
Conversation
1. Hard-code more common query packs 2. Correctly resolve workspace packs 3. Only install workspace packs
Thanks for the review! (And for improving my understanding of how people use these packaging commands 😅) Pushed some updates based your feedback. Next up: I'll add tests! 👩🏽🔬 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tried it out. Works beautifully. I didn't know there was a multi-select option for quick picks.
The changes I'm suggesting are not blockers, but I will hold off approval until there are some tests. |
To answer a question posed by Aditya in a comment (which I can't see right now, was it deleted?).
|
I'm struggling with tests (again)... 🙈 The functions I'm testing don't return anything, so I'm trying to just check the logging output. I've pushed my attempt so far. @aeisenberg, would you be able to take a look? 🙏🏽 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be nice to somehow test that the pack was actually downloaded, but there's no way to do it right now. You would need to run resolve queries
. So, for now, just testing whether the command throws or not is good enough.
extensions/ql-vscode/src/vscode-tests/cli-integration/packaging.test.ts
Outdated
Show resolved
Hide resolved
I'd forgotten to add a CLI version constraint for packaging 🤦🏽 🤦🏽 🤦🏽 (fixed in the latest commit) The mysterious failures have now disappeared, so I'm optimistic that this is now in good shape 🤞🏽 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job!
I have a slight concern that these tests will now be downloading packs into the global package cache. So, if you run it locally, the tests will hit your cache (and likely not actually download anything). But on CI, the cache will be empty on each run, so packages will be downloaded.
I don't think this is an issue since the tests are not really about the CLI, but only about how he extension interacts with the CLI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I have a few non-blocking suggestions about user-visible text, to indicate that pack install
is really installing pack dependencies. Feel free to incorporate them now or consider them for later.
ignoreFocusOut: true, | ||
}); | ||
if (packsToInstall && packsToInstall.length > 0) { | ||
progress({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: You know the total number of root packs, so you could make the progress monitor actually reflect the progress through the root packs. Save that for a future PR though :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! I will try that in a follow-up PR 🔄
Co-authored-by: Aditya Sharad <[email protected]>
Adds commands to install and download packs 📦
For the "Download Packs" command, I've added a hard-coded option to download all core packs, and a custom option if users want to specify a custom pack.
Expand for demo GIF
For "Install Packs", the user can select packs from their workspace
Expand for demo GIF
PS: I will add a test for these commands 🧪 But I'd be grateful for a quick review first to see if this is a plausible way of doing things!🙈 Done!Checklist
If this pull request makes user-facing changes that require documentation changes, theDocs PR: Docs: Mention packaging commands in CodeQL extension codeql#7661ready-for-doc-review
label has been added to this pull request or the corresponding issue.