Implement remote plugin runtime injection #71
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,10 +50,11 @@ type PluginMeta struct { | |
| } | ||
|
|
||
| type PluginMetaSpec struct { | ||
| Endpoints []Endpoint `json:"endpoints" yaml:"endpoints"` | ||
| Containers []Container `json:"containers" yaml:"containers"` | ||
| WorkspaceEnv []EnvVar `json:"workspaceEnv" yaml:"workspaceEnv"` | ||
| Extensions []string `json:"extensions" yaml:"extensions"` | ||
| PluginPatcher PluginPatcher `json:"pluginPatcher" yaml:"pluginPatcher"` | ||
| } | ||
|
|
||
| type PluginFQN struct { | ||
|
|
@@ -98,14 +99,34 @@ type Container struct { | |
| Ports []ExposedPort `json:"ports" yaml:"ports"` | ||
| MemoryLimit string `json:"memoryLimit,omitempty" yaml:"memoryLimit,omitempty"` | ||
| MountSources bool `json:"mountSources" yaml:"mountSources"` | ||
|
|
||
| // Base root command inside container | ||
| Command []string `json:"command" yaml:"command"` | ||
|
There was a problem hiding this comment. What is the goal of this addition and how is it managed by the plugin broker ? There was a problem hiding this comment. Command and args defines root process entrypoint container command. We could declare here command, for example, to copy remote binary from container to volume. Currently plugin broker doesn't manage it, this logic was implemented in the che-server... |
||
| // Arguments of the base root command inside container | ||
| Args []string `json:"args" yaml:"args"` | ||
| } | ||
|
|
||
| type ChePlugin struct { | ||
| ID string `json:"id" yaml:"id"` | ||
| Version string `json:"version" yaml:"version"` | ||
| Name string `json:"name" yaml:"name"` | ||
| Type string `json:"type" yaml:"type"` | ||
| Publisher string `json:"publisher" yaml:"publisher"` | ||
| Endpoints []Endpoint `json:"endpoints" yaml:"endpoints"` | ||
| Containers []Container `json:"containers" yaml:"containers"` | ||
| WorkspaceEnv []EnvVar `json:"workspaceEnv" yaml:"workspaceEnv"` | ||
| PluginPatcher PluginPatcher `json:"pluginPatcher" yaml:"pluginPatcher"` | ||
|
There was a problem hiding this comment. Why isn't the plugin patcher managed by the plugin broker directly ? There was a problem hiding this comment.
I believe we try to be as close to K8s API as possible. So, accordingly to K8s API it would be better to introduce new field
I believe that the current implementation brings a consistent picture(I'm not talking about fields naming but about idea in general). Let me explain my thought: pluginPatcher:
pluginTypeMatcher: ["vs code extension", "theia plugin"]
pluginContainerCommand: ["sh", "-c"]
pluginContainerArgs: ["/plugins/remote-launcher/entrypoint.sh"]
initContainers:
- name: theia-remote-plugin-laucher
image: aandrienko/che-theia-endpoint-runtime:next
initContainer: true
command: ['cp']
args: ['-rf', '/remote-plugin-launcher', '/plugins/remote-launcher']
volumes:
- mountPath: "/plugins"
name: pluginsMore see https://github.com/eclipse/che-plugin-registry/pull/210/files In case of adding only just initContainers to meta.yaml - we would have such configuration in different places: Then Dockerfile of And we can easily imagine a situation when vscode and remote plugin need different contianers entrypoints, or even more difficult situation - different entrypoint according to Another case, what if a workspace does not have any vscode or remote plugin, in the current solution init container won't be propagated because of a matcher. The current solution allows solving these issue quite nicely. P.S. Hope I explained my thought clearly and feel free to reask if something is unclear or missing There was a problem hiding this comment. +1 To have pluginPatcher configuration in one place. There was a problem hiding this comment. I'm a bit worried with this approach, since it changes the schema of the plugin Adding the concept of BUT The concept of some logic that should be triggered by the presence of 1 plugin for other plugins seems not straightforward to grasp for the Let's not forget that plugin definitions are in fact a part of a Devfile (definition of the workspace structure). IMO we should keep the Devfile format (as well as the format of the plugin defintions it references) as much declarative and simple as possible. There was a problem hiding this comment.
Totally agree with that. And here
I tried to explain that I prefer I think it makes sense to separate and merge changes we all are agreed:
So, I propose to do the following changes independently from pluginMatcher discussions: change meta.yaml or put all logic into che-plugin-broker. @davidfestal @amisevsk @l0rd WDYT? There was a problem hiding this comment.
Cool idea. +1 To split work part which was agreed like positive changes and move incremental forward. Che server then will be ready for any our solution with plugin-broker. And then rework plugin-broker.
+1 for |
||
| } | ||
|
|
||
| type PluginPatcher struct { | ||
|
There was a problem hiding this comment. Is the whole Is the |
||
| // List init containers | ||
| InitContainers []Container `json:"initContainers" yaml:"initContainers"` | ||
|
|
||
| // List plugin types matched to PluginPatcher | ||
| PluginTypeMatcher []string `json:"pluginTypeMatcher" yaml:"pluginTypeMatcher"` | ||
|
|
||
| // Command to patch original plugin container command | ||
| PluginContainerCommand []string `json:"pluginContainerCommand" yaml:"pluginContainerCommand"` | ||
| // Argument to patch original plugin container arguments | ||
| PluginContainerArgs []string `json:"pluginContainerArgs" yaml:"pluginContainerArgs"` | ||
| } | ||
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.
So I assume adding the type here is only a way to forward the type (defined in the meta.yaml) to the Che server in order to use it in the Che server Kubernetes infrastructure ?