Merge wiptheia dockerfile with Che Theia IDE dockerfile by mmorhun · Pull Request #10148 · eclipse-che/che

mmorhun · 2018-06-22T07:47:30Z

Signed-off-by: Mykola Morhun mmorhun@redhat.com

What does this PR do?

Combines features of witheia docker image with Che one.
Removes unneeded user and sudo from the image.
Drops support for Theia extensions vie env variable. Now thay must be added on docker image build phase.

What issues does this PR fix or reference?

#10124

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

mmorhun · 2018-06-22T07:50:23Z

This PR uses next version of Theia in the image because we don't have released Theia modules with plugin-ext extension. Also patched (for the next version) extensions were used. I added ==TODO==s what should be fixed after the module release.

It is open for discussions.

benoitf · 2018-06-22T07:53:26Z

hello, could you clarify the 'uneeded user' ?
with the changes, this image is working on che.openshift.io ?

AndrienkoAleksandr · 2018-06-22T09:07:56Z

Maybe we should use next with hash commit instead of next version?

mmorhun · 2018-06-22T09:13:09Z

@benoitf Yes. I see no pint in having it in the image. I've just checked it on docker and openshift with machines extension - and it works fine (with next versions of course)

benoitf · 2018-06-22T09:22:38Z

when you say "openshift", is it openshift.io ?

mmorhun · 2018-06-22T09:59:55Z

It is local ocp

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

benoitf · 2018-06-26T09:47:40Z

dockerfiles/theia/Dockerfile

+    rm -rf /tmp/* /var/cache/apk/*
+
+# Packages which will be included into default build of Theia
+ADD theia-default-package.json ${HOME}/package.json


as we're adding all files into ${HOME}
maybe we could use a single ADD instruction for everything that goes into /home/theia
(instead of one for package.json, one for versions.sh, one for add-extensions.js and one for start.sh

we could have folders like

src/home/theia --> /home/theia src/etc/ ---> /etc/

To me it is better to keep it as is because that ADD instructions appears when target file is needed. For example:

ADD src/versions.sh ${HOME}/versions.sh RUN ${HOME}/versions.sh && rm ${HOME}/versions.sh

We add, use and finally remove file. No need to search whole dockerfile and think about context (who else could use or modify or require it).

It adds useless extra lines/layers
if you store files in dedicated folders then you're expecting their location inside the image so reading the Dockerfile is easier. And of course, you don't need to bother where this file has been added.

Also note that the file that you're removing is not also really removed from layers.

mmorhun · 2018-06-26T09:48:58Z

I updated this PR. Tested on docker and openshift.io.
Review again please.

benoitf · 2018-06-26T10:24:43Z

dockerfiles/theia/Dockerfile

+WORKDIR ${HOME}
+ADD supervisord.conf /etc/
+ADD src/start.sh ${HOME}/start.sh
+RUN touch /var/log/supervisord.log && chmod g+rwX /var/log/supervisord.log && chgrp 0 /var/log/supervisord.log


RUN instruction should be appended to the previous

I think it will impact how easy to read this dockerfile is. But ok, I'll move it.

@mmorhun I would say that workspace should start as fast as possible (minimal set of layers, best size as possible)

benoitf · 2018-06-26T10:25:43Z

dockerfiles/theia/Dockerfile

+    # ==TODO== add after generator-theia-plugin release
+    # npm install -g yo @wiptheia/generator-theia-plugin
+# Change permissions to allow editing of files for openshift user
+RUN find ${HOME} -exec sh -c "chgrp 0 {}; chmod g+rwX {}" \;


RUN instruction should be merged with the previous RUN instruction
having two RUN instructions with one changing permission is duplicating the layer for nothing

benoitf

image is almost 1GB, IMHO there are layers that need to be skipped

ghost · 2018-06-26T10:46:57Z

@mmorhun I'd combine all RUN instructions if possible and have one ADD instruction that adds all files and scripts to one directory from which you can then consume them in a Dockerfile

benoitf · 2018-06-26T11:39:06Z

dockerfiles/theia/Dockerfile

+    USE_LOCAL_GIT=true \
+    HOME=/home/theia
+
+RUN apk update && apk add --no-cache make gcc g++ python git openssh bash supervisor jq && \


on a side note, I notice that you're installing openssh but can we really connect through ssh ?

Yeah, good catch. It is result of merge. Will remove it.

benoitf · 2018-06-26T11:51:13Z

on a side note, is it possible to make GITHUB_TOKEN optional ?
it seems it is mandatory to build the docker image while it should only be used if defined

mmorhun · 2018-06-26T11:52:13Z

No, it is not mandatory. One could provide it as a build arg.

benoitf · 2018-06-26T12:00:46Z

@mmorhun
If I don't export GITHUB_TOKEN to a valid token before launching ./build.sh
I got

error An unexpected error occurred: "/home/theia/node_modules/vscode-ripgrep: Command failed.
Exit code: 1
Command: sh
Arguments: -c node ./lib/postinstall.js
Directory: /home/theia/node_modules/vscode-ripgrep
Output:
Downloading ripgrep failed: Error: Request failed with code 401".

(it's like it provides an empty value token)

ghost · 2018-06-26T12:41:17Z

@benoitf this is a known limitation of Theia

benoitf · 2018-06-26T12:43:32Z

@eivantsov I don't see how it's a limitation of theia ?

I can clone theia, build theia and run theia without defining GITHUB_TOKEN
while here, the Dockerfile is defining GITHUB_TOKEN as env var so theia will try to use this empty token if I don't specify it through a build arg.

What I wanted was:

ok I share my network with many ppl building theia, OK I need to add GITHUB_TOKEN
simple case: I try to build the Dockerfile, first guy on network to do that, it should work without grabbing a token from github in order to set GITHUB_TOKEN

ghost · 2018-06-26T12:52:48Z

Yes, Theia will use an empty token and build may be a success unless you hit access rate on Github. At least it worked for me this way.

Everyone had such an issue when building Theia. Once the entire office started building Theia and we quickly hit the limit from one IP.

Btw, do we still need GITHUB_TOKEN after eclipse-theia/theia#2091?

mmorhun · 2018-06-26T13:22:41Z

@eivantsov seems yes. I've tried next version and the problem was there.

ghost · 2018-06-26T13:27:40Z

We may remove GitHub token arg, but this way there are NO guarantees that the image build won't fail in CI.

benoitf · 2018-06-26T13:28:43Z

@eivantsov @mmorhun my problem is not with providing GITHUB_TOKEN which is working fine
it's that I shouldn't be forced to provide it if the network is not hitting rate API limit on github.

ghost · 2018-06-26T13:36:42Z

@benoitf I don't know a good way to know in advance if or not vscode-ripgrep fails that's why GITHUB_TOKEN was added

As the image is built mostly by CI it wasn't a problem.

If you have a better way to build Theia and make one of its components not to download anything from GitHub, we will all appreciate it.

benoitf · 2018-06-26T13:43:29Z

curl https://api.github.com/rate_limit ?

benoitf · 2018-06-26T13:52:10Z

BTW I'm not trying to argue that GITHUB_TOKEN is a bad idea and that it doesn't solve an issue. just that it seems it's forcing ppl to give that parameter even if they don't require it.

with curl https://api.github.com/rate_limit you can check the limit rate and probably you can see if it will fail or not. In that case we could say : Please define GITHUB_TOKEN to make the build OK.

ghost · 2018-06-26T13:53:56Z

Thanks so much. This will of course add complexity to a Dockerfile. An additional script?

benoitf · 2018-06-26T14:00:18Z

yes maybe we can split long RUN instructions to small shell scripts to be more readable.
RUN fork-theia && compile-theia && update-extensions

I'm fine with all solutions as long as I may not be forced to give GITHUB_TOKEN if not required

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

mmorhun · 2018-06-26T14:51:26Z

So, we can split it, but I added comments to the sections and finally it doesn't look huge and unreadable.

benoitf · 2018-06-26T14:57:49Z

dockerfiles/theia/Dockerfile

+    # Add Theia extensions
+    #==TODO== replace with Eclipse official version after Theia release
+    #git clone https://github.com/eclipse/che-theia-hosted-plugin-manager-extension /tmp/hosted-plugin-extension && \
+    git clone https://github.com/mmorhun/che-theia-hosted-plugin-manager-extension /tmp/hosted-plugin-extension && \


what is the difference between mmorhun/che-theia-hosted-plugin-manager-extension and https://github.com/eclipse/che-theia-hosted-plugin-manager-extension ? could it just be a branch of main repo ?

benoitf · 2018-06-26T14:58:27Z

dockerfiles/theia/Dockerfile

+    yarn theia build && \
+    # Install Theia plugin generator
+    #==TODO== add after generator-theia-plugin release
+    #npm install -g yo @wiptheia/generator-theia-plugin && \


you think we shouldn't include the wip package for now ?

I think we need release it first. Do not want to download wiptheia core and stuff.

ah yes I forgot to push changes that use @theia/plugin
let's keep commented as you made 👍

benoitf · 2018-06-26T14:59:26Z

dockerfiles/theia/Dockerfile

+    # Grant permissions for modifying supervisor log file
+    touch /var/log/supervisord.log && chmod g+rwX /var/log/supervisord.log && chgrp 0 /var/log/supervisord.log

+WORKDIR ${HOME}


(not important) could we move WORKDIR at the top of the file before RUN instruction so we can reuse that layer every time

benoitf · 2018-06-26T15:01:50Z

dockerfiles/theia/Dockerfile

-ARG THEIA_VERSION=0.3.10
-ENV USE_LOCAL_GIT=true \
-    GITHUB_TOKEN=${GITHUB_TOKEN} \
+ARG THEIA_VERSION=0.4.0-next.b17727c1


why using a specific version of next instead of next ?
we may want to include all upstream theia fixes on nightlies

So you want to use next?

I do not mind

Just to be sure we have working image (I tested it on both docker and oso infras)

It will be confused when we move back to the strict Theia version, but dockerhub save "next" tag. @benoitf What do you think? Somebody can try to use it, but it won't be next any more. And if we have next tag we should update it every night.

@AndrienkoAleksandr I would think to have a real "nightly" based on "next" and a fixed version as suggested by @mmorhun

benoitf

we can merge and iterate later on the changes

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

benoitf · 2018-06-27T05:48:45Z

FYI I created #10205 for improvements

mmorhun · 2018-06-27T06:14:18Z

Thanks

…10148) * CHE-10124: Merge wiptheia dockerfile with Che Theia IDE dockerfile Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

CHE-10124: Merge wiptheia dockerfile with Che Theia IDE dockerfile

90e320a

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

mmorhun requested review from a user, AndrienkoAleksandr, benoitf, evidolob and riuvshin June 22, 2018 07:47

mmorhun self-assigned this Jun 22, 2018

benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Jun 22, 2018

Remove Theia extensions support via env variable

122ca79

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

benoitf reviewed Jun 26, 2018

View reviewed changes

AndrienkoAleksandr approved these changes Jun 26, 2018

View reviewed changes

evidolob approved these changes Jun 26, 2018

View reviewed changes

ghost approved these changes Jun 26, 2018

View reviewed changes

benoitf reviewed Jun 26, 2018

View reviewed changes

benoitf requested changes Jun 26, 2018

View reviewed changes

benoitf reviewed Jun 26, 2018

View reviewed changes

Merge RUN commands in dockerfile. Use new image in the predefined stacks

ba8a7b7

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

mmorhun requested a review from ashumilova as a code owner June 26, 2018 14:36

benoitf reviewed Jun 26, 2018

View reviewed changes

benoitf approved these changes Jun 26, 2018

View reviewed changes

Code cleanup

ad34273

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

mmorhun merged commit 44681e8 into master Jun 26, 2018

mmorhun deleted the CHE-10124 branch June 26, 2018 15:58

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jun 26, 2018

benoitf added this to the 6.8.0 milestone Jun 26, 2018

Conversation

mmorhun commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

What issues does this PR fix or reference?

Uh oh!

mmorhun commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benoitf commented Jun 22, 2018

Uh oh!

AndrienkoAleksandr commented Jun 22, 2018

Uh oh!

mmorhun commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benoitf commented Jun 22, 2018

Uh oh!

mmorhun commented Jun 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmorhun commented Jun 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benoitf left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benoitf commented Jun 26, 2018

Uh oh!

mmorhun commented Jun 26, 2018

Uh oh!

benoitf commented Jun 26, 2018

Uh oh!

ghost commented Jun 26, 2018

Uh oh!

benoitf commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 26, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmorhun commented Jun 26, 2018

Uh oh!

ghost commented Jun 26, 2018

Uh oh!

benoitf commented Jun 26, 2018

Uh oh!

ghost commented Jun 26, 2018

Uh oh!

benoitf commented Jun 26, 2018

Uh oh!

benoitf commented Jun 26, 2018

Uh oh!

ghost commented Jun 26, 2018

Uh oh!

benoitf commented Jun 26, 2018

Uh oh!

mmorhun commented Jun 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mmorhun commented Jun 22, 2018 •

edited

Loading

mmorhun commented Jun 22, 2018 •

edited

Loading

mmorhun commented Jun 22, 2018 •

edited

Loading

benoitf commented Jun 26, 2018 •

edited

Loading

ghost commented Jun 26, 2018 •

edited by ghost

Loading

mmorhun Jun 26, 2018 •

edited

Loading

AndrienkoAleksandr Jun 26, 2018 •

edited

Loading