Compatibility the new automatic multicast discovery and plugin file isolation #60

davidfestal · 2019-05-13T13:07:51Z

What does this PR do?

This PR Implements the compatibility the new automatic multicast discovery and the isolation of plugin files (work done on che-theia by @benoitf in PR eclipse-che/che-theia#91

What issues does this PR fix or reference?

This fixes issue redhat-developer/rh-che#1354 (also described in eclipse-che/che#13272 and eclipse-che/che#12395 (comment)

This also includes a fix for issue eclipse-che/che#13349

Was this PR tested ?

Yes, with upstream Che nightly docker image running on Minishift.
Still want to test the che-in-che flow (Development and test of remote Theia plugins under Che-Theia)

amisevsk

Looks mostly good on a first readthrough; I'll test more directly tomorrow.

Did you mean to make this PR into a feature branch?

amisevsk · 2019-05-13T14:13:16Z

brokers/unified/vscode/sidecar.go

-	port := endpoint.TargetPort
-	container.Ports = append(container.Ports, model.ExposedPort{ExposedPort: port})
+	if (! useLocalhost) {
+  	endpoint := generateTheiaSidecarEndpoint(rand)


Indentation issues, please make sure all indentation is done via tabs.

codecov · 2019-05-14T17:05:15Z

Codecov Report

Merging #60 into master will increase coverage by 2.95%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   62.52%   65.47%   +2.95%     
==========================================
  Files           6        6              
  Lines         555      559       +4     
==========================================
+ Hits          347      366      +19     
+ Misses        182      168      -14     
+ Partials       26       25       -1

Impacted Files	Coverage Δ
brokers/unified/vscode/sidecar.go	`100% <100%> (ø)`	⬆️
utils/ioutil.go	`28.66% <86.66%> (+19.1%)`	⬆️
brokers/unified/vscode/broker.go	`73.93% <86.95%> (-0.83%)`	⬇️

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 23a7e2d...cada295. Read the comment docs.

amisevsk

Thanks David

amisevsk

Please set target branch to master; the target is a currently merged branch.

amisevsk · 2019-05-17T12:55:11Z

brokers/unified/vscode/broker.go

@@ -245,8 +245,9 @@ func extensionOrURL(extensionOrURL string) (extension string, URL string) {
 	}
 }

-func (b *brokerImpl) generatePluginArchiveName(plugin model.ChePlugin) string {
-	return fmt.Sprintf("%s.%s.%s.%s", plugin.Publisher, plugin.Name, plugin.Version, b.rand.String(10))
+func (b *brokerImpl) generatePluginArchiveName(plugin model.ChePlugin, archivePath string) string {


This likely won't work on current master; archivePath is generated and no longer resolves filename, and resolving filename from URL won't work for many URLs.

Ah nevermind I see this commit also modifies how archivePath is generated.

I worry that depending on URLs ending in the filename will cause problems -- e.g. https://marketplace.visualstudio.com/_apis/public/gallery/publishers/vscjava/vsextensions/vscode-java-debug/0.16.0/vspackage is a valid URL.

This is fixed with commit ce4c699

amisevsk

LGTM, pending switching base branch to master

amisevsk · 2019-05-17T17:21:50Z

utils/ioutil.go

+			contentDispoFilename = path.Base(path.Clean("/" + contentDispoFilename))
+			if contentDispoFilename == "." || contentDispoFilename == "/" {
+				contentDispoFilename = ""
+			}


Could you add a few comments about why the steps above are necessary? Having multiple cases where contentDispoFilename is reset to the empty string is a little confusing. Some examples of cases being addressed would be useful.

I refactored a bit the code to make it clearer in commit 103113f

Can you tell if you think a comment is still necessary ?

@amisevsk

after @amisevsk comment: #60 (comment) Signed-off-by: David Festal <dfestal@redhat.com>

Signed-off-by: David Festal <dfestal@redhat.com>

... and use it to define the filename when downloading the Theia or VsCode extension archives Signed-off-by: David Festal <dfestal@redhat.com>

Signed-off-by: David Festal <dfestal@redhat.com>

@amisevsk

after @amisevsk comment: #60 (comment) Signed-off-by: David Festal <dfestal@redhat.com>

This reverts commit ae4dc5b.

ibuziuk

LGTM

davidfestal · 2019-05-20T14:35:41Z

@ibuziuk @amisevsk @benoitf I updated the ReadMe accordingly. Do you want to have a look before we merge ?

davidfestal requested a review from amisevsk as a code owner May 13, 2019 13:07

davidfestal requested a review from benoitf May 13, 2019 13:08

davidfestal self-assigned this May 13, 2019

davidfestal requested a review from ibuziuk May 13, 2019 13:08

amisevsk reviewed May 13, 2019

View reviewed changes

amisevsk approved these changes May 16, 2019

View reviewed changes

davidfestal changed the title ~~[WIP] Compatibility the new automatic multicast discovery and plugin file isolation~~ Compatibility the new automatic multicast discovery and plugin file isolation May 17, 2019

amisevsk suggested changes May 17, 2019

View reviewed changes

amisevsk reviewed May 17, 2019

View reviewed changes

amisevsk mentioned this pull request May 17, 2019

Make broker preserve extensions for downloaded plugins #61

Closed

davidfestal requested a review from amisevsk May 17, 2019 16:33

amisevsk approved these changes May 17, 2019

View reviewed changes

davidfestal changed the base branch from fix-che-11018 to master May 17, 2019 19:03

davidfestal added a commit that referenced this pull request May 20, 2019

Refactor a bit the code to make clearer

103113f

after @amisevsk comment: #60 (comment) Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal added 14 commits May 20, 2019 10:55

Add vscode launch configuration

ae4dc5b

Signed-off-by: David Festal <dfestal@redhat.com>

isolate plugin files and drop endpoints

102d8fe

Signed-off-by: David Festal <dfestal@redhat.com>

Only drop endpoints when on using localhost

48c1d10

Signed-off-by: David Festal <dfestal@redhat.com>

add tests

13f2264

Signed-off-by: David Festal <dfestal@redhat.com>

clean both /plugins and /sidecar-plugins

587e0fe

Signed-off-by: David Festal <dfestal@redhat.com>

clean code

5e1adb5

Signed-off-by: David Festal <dfestal@redhat.com>

Add the THEIA_PLUGINS container env var

998af28

Signed-off-by: David Festal <dfestal@redhat.com>

Fix tests

2cbed49

Signed-off-by: David Festal <dfestal@redhat.com>

Use only one volume

c4f0cbb

Signed-off-by: David Festal <dfestal@redhat.com>

copy extension archives with their real file name

0fa212f

Signed-off-by: David Festal <dfestal@redhat.com>

Honor the Content-Disposition header if any ...

5103797

... and use it to define the filename when downloading the Theia or VsCode extension archives Signed-off-by: David Festal <dfestal@redhat.com>

Add tests for the Download method

1e549d0

Signed-off-by: David Festal <dfestal@redhat.com>

Refactor a bit the code to make clearer

22d14a4

after @amisevsk comment: #60 (comment) Signed-off-by: David Festal <dfestal@redhat.com>

Revert "Add vscode launch configuration"

bc602f6

This reverts commit ae4dc5b.

davidfestal force-pushed the fix-rhche-1354 branch from 103113f to bc602f6 Compare May 20, 2019 09:43

ibuziuk approved these changes May 20, 2019

View reviewed changes

Update the Readme according to the last changes

cada295

benoitf approved these changes May 20, 2019

View reviewed changes

davidfestal merged commit ff656fa into master May 20, 2019

nickboldt deleted the fix-rhche-1354 branch June 29, 2021 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compatibility the new automatic multicast discovery and plugin file isolation #60

Compatibility the new automatic multicast discovery and plugin file isolation #60

davidfestal commented May 13, 2019 •

edited

Loading

amisevsk left a comment

amisevsk May 13, 2019

codecov bot commented May 14, 2019 •

edited

Loading

amisevsk left a comment

amisevsk left a comment

amisevsk May 17, 2019

amisevsk May 17, 2019

davidfestal May 17, 2019

amisevsk left a comment

amisevsk May 17, 2019

davidfestal May 20, 2019

ibuziuk left a comment

davidfestal commented May 20, 2019

Compatibility the new automatic multicast discovery and plugin file isolation #60

Compatibility the new automatic multicast discovery and plugin file isolation #60

Conversation

davidfestal commented May 13, 2019 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Was this PR tested ?

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk May 13, 2019

Choose a reason for hiding this comment

codecov bot commented May 14, 2019 • edited Loading

Codecov Report

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk May 17, 2019

Choose a reason for hiding this comment

amisevsk May 17, 2019

Choose a reason for hiding this comment

davidfestal May 17, 2019

Choose a reason for hiding this comment

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk May 17, 2019

Choose a reason for hiding this comment

davidfestal May 20, 2019

Choose a reason for hiding this comment

ibuziuk left a comment

Choose a reason for hiding this comment

davidfestal commented May 20, 2019

davidfestal commented May 13, 2019 •

edited

Loading

codecov bot commented May 14, 2019 •

edited

Loading