Load snippets via plugin resource #948

tsmaeder · 2020-12-08T08:56:44Z

Signed-off-by: Thomas Mäder tmader@redhat.com

What does this PR do?

Load snippet resources via the plugin resource loading mechanism instead of file uris (like the are in Theia)

What issues does this PR fix or reference?

How to test this PR?

Use the Che-Theia image built for the happy-path test in any workspace including the Java plugin and try using snippets in any Java file.

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

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

Happy Path Channel

HAPPY_PATH_CHANNEL=next

tsmaeder · 2020-12-09T09:08:06Z

[crw-ci-test --rebuild]

benoitf · 2020-12-09T10:36:49Z

extensions/eclipse-che-theia-plugin-remote/src/browser/plugin-resource.ts

+export class PluginResourceResolver implements ResourceResolver {
+  resolve(uri: URI): MaybePromise<Resource> {
+    if (uri.scheme !== PluginUri.SCHEME) {
+      throw new Error('Not a plugin resource');


Provides the uri that is invalid ?

True. Note that throwing an exception is just the way for the plugin resolver to indicate it cannot handle this scheme. The error is swallowed silently.

benoitf · 2020-12-09T10:37:55Z

extensions/eclipse-che-theia-plugin-remote/src/common/plugin-uri.ts

+ * front end.
+ */
+export namespace PluginUri {
+  export const SCHEME = 'pluginresource';


if it's specific to che, probably add che into the SCHEME (in case upstream provides pluginresource as well) to avoid any mismatch

It's not really specific to che: the plugin resources at /hostedPlugin/... are also available upstream.

it's tied to che because this protocol is not available upstream

benoitf · 2020-12-09T10:46:31Z

extensions/eclipse-che-theia-plugin-remote/src/node/che-plugin-scanner.ts

+      result.push({
+        language: contribution.language,
+        source: pck.displayName || pck.name,
+        uri: PluginUri.createUri(getPluginId(pck), relativePath),


we should just simply make a SnippetContributionProviderURI or SnippetContributionProvider singleton upstream and then just overrides how we create URI downstream
Here there are too many overrides

What do you mean, there are too many overrides? It's a single method. If we extract the various contribution providers upstream, we would logically have to to it for all kinds of contributions. That may or may not be desirable, but one of the reasons we're doing this downstream is that the work was deemed urgent.

I mean here that there are too many classes being extended (and we may miss some of them)
while we're only interested on delegating the contribution uri.

That may or may not be desirable, but one of the reasons we're doing this downstream is that the work was deemed urgent.

eclipse-che/che#16502 is opened since march
I think that adding a new simple method in upstream theia to handle the contribution can be accepted in a couple of days as it's a very minor change.

Also we could just delegate the creation of URI for a plug-in (not tied to snippets) and use for upstream theia File protocol and for che: a custom provider.

As far as I understand it, this issue is not reproducible in upstream theia. In the interest of saving time, I suggest we keep the fix downstream: not only because the benefit for theia is low, but also because the holidays are coming and getting the PR merged on time is not guaranteed. We can always iterate later if the need for such methods in theia arises.

I don't think it would be a problem to have something merge upstream if it's only an helper to get an URI for a plugin resource. Upstream theia having implementation with File and downstream with something else.
Discussing it is taking more time than having proposed a simple tiny PR url upstream.

Also I see two different ways being implemented for getting resources for a plug-in this one and #946 which is kind of colliding.

I asked explicitly on a Theia dev call whether separation of plugin host and back end on different machines was something that should be supported in Theia and the answer was no. Hence the downstream effort instead of pursuing eclipse-theia/theia#8468

And again, we can push for this refactoring upstream any time later, but we already have all the necessary parts to fix the issue.

It's different. It's not to have a plug-in-resource: protocol upstream, just to have something than convert a plug-in path to an URI by using File and that can be replaced in Che (with protocols, etc)

I agree with @benoitf. Here using inheritance and overrides could be quite hard to maintain in the future. It is overriding methods and copying some parts of upstream code. So if, somehow, in upstream, someone changes that line https://github.com/eclipse-theia/theia/blob/bd25d737742a0974b911da5d3a5c978d950fc64e/packages/monaco/src/browser/monaco-snippet-suggest-provider.ts#L146, it would not be reflected downstream and could cause bugs that are hard to track.

benoitf · 2020-12-09T10:47:07Z

extensions/eclipse-che-theia-plugin-remote/src/browser/plugin-resource.ts

+ * The path is resolve relative to the directory designated by the
+ * plugins package path.
+ */
+export class PluginResource implements Resource {


Should be a ChePluginResource

No, it's not Che specific, even if it lives inside che-theia: same reasoning as above. I'm open to proposing a PR upstream with the plugin resource stuff.

if it's becoming an upstream yes it'll be a pluginResource else it'll be a Che plugin resource.
Proposing it upstream will be better if you want to stick to 'pluginResource'

But I think it's only useful upstream if this protocol is used somewhere

tsmaeder · 2020-12-09T11:34:24Z

[crw-ci-test]

che-bot · 2020-12-09T11:46:27Z

❌ E2E Happy path tests failed ❗

See Details

name	link
che-theia	docker.io/maxura/che-theia:948
che-theia-endpoint-runtime-binary	docker.io/maxura/che-theia-endpoint-runtime-binary:948

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.

dmytro-ndp · 2020-12-09T13:46:23Z

[crw-ci-test --rebuild]

che-bot · 2020-12-09T14:20:25Z

❌ E2E Happy path tests failed ❗

See Details

name	link
che-theia	docker.io/maxura/che-theia:948
che-theia-endpoint-runtime-binary	docker.io/maxura/che-theia-endpoint-runtime-binary:948

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.

azatsarynnyy · 2020-12-09T18:46:03Z

[crw-ci-test --rebuild]

che-bot · 2020-12-09T19:21:40Z

✅ E2E Happy path tests succeed 🎉

See Details

name	link
che-theia	docker.io/maxura/che-theia:948
che-theia-endpoint-runtime-binary	docker.io/maxura/che-theia-endpoint-runtime-binary:948

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.

dmytro-ndp · 2020-12-11T13:24:39Z

[crw-ci-test --rebuild]

che-bot · 2020-12-11T13:53:34Z

✅ E2E Happy path tests succeed 🎉

See Details

name	link
che-theia	docker.io/maxura/che-theia:948
che-theia-endpoint-runtime-binary	docker.io/maxura/che-theia-endpoint-runtime-binary:948

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.

tsmaeder · 2021-01-07T16:18:15Z

I've tried using the sidecar file system to load snippet files, but that does not work because monaco tries to fetch the snippet files before the sidecar files system is registered.

benoitf · 2021-01-08T08:19:53Z

@tsmaeder we can't register the filesystem earlier ?

tsmaeder · 2021-01-08T15:47:42Z

@benoitf not easily: the file system is initialized when the plugin manager for a particular front-end is initialized. However, the initialization of connections to plugin hosts is done in parallel to opening the editor, it seems. I believe we would have to rework the lifecycle of plugin host connections. T.b.h, I think it would be beneficial to the project, but might introduce bugs and will delay startup.

tsmaeder · 2021-01-18T09:59:20Z

@benoitf could we move forward with this PR? It works and your suggestion of introducing a SnippetContributionProviderURI will not work until we make the plugin host file system provider work earlier as I described, because the upstream still uses the file system to load the snippets.

benoitf · 2021-01-20T16:52:14Z

will not work until we make the plugin host file system provider work earlier as I described, because the upstream still uses the file system to load the snippets.

as compromise, I'm fine to keep the logic of custom URI on Che-Theia, but not having to override multiple scanners / copy almost all content of upstream readSnippets

tsmaeder · 2021-01-21T08:33:04Z

as compromise, I'm fine to keep the logic of custom URI on Che-Theia
@benoitf What do you mean by "logic of custom URI"? Do you mean using a resource instead of the file service to read the contents of 'snippets.json'? Or do you mean the generation of uri's from extension contributions like 'contributes.snippets'?

benoitf · 2021-01-21T08:44:09Z

@tsmaeder to keep ChePluginResource and CheSnippetSuggestProvider, but remove the need of CheTheiaPluginScanner, CheVsCodePluginScanner by implementing upstream the delegation of readSnippet URI (SnippetContributionProviderURI)

I'm ok to have it merge if during the upcoming sprint, SnippetContributionProviderURI is coming, and icing on the cake, we can remove ChepluginUri/chepluginresource and use an eager registration of filesystem provider

che-bot · 2021-02-12T11:29:37Z

❌ E2E Happy path tests failed ❗

See Details

name	link
che-theia	docker.io/maxura/che-theia:948
che-theia-endpoint-runtime-binary	docker.io/maxura/che-theia-endpoint-runtime-binary:948

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.

che-bot · 2021-02-12T13:22:20Z

✅ E2E Happy path tests succeed 🎉

See Details

name	link
che-theia	docker.io/maxura/che-theia:948
che-theia-endpoint-runtime-binary	docker.io/maxura/che-theia-endpoint-runtime-binary:948

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.

azatsarynnyy

I tested the changes - snippets work well:

che-bot · 2021-02-18T16:16:55Z

❌ E2E Happy path tests failed ❗

See Details

name	link
che-theia	docker.io/maxura/che-theia:948
che-theia-endpoint-runtime-binary	docker.io/maxura/che-theia-endpoint-runtime-binary:948

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.

ericwill · 2021-02-22T20:13:49Z

If we want this in Che 7.26, we'll need to backport this PR and also fork theia at this commit and cherry pick the upstream fix onto our fork.

che-bot · 2021-02-26T09:06:40Z

❌ E2E Happy path tests failed ❗

See Details

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

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.

che-bot · 2021-03-02T12:48:00Z

❌ E2E Happy path tests failed ❗

See Details

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

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.

che-bot · 2021-03-03T10:47:46Z

❌ E2E Happy path tests failed ❗

See Details

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

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.

tsmaeder · 2021-03-10T14:00:26Z

@azatsarynnyy @ericwill have another look, if you please. PR is considerably changed, so you should re-review

che-bot · 2021-03-10T14:39:32Z

✅ E2E Happy path tests succeed 🎉

See Details

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

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.

extensions/eclipse-che-theia-plugin-remote/src/node/plugin-remote-backend-module.ts

extensions/eclipse-che-theia-plugin-remote/package.json

benoitf · 2021-03-10T15:07:14Z

extensions/eclipse-che-theia-plugin-remote/src/node/hosted-plugin-remote.ts

      return;
    }
  }

+  requestPluginResourceStat(pluginId: string, resourcePath: string): Promise<Stat> {


for readability maybe it would be easier to flag it as async method (and use await instead of chaining promises)

I used to like the async/await syntax, but t.b.h these days I find it makes following the control flow rather confusing, in particular when debugging. I tend to only use async/await where it really improves readability. Here it does not, IMO:

che-bot · 2021-03-10T22:46:19Z

✅ E2E Happy path tests succeed 🎉

See Details

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

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.

benoitf · 2021-03-11T07:34:17Z

as it's a bugfix, would be nice to have unit tests

- [ ]:  Tests are covering the bugfix

all modules having now jest setup.

tsmaeder · 2021-03-11T11:20:28Z

@benoitf since this is a bug that rises from the integration of multiple components (containers, even), I don't see an easy way to write a unit test that would be meaningful.
On a more general level, requiring regression tests for every bugfix does not yield sufficient quality gains to justify the expense in development time, IMO. Investing in reducing the complexity or our codebase and better manual testing possibilities would have a better return on investment. (That and making everyone use Che to develop Che).

Signed-off-by: Thomas Mäder <tmader@redhat.com>

codecov · 2021-03-15T16:25:38Z

Codecov Report

Merging #948 (0f3faf1) into master (2628ea7) will decrease coverage by 0.28%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #948      +/-   ##
==========================================
- Coverage   21.07%   20.78%   -0.29%     
==========================================
  Files         298      302       +4     
  Lines       11090    11244     +154     
  Branches     1680     1706      +26     
==========================================
  Hits         2337     2337              
- Misses       8603     8756     +153     
- Partials      150      151       +1

Impacted Files	Coverage Δ
...lugin-remote/src/browser/che-plugin-file-system.ts	`0.00% <0.00%> (ø)`
...emote/src/browser/plugin-remote-frontend-module.ts	`0.00% <0.00%> (ø)`
...e-theia-plugin-remote/src/common/che-plugin-uri.ts	`0.00% <0.00%> (ø)`
...a-plugin-remote/src/node/che-plugin-uri-factory.ts	`0.00% <0.00%> (ø)`
...eia-plugin-remote/src/node/hosted-plugin-remote.ts	`0.00% <0.00%> (ø)`
...-theia-plugin-remote/src/node/internal-protocol.ts	`0.00% <0.00%> (ø)`
...-plugin-remote/src/node/plugin-reader-extension.ts	`0.00% <0.00%> (ø)`
...in-remote/src/node/plugin-remote-backend-module.ts	`0.00% <0.00%> (ø)`
...theia-plugin-remote/src/node/plugin-remote-init.ts	`0.00% <0.00%> (ø)`
...ia-plugin-remote/src/node/plugin-remote-mapping.ts	`0.00% <0.00%> (ø)`
... and 4 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 2628ea7...0f3faf1. Read the comment docs.

che-bot · 2021-03-15T16:29:54Z

✅ E2E Happy path tests succeed 🎉

See Details

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

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.

tsmaeder requested review from azatsarynnyy, benoitf, RomanNikitenko and vzhukovs as code owners December 8, 2020 08:56

benoitf previously requested changes Dec 9, 2020

View reviewed changes

azatsarynnyy mentioned this pull request Dec 9, 2020

Che-Theia build is failed sporadically within the HappyPath test jobs eclipse-che/che#18575

Closed

22 tasks

benoitf mentioned this pull request Dec 10, 2020

Implement simple side car file system which fetches file content #946

Merged

9 tasks

ericwill requested a review from sunix February 3, 2021 15:42

ericwill mentioned this pull request Feb 4, 2021

Plugins Sprint 197 eclipse-che/che#18987

Closed

35 tasks

tsmaeder force-pushed the 16502_fix_snippet_load branch from dd554e0 to 74c25db Compare February 12, 2021 12:51

azatsarynnyy reviewed Feb 12, 2021

View reviewed changes

benoitf self-requested a review February 18, 2021 15:40

tsmaeder force-pushed the 16502_fix_snippet_load branch from 398ccfa to 0f12eb8 Compare March 2, 2021 11:59

benoitf requested a review from sunix March 2, 2021 12:49

tsmaeder force-pushed the 16502_fix_snippet_load branch from 0f12eb8 to ac402f3 Compare March 3, 2021 10:00

benoitf reviewed Mar 10, 2021

View reviewed changes

extensions/eclipse-che-theia-plugin-remote/src/node/plugin-remote-backend-module.ts Outdated Show resolved Hide resolved

benoitf reviewed Mar 10, 2021

View reviewed changes

extensions/eclipse-che-theia-plugin-remote/package.json Outdated Show resolved Hide resolved

benoitf reviewed Mar 10, 2021

View reviewed changes

tsmaeder added 7 commits March 15, 2021 15:51

Load snippets via plugin resource

b597c35

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Resource file system

b0c4a5d

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Intermediate state

5c95c5a

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Make remote and local resource accesses work

fd994ee

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Fix formatting

65db992

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Address PR comments

f68a6b3

Signed-off-by: Thomas Mäder <tmader@redhat.com>

Update yarn.lock

0f3faf1

Signed-off-by: Thomas Mäder <tmader@redhat.com>

tsmaeder force-pushed the 16502_fix_snippet_load branch from a314810 to 0f3faf1 Compare March 15, 2021 15:53

tsmaeder merged commit 552f160 into eclipse-che:master Mar 16, 2021

che-bot added this to the 7.28 milestone Mar 16, 2021

l0rd added the new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes label Mar 23, 2021

Load snippets via plugin resource #948

Load snippets via plugin resource #948

Conversation

tsmaeder commented Dec 8, 2020

What does this PR do?

What issues does this PR fix or reference?

How to test this PR?

PR Checklist

Reviewers

Happy Path Channel

tsmaeder commented Dec 9, 2020

Choose a reason for hiding this comment

tsmaeder Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoitf Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoitf Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunix Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder commented Dec 9, 2020

che-bot commented Dec 9, 2020

dmytro-ndp commented Dec 9, 2020

che-bot commented Dec 9, 2020

azatsarynnyy commented Dec 9, 2020

che-bot commented Dec 9, 2020

dmytro-ndp commented Dec 11, 2020

che-bot commented Dec 11, 2020

tsmaeder commented Jan 7, 2021

benoitf commented Jan 8, 2021

tsmaeder commented Jan 8, 2021

tsmaeder commented Jan 18, 2021

benoitf commented Jan 20, 2021

tsmaeder commented Jan 21, 2021

benoitf commented Jan 21, 2021

che-bot commented Feb 12, 2021

che-bot commented Feb 12, 2021

azatsarynnyy left a comment

Choose a reason for hiding this comment

che-bot commented Feb 18, 2021

ericwill commented Feb 22, 2021

che-bot commented Feb 26, 2021

che-bot commented Mar 2, 2021

che-bot commented Mar 3, 2021

tsmaeder commented Mar 10, 2021

che-bot commented Mar 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

che-bot commented Mar 10, 2021

benoitf commented Mar 11, 2021

tsmaeder commented Mar 11, 2021

codecov bot commented Mar 15, 2021 • edited Loading

Codecov Report

che-bot commented Mar 15, 2021

tsmaeder Dec 9, 2020 •

edited

Loading

benoitf Dec 9, 2020 •

edited

Loading

benoitf Dec 10, 2020 •

edited

Loading

sunix Feb 5, 2021 •

edited

Loading

codecov bot commented Mar 15, 2021 •

edited

Loading