Add backward compatibility related to switching to multi root mode #1074

RomanNikitenko · 2021-04-10T09:51:10Z

Signed-off-by: Roman Nikitenko rnikiten@redhat.com

What does this PR do?

Provide backward compatibility for workspaces which were created before switching multi-root mode to ON by default.

Screenshot/screencast of this PR

Reviewers

Reviewers, please comment how you tested the PR when approving it.

Happy Path Channel

HAPPY_PATH_CHANNEL=next

codecov · 2021-04-10T10:03:27Z

Codecov Report

Merging #1074 (f4941d8) into master (eeb4526) will increase coverage by 1.88%.
The diff coverage is 74.24%.

❗ Current head f4941d8 differs from pull request most recent head 566aa15. Consider uploading reports for the commit 566aa15 to get more accurate results

@@            Coverage Diff             @@
##           master    #1074      +/-   ##
==========================================
+ Coverage   29.45%   31.34%   +1.88%     
==========================================
  Files         277      277              
  Lines        9336     9364      +28     
  Branches     1380     1392      +12     
==========================================
+ Hits         2750     2935     +185     
+ Misses       6487     6332     -155     
+ Partials       99       97       -2

Impacted Files	Coverage Δ
...theia-about/src/browser/about-che-theia-dialog.tsx	`0.00% <ø> (ø)`
...ovisioner/src/node/git-configuration-controller.ts	`0.00% <0.00%> (ø)`
...che-theia-remote-api/src/common/devfile-service.ts	`0.00% <ø> (ø)`
...er/src/node/che-server-certificate-service-impl.ts	`0.00% <0.00%> (ø)`
...-impl-k8s/src/node/k8s-certificate-service-impl.ts	`0.00% <0.00%> (ø)`
...ces/src/node/che-theia-preferences-synchronizer.ts	`0.00% <0.00%> (ø)`
...e-theia-workspace/src/node/che-workspace-server.ts	`0.00% <0.00%> (ø)`
generator/src/init-sources.ts	`98.17% <ø> (ø)`
...ainers-plugin/src/containers-tree-data-provider.ts	`0.00% <0.00%> (ø)`
plugins/task-plugin/src/task/che-task-runner.ts	`0.00% <ø> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b7d633...566aa15. Read the comment docs.

tsmaeder

Haven't tested it, but code looks good.

tsmaeder · 2021-04-12T07:37:00Z

plugins/workspace-plugin/src/workspace-projects-manager.ts

@@ -103,6 +108,15 @@ export class WorkspaceProjectsManager {
    onDidCloneSourcesEmitter.fire();
  }

+  // back compatibility for single-root workspaces
+  // we need it to support workspaces wich were created before switching multi-root mode to ON by default
+  protected ensureWorkspaceFoldersFor(projects: che.devfile.DevfileProject[]): void {


I think this method is misnamed: as I read the code, it filters out projects that do not exist. If I read ensureWorkspaceFolders, I would expect the missing folders to be created. Maybe just inline this method: it's only a single statement.

thank you, will update my PR soon!

tsmaeder · 2021-04-12T07:38:37Z

plugins/workspace-plugin/src/workspace-folder-updater.ts

@@ -13,7 +13,7 @@ export class WorkspaceFolderUpdater {
  private pendingFolders: string[] = [];
  private addingWorkspaceFolderPromise: Promise<void> | undefined;

-  constructor(protected readonly updateWorkspaceFolderTimeout = 3000) {}
+  constructor(protected readonly updateWorkspaceFolderTimeout = 10000) {}


It feels strange that we need 10 seconds here. Do we know where the time goes? Maybe something to investigate some time later. But +1 on the longer timeout.

Within testing my PR on the dogfooding instance I found that sometimes 3 seconds is not enough.
I guess that 5 seconds - is good value for the dogfooding instance, but I'm not sure it will be OK for another instance, for example for minikube that running locally on a user machine with low resources.

So, I can reduce that value to 5-7 seconds within current PR and we could see if there is reported issues about it in further.
WDYT?

At the moment I see that it takes 3-4 seconds on the dogfooding instance for:

using API che-theia from workspace-plugin send a request on theia side to add a project as a workspace folder

theia does some actions and send an event that workspace folders were updated

workspace-plugin catch the event.

I'm going to create an issue to investigate why it takes 3-4 seconds and if we can reduce such delays.

Having a large timeout is fine: most of the time, it will not be reached.

Created the issue for the delay investigation eclipse-che/che#19566

benoitf

I deleted /projects/che-theia.workspace file, stopped my workspace and restarted it.
Project is there

volX4XVHvX.mp4

it's worth adding a unit test

plugins/workspace-plugin/src/workspace-projects-manager.ts

RomanNikitenko · 2021-04-12T08:47:28Z

it's worth adding a unit test

will do it today.

@benoitf @tsmaeder
thank you for the review and testing!

Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>

RomanNikitenko · 2021-04-12T11:40:06Z

@sleshchenko
The happy path tests are broken atm due to other reasons, so I don't expect that they can pass on any PR successfully

azatsarynnyy

I tested it on my local minikube. Project explorer shows the projects structure correctly now. Thanks!

RomanNikitenko · 2021-04-12T15:37:49Z

@sleshchenko
I think the Happy Path tests are run for my current PR from your branch https://github.com/eclipse/che/tree/multiRootBackCompatibility

Do you still need the branch?

sleshchenko · 2021-04-12T16:04:51Z

[crw-ci-test]

azatsarynnyy · 2021-04-12T16:18:53Z

Both Happy Path / happy-path (alpine, next) and Happy Path / happy-path (ubi8, next) are green now.
We can merge it.

che-bot · 2021-04-12T16:24:18Z

✅ E2E Happy path tests succeed 🎉

See Details

name	link
che-theia	quay.io/crw_pr/che-theia:1074
che-theia-endpoint-runtime-binary	quay.io/crw_pr/che-theia-endpoint-runtime-binary:1074

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

Use comment "[crw-ci-test]" to rerun happy path E2E test.
Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

This comment has been minimized.

Sign in to view

RomanNikitenko marked this pull request as ready for review April 12, 2021 06:26

RomanNikitenko requested review from benoitf, ericwill, sunix, svor, tsmaeder, vinokurig and vitaliy-guliy as code owners April 12, 2021 06:26

RomanNikitenko requested a review from azatsarynnyy April 12, 2021 06:26

tsmaeder reviewed Apr 12, 2021

View reviewed changes

benoitf approved these changes Apr 12, 2021

View reviewed changes

plugins/workspace-plugin/src/workspace-projects-manager.ts Outdated Show resolved Hide resolved

plugins/workspace-plugin/src/workspace-projects-manager.ts Outdated Show resolved Hide resolved

RomanNikitenko changed the title ~~Add back compatibility related to switching to multi root mode~~ Add backward compatibility related to switching to multi root mode Apr 12, 2021

This comment has been minimized.

Sign in to view

Add backward compatibility related to switching to multi root mode

566aa15

Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>

RomanNikitenko force-pushed the multiRootBackCompatibility branch from f4941d8 to 566aa15 Compare April 12, 2021 09:33

This comment has been minimized.

Sign in to view

sleshchenko mentioned this pull request Apr 12, 2021

Fix e2e tests after GetStarted -> CreateWorkspace eclipse-che/che#19558

Merged

9 tasks

This comment has been minimized.

Sign in to view

azatsarynnyy approved these changes Apr 12, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

RomanNikitenko merged commit 566529c into eclipse-che:master Apr 12, 2021

RomanNikitenko deleted the multiRootBackCompatibility branch April 12, 2021 18:17

che-bot added this to the 7.29 milestone Apr 12, 2021

RomanNikitenko mentioned this pull request Apr 21, 2021

Che Theia (multi-root) fails to detect projects when they are cloned by another tool eclipse-che/che#19489

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add backward compatibility related to switching to multi root mode #1074

Add backward compatibility related to switching to multi root mode #1074

RomanNikitenko commented Apr 10, 2021 •

edited

Loading

codecov bot commented Apr 10, 2021 •

edited

Loading

This comment has been minimized.

tsmaeder left a comment

tsmaeder Apr 12, 2021

RomanNikitenko Apr 12, 2021

tsmaeder Apr 12, 2021

RomanNikitenko Apr 12, 2021

tsmaeder Apr 12, 2021

RomanNikitenko Apr 12, 2021

benoitf left a comment

This comment has been minimized.

RomanNikitenko commented Apr 12, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

RomanNikitenko commented Apr 12, 2021 •

edited

Loading

This comment has been minimized.

azatsarynnyy left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

RomanNikitenko commented Apr 12, 2021

This comment has been minimized.

sleshchenko commented Apr 12, 2021

azatsarynnyy commented Apr 12, 2021

che-bot commented Apr 12, 2021

Add backward compatibility related to switching to multi root mode #1074

Add backward compatibility related to switching to multi root mode #1074

Conversation

RomanNikitenko commented Apr 10, 2021 • edited Loading

What does this PR do?

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

PR Checklist

Reviewers

Happy Path Channel

codecov bot commented Apr 10, 2021 • edited Loading

Codecov Report

This comment has been minimized.

tsmaeder left a comment

Choose a reason for hiding this comment

tsmaeder Apr 12, 2021

Choose a reason for hiding this comment

RomanNikitenko Apr 12, 2021

Choose a reason for hiding this comment

tsmaeder Apr 12, 2021

Choose a reason for hiding this comment

RomanNikitenko Apr 12, 2021

Choose a reason for hiding this comment

tsmaeder Apr 12, 2021

Choose a reason for hiding this comment

RomanNikitenko Apr 12, 2021

Choose a reason for hiding this comment

benoitf left a comment

Choose a reason for hiding this comment

This comment has been minimized.

RomanNikitenko commented Apr 12, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

RomanNikitenko commented Apr 12, 2021 • edited Loading

This comment has been minimized.

azatsarynnyy left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

RomanNikitenko commented Apr 12, 2021

This comment has been minimized.

sleshchenko commented Apr 12, 2021

azatsarynnyy commented Apr 12, 2021

che-bot commented Apr 12, 2021

RomanNikitenko commented Apr 10, 2021 •

edited

Loading

codecov bot commented Apr 10, 2021 •

edited

Loading

RomanNikitenko commented Apr 12, 2021 •

edited

Loading

RomanNikitenko commented Apr 12, 2021 •

edited

Loading