Skip to content
This repository has been archived by the owner on Jan 20, 2023. It is now read-only.

Start remote binary with override entry point #75

Merged
merged 10 commits into from
Oct 9, 2019

Conversation

AndrienkoAleksandr
Copy link
Contributor

@AndrienkoAleksandr AndrienkoAleksandr commented Sep 29, 2019

What does this PR do?

At this pr I changed a bit model: applied Type field to the model.ChePlugin

Inject remote endpoint binary with override entrypoint sidecar containers. For this purpose we are using init container from che-theia editor-plugin:

initContainers:
  - name: remote-runtime-injector
    image: aandrienko/che-theia-endpoint-runtime-binary:next
    volumes:
      - mountPath: "/remote-endpoint"
        name: remote-endpoint
        ephemeral: true
    env:
      - name: PLUGIN_REMOTE_ENDPOINT_EXECUTABLE
        value: /remote-endpoint/plugin-remote-endpoint
      - name: REMOTE_ENDPOINT_VOLUME_NAME
        value: remote-endpoint

Plugin broker should find this init container by name remote-runtime-injector and only for che-theia editor plugin. Then plugin-broker should analize env variables and volume from init container.

REMOTE_ENDPOINT_VOLUME_NAME env we are using to find one volume which should be shared with other sidecar containers. In this volume init container on start should copy remote binary

PLUGIN_REMOTE_ENDPOINT_EXECUTABLE env will be shared with all sidecar containers.

Also che-plugin-broker override entypoint for all sidecar containers to start remote plugin. But if sidecar container has in the meta.yaml already defined entrypoint che-plugin broker don't override it. It's mean that plugin writer could define do something and then start remote plugin(using env PLUGIN_REMOTE_ENDPOINT_EXECUTABLE).

What issues does this PR fix or reference?

eclipse-che/che#13387

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@codecov
Copy link

codecov bot commented Sep 29, 2019

Codecov Report

Merging #75 into master will increase coverage by 0.29%.
The diff coverage is 73.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
+ Coverage   67.17%   67.46%   +0.29%     
==========================================
  Files           6        7       +1     
  Lines         591      664      +73     
==========================================
+ Hits          397      448      +51     
- Misses        168      181      +13     
- Partials       26       35       +9
Impacted Files Coverage Δ
brokers/unified/vscode/broker.go 74.21% <100%> (+0.13%) ⬆️
brokers/unified/vscode/sidecar.go 100% <100%> (ø) ⬆️
brokers/unified/broker.go 80.82% <62.5%> (-2.42%) ⬇️
brokers/unified/remote_runtime_injector.go 75.38% <75.38%> (ø)

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 8b3c7a9...fc814c1. Read the comment docs.

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this PR vs #76, this one should be closed, right?

@AndrienkoAleksandr
Copy link
Contributor Author

Looking at this PR vs #76, this one should be closed, right?

Both PRs #76 and #75 works fine, but yes one of them should be closed after discussion.

@AndrienkoAleksandr
Copy link
Contributor Author

Description updated

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@AndrienkoAleksandr
Copy link
Contributor Author

AndrienkoAleksandr commented Oct 3, 2019

@amisevsk @davidfestal @sleshchenko @l0rd pr updated and ready for review.

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM code-wise, but I think we should have test cases for remote runtime injection.

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


InjectorContainerName = "remote-runtime-injector"

RemoteEndPointExecutableEnvVar = "PLUGIN_REMOTE_ENDPOINT_EXECUTABLE"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EndPoint -> Endpoint

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@AndrienkoAleksandr AndrienkoAleksandr merged commit 44c285a into master Oct 9, 2019
@AndrienkoAleksandr AndrienkoAleksandr deleted the startRemoteBinaryWithOverrideEntryPoint branch October 9, 2019 12:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants