Skip to content
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

feat(vscode): add support for "workbench.extensions.installExtension" #8745

Merged
merged 1 commit into from
Dec 2, 2020

Conversation

fgreinacher
Copy link
Contributor

@fgreinacher fgreinacher commented Nov 12, 2020

What it does

Adds support for the workbench.extensions.installExtension command (part of #8387).

We want this because we're considering to use https://github.com/joelspadin-garmin/vscode-private-extension-manager which depends on this API to install extensions.

🛠️ with ❤️ by @siemens

How to test

  1. Build the Example app
  2. Put the test extension in the plugins directory
  3. Execute the command "Install extension via ID and VS Code API" with a valid an extension ID (e.g. redhat.java).
  4. Execute the command "Install extension via VSIX and VS Code API" with the URI to a locally available VSIX file (something like file:///home/gitpod/temp/some-other.vsix)
  5. Ensure that both extensions have been installed via the extension manager.

Review checklist

Reminder for reviewers

@fgreinacher fgreinacher force-pushed the feat/extension-install branch from 9741a4e to 2c1b65d Compare November 12, 2020 14:01
@fgreinacher
Copy link
Contributor Author

fgreinacher commented Nov 12, 2020

Am I missing anything important or is it really that simple? I could not find any existing tests for this layer, could you guide me a bit here?

@fgreinacher fgreinacher force-pushed the feat/extension-install branch 2 times, most recently from 63f44b7 to 097079f Compare November 13, 2020 08:42
@fgreinacher fgreinacher marked this pull request as ready for review November 13, 2020 09:44
@vince-fugnitto vince-fugnitto added vsx-registry Issues related to Open VSX Registry Integration vscode issues related to VSCode compatibility labels Nov 13, 2020
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@fgreinacher can you update the 'how to test' section of the pull-request so reviewers can easily review and test the functionality? Generally, in order to review changes to the plugin API, a corresponding plugin is provided or implemented to validate the feature.

@DucNgn
Copy link
Contributor

DucNgn commented Nov 13, 2020

I created a quick test plugin to verify this PR. But the changes are not working on my side atm.

Steps

  • Download test plugin (source code) and place in plugins folder in theia.

  • Restart the example application.

  • Call Install Material Theme Icons (test installing extension by id) from the command palette.

  • Material Theme Icons should be installed (it is not currently).

@fgreinacher
Copy link
Contributor Author

Thanks a ton @dukengn!

I played around a bit. The problem is that the plugin server currently cannot deploy .vsix files. Deploying extensions by ID works when adding the vscode:extension/ prefix, e.g. `vscode:extension/pkief.material-icon-theme".

@fgreinacher fgreinacher force-pushed the feat/extension-install branch 3 times, most recently from 62a341b to 1e53885 Compare November 14, 2020 00:59
@fgreinacher fgreinacher force-pushed the feat/extension-install branch from 1e53885 to e7dd47c Compare November 14, 2020 01:23
@fgreinacher
Copy link
Contributor Author

@fgreinacher can you update the 'how to test' section of the pull-request so reviewers can easily review and test the functionality? Generally, in order to review changes to the plugin API, a corresponding plugin is provided or implemented to validate the feature.

@vince-fugnitto I added some test steps, hope they are understandable! Looking forward to some feedback!

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I believe that the following comment will need to be addressed:

I'm however not sure about the proper approach, for example if we should merge file and directory resolvers.

@fgreinacher fgreinacher force-pushed the feat/extension-install branch 3 times, most recently from 0afd653 to ea032a2 Compare November 18, 2020 21:13
@fgreinacher
Copy link
Contributor Author

I believe that the following comment will need to be addressed:

I'm however not sure about the proper approach, for example if we should merge file and directory resolvers.

I unified the common logic in ea032a2.

@azatsarynnyy
Copy link
Member

@fgreinacher does this PR enables installing VS Code extensions by ID from Microsoft's Marketplace?

@fgreinacher
Copy link
Contributor Author

@fgreinacher does this PR enables installing VS Code extensions by ID from Microsoft's Marketplace?

Nope, that is not be allowed. It just adds support for the two workflows mentioned in the "How to test" section where in the "Install by ID" workflow the data source would be the registry the VSX extension is connected to, by default OpenVSX.org.

@azatsarynnyy
Copy link
Member

@fgreinacher let me know when you fix the latest comment from @vince-fugnitto and I'll merge it.
Please, don't forget to squash the commits.

@fgreinacher fgreinacher force-pushed the feat/extension-install branch 2 times, most recently from 2816f57 to 2761092 Compare November 20, 2020 17:18
@fgreinacher
Copy link
Contributor Author

@vince-fugnitto Good hint, I updated the changelog.
@azatsarynnyy All done and ready for merge once CI is finished 👍🏽

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@fgreinacher fgreinacher force-pushed the feat/extension-install branch 2 times, most recently from 0a21765 to c372d16 Compare November 20, 2020 18:49
@fgreinacher
Copy link
Contributor Author

CI does not seem to be triggered anymore. Any ideas @vince-fugnitto?

@vince-fugnitto
Copy link
Member

CI does not seem to be triggered anymore. Any ideas @vince-fugnitto?

I'm not sure, an easy fix would be to perform an amend and force push.

@vince-fugnitto
Copy link
Member

CI does not seem to be triggered anymore. Any ideas @vince-fugnitto?

I believe it is related to the changes by Travis: #8765 (comment).

@fgreinacher
Copy link
Contributor Author

CI does not seem to be triggered anymore. Any ideas @vince-fugnitto?

I believe it is related to the changes by Travis: #8765 (comment).

😿

@azatsarynnyy
Copy link
Member

CI does not seem to be triggered anymore. Any ideas @vince-fugnitto?

I'm not sure, an easy fix would be to perform an amend and force push.

@fgreinacher could you try this suggestion ^^ in order to retrigger the tests on Travis?

@DucNgn
Copy link
Contributor

DucNgn commented Nov 23, 2020

CI does not seem to be triggered anymore. Any ideas @vince-fugnitto?

I'm not sure, an easy fix would be to perform an amend and force push.

@fgreinacher could you try this suggestion ^^ in order to retrigger the tests on Travis?

We have run out of credits on Travis so the CI is temporarily disabled for all PRs at the moment.

@fgreinacher fgreinacher force-pushed the feat/extension-install branch 3 times, most recently from 0ad933d to 15b935b Compare December 1, 2020 18:40
@fgreinacher
Copy link
Contributor Author

@azatsarynnyy
Copy link
Member

Hm, one job seems to be stuck: https://github.com/eclipse-theia/theia/pull/8745/checks?check_run_id=1481816411

I've rerun it

@azatsarynnyy
Copy link
Member

Hm, one job seems to be stuck: https://github.com/eclipse-theia/theia/pull/8745/checks?check_run_id=1481816411

I saw that check has failed on other PRs as well but for a different reason.
I'm not sure if can skip it to merge the PR.
cc @kittaakos @vince-fugnitto

Copy link
Contributor

@DucNgn DucNgn left a comment

Choose a reason for hiding this comment

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

Hm, one job seems to be stuck: #8745 (checks)

I saw that check has failed on other PRs as well but for a different reason.
I'm not sure if can skip it to merge the PR.
cc @kittaakos @vince-fugnitto


@theia/example-browser: Error: The number of constructor arguments in the derived class LocalDirectoryPluginDeployerResolver must be >= than the number of constructor arguments of its base class.\n    at 
/home/runner/work/theia/theia/node_modules/inversify/lib/planning/planner.js:111:27\n    at Array.forEach (<anonymous>)\n    at _createSubRequests 
(/home/runner/work/theia/theia/node_modules/inversify/lib/planning/planner.js:94:20)\n    at 
/home/runner/work/theia/theia/node_modules/inversify/lib/planning/planner.js:115:17\n    at Array.forEach (<anonymous>)\n    at 
/home/runner/work/theia/theia/node_modules/inversify/lib/planning/planner.js:114:26\n    at Array.forEach (<anonymous>)\n    at _createSubRequests 
(/home/runner/work/theia/theia/node_modules/inversify/lib/planning/planner.js:94:20)\n    at 
/home/runner/work/theia/theia/node_modules/inversify/lib/planning/planner.js:115:17\n    at Array.forEach (<anonymous>)

https://github.com/eclipse-theia/theia/pull/8745/checks?check_run_id=1482104865#step:7:24

Looking at the log, I think the CI is failing because of the changes from this PR?

Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

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

I've re-checked it again after the changes have been made.
Works as expected. Thanks!

@azatsarynnyy azatsarynnyy merged commit f1e37fb into eclipse-theia:master Dec 2, 2020
@fgreinacher fgreinacher deleted the feat/extension-install branch December 2, 2020 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vscode issues related to VSCode compatibility vsx-registry Issues related to Open VSX Registry Integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants