-
Notifications
You must be signed in to change notification settings - Fork 111
WIP Make Che use multi root workspaces by default #408
WIP Make Che use multi root workspaces by default #408
Conversation
Signed-off-by: Thomas Mäder <tmader@redhat.com>
Can one of the admins verify this patch? |
crw-ci-test |
E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:
|
if (cloneCommand.isInTheiaWorkspace()) { | ||
workspaceFolders.push(theia.Uri.file(cloneCommand.folder)); | ||
} | ||
cloneCommand.clone(); |
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.
cloneCommand.execute ?
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.
I am not in favor of having logic implementations here. It should go to the command implementation.
@@ -68,13 +69,13 @@ export class TheiaCloneCommand { | |||
this.projectsRoot = projectsRoot; | |||
} | |||
|
|||
execute(): PromiseLike<void> { |
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.
i don't see any good reasons to rename execute
to clone
.
This is a command. At the moment we tell Che-theia to clone (running clone commands) at start up but we may chain other commands that are not about cloning.
); | ||
await theia.commands.executeCommand('che.workspace.addFolder', workspaceFolders); |
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.
same comments i had for factory-initializer
I am not in favor of having logic implementations here. It should go to the command implementation.
You see here you are duplicating code.
Also, workspace root folders may be handle differently in the future see: eclipse-che/che#15347
currently superseed by #778 |
What does this PR do?
This PR makes Che use multi root workspaces by default.
It is still WIP because there are underlying issues with the vscode-languageclient-node library that causes a race condition between when a project is cloned and when the didUpdateWorkspaceFolders is enabled. See: microsoft/vscode-languageserver-node#451
Related issues for C# and PHP support:
eclipse-che/che#14223
eclipse-che/che#14225
This PR will be considered mergable when the vscode-languageserver-node fix is in and all the stacks are behaving as expected.
Currently, this is status of the stacks:
Testing involved using the sample and adding in:
as the cheEditor in the devfile. That referenced yaml is the result of building a che-theia image with these changes.
Then I waited for clone to finish, tested out features. Then added in a second project (either by cloning or by creating a new folder in /projects) and tested the features on that. For Golang, Python and NodeJS React I just made new sample project myself inside of the /projects folder and added them to the workspace. For PHP I cloned https://github.com/banago/simple-php-website.
What issues does this PR fix or reference?
Part of the fixes for eclipse-che/che#13427
Release Notes
Docs PR