Allow using the che-plugin-broker as a library in the Workspace CRD controller
#38
Conversation
brokers/theia/broker.go
Outdated
| @@ -130,10 +130,10 @@ func (b *Broker) processPlugin(meta model.PluginMeta) error { | |||
| } | |||
| if pluginImage == "" { | |||
| // regular plugin | |||
| return b.injectTheiaFile(meta, archivePath) | |||
| return b.injectTheiaFile(meta, archivePath, onlyMetadata) | |||
There was a problem hiding this comment.
The purpose of this method is to inject file. It is better to refactor code instead of just adding boolean parameter to it. And with the remote plugin the problem is the same.
There was a problem hiding this comment.
I agree that some refactoring might be useful, but my idea was mainly to change the structure of the code as less as possible, knowing that a number of changes are ongoing or planned. I would have let the refactoring phase for the upcoming changes planned in the context of meta.yaml and plugin yaml merge.
| @@ -53,7 +54,11 @@ func AddExtension(toolingConf *model.ToolingConf, pj model.PackageJSON) { | |||
| sidecarEndpoint := toolingConf.Endpoints[0] | |||
| prettyID := re.ReplaceAllString(pj.Publisher+"_"+pj.Name, `_`) | |||
| sidecarTheiaEnvVarName := "THEIA_PLUGIN_REMOTE_ENDPOINT_" + prettyID | |||
| sidecarTheiaEnvVarValue := "ws://" + sidecarEndpoint.Name + ":" + strconv.Itoa(sidecarEndpoint.TargetPort) | |||
| sidecarHostname := sidecarEndpoint.Name | |||
| if cfg.UseLocalhostInPluginUrls { | |||
There was a problem hiding this comment.
When I was designing this utility it was supposed to not interact with config package. In fact, interacting with the config package is like using global variables.
There was a problem hiding this comment.
That's what it is: a global setting that is taken into account every time we build an endpoint URL. BTW this global setting is only changed when the brokers are used as a library, so no change for your use-case.
There was a problem hiding this comment.
Here I'm more concerned about maintainability of the codebase rather than any particular case
There was a problem hiding this comment.
Maybe this concern could be tackled in the context of one of the future PRs that are still expected in the upcoming refactoring work on the che-plugin-broker ?
I tried to reduce the changes in this PR to the minimum, and in a way that introduce only local non-invasive changes.
And Use the modified `che-plugin-broker` (cf. PR eclipse-che/che-plugin-broker#38) This fixes issues eclipse-che/che#12934 and redhat-developer/rh-che#1315
Signed-off-by: David Festal <dfestal@redhat.com>
Signed-off-by: David Festal <dfestal@redhat.com>
…oker Conflicts: brokers/theia/broker.go brokers/vscode/broker.go
Signed-off-by: David Festal <dfestal@redhat.com>
Signed-off-by: David Festal <dfestal@redhat.com>
|
@amisevsk @ibuziuk @garagatyi @l0rd Could you review the PR again please ? |
What does this PR do?
This PR introduces minimal changes to allow using the
che-plugin-brokeras a library in the Workspace CRD Controller Proof of concept.Added 2 global configuration options to :
/pluginsdirectory (when we are only interested in generating theToolingConfmetadata)localhostinstead of the service name in the endpoint URL of the Theia or VSCode plugins