Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Theia docker file with launcher script #8732

Merged
merged 11 commits into from
Mar 5, 2018
Merged

Theia docker file with launcher script #8732

merged 11 commits into from
Mar 5, 2018

Conversation

evidolob
Copy link
Contributor

What does this PR do?

add Docker file with Theia and launcher script that can rebuild Theia with provided set of the plugin.

What issues does this PR fix or reference?

#8701

Release Notes

Docs PR

evidolob and others added 4 commits February 12, 2018 12:58
Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
…ible to avoid building it if no plugins are requested from user
Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
@evidolob evidolob self-assigned this Feb 12, 2018
@benoitf benoitf added kind/task Internal things, technical debt, and to-do tasks to be performed. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Feb 12, 2018
@@ -0,0 +1,20 @@
FROM node:8-alpine
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add Copyright headers .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

WORKDIR /home/theia
# build Theia with all extensions to persist yarn cache in the image and
# have default Theia build in the workspace in case no plugins are requested
ADD theia-full-package.json /home/theia/package.json
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like there is a fork of upstream package.json, how to be in sync with upstream changes automatically ?

Copy link

Choose a reason for hiding this comment

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

I think we can have two options here:

  1. curl original Theia package.json as a Dockerfile instruction
  2. Rely on our CI to do this after a triggered webhook.

Copy link
Contributor

Choose a reason for hiding this comment

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

having the github URL of theia package.json will allow to only re-run the RUN instruction only if the remote side is updated

Copy link
Contributor

Choose a reason for hiding this comment

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

(so we can reuse layer cache if there is no remote change)

Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
"devDependencies": {
"@theia/cli": "latest"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please add missing eof line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
EXPOSE 3000
ENV USE_LOCAL_GIT=true \
GITHUB_TOKEN=957257352ad57b7853c8b8714d2201bf4d337b5e
# ENV THEIA_PLUGINS=[\"@theia\/extension-manager\",\"\@theia\/python\"]
Copy link
Contributor

Choose a reason for hiding this comment

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

should we remove this comment ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we leave it for testing purpose, but we also can use -e THEIA_PLUGINS=... for this, so I remove it

Copy link
Contributor

Choose a reason for hiding this comment

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

@evidolob you may add a README or in the header of Dockerfile on how to use that image

Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
yarn theia build
EXPOSE 3000
ENV USE_LOCAL_GIT=true \
GITHUB_TOKEN=957257352ad57b7853c8b8714d2201bf4d337b5e
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think that this is a good idea to put someone's token in dockerfile, I think it user should pass it's own GH token if that is necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link

@ghost ghost Feb 13, 2018

Choose a reason for hiding this comment

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

Sure it's not good. But at this point we don't have any way to get user's token

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

@l0rd how does that help? The scenario is as follows:

  1. Theia image starts, we have a running container
  2. A node script checks for Theia plugins env and once found runs yarn and yarn theia build against a generated or existing package.json. It is during this build that a token is required.

Maybe I don't understand smth about env replacement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@riuvshin riuvshin Feb 13, 2018

Choose a reason for hiding this comment

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

@eivantsov we don't need to get token, user should pass it
if you need token in runtime just pass it with docker run -e GITHUB_TOKEN=, if you need token for docker build you can do

+ARG GITHUB_TOKEN
+ENV GITHUB_TOKEN ${GITHUB_TOKEN}

docker build --build-arg GITHUB_TOKEN=<token>

Copy link

Choose a reason for hiding this comment

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

A user will just launch a workspace. It is us who do docker run, Che server I mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh, I thought it is image for CI, because all images we have in dockerfiles/ are for CLI and they are supposed to be built on CI. if that is image for WS maybe it should be moved to github.com/eclipse/che-dockerfiles ?

@benoitf
Copy link
Contributor

benoitf commented Feb 13, 2018

based on demo, could it be possible to give

-e THEIA_PLUGINS="@theia/extension-manager,@theia/python"

instead of

-e THEIA_PLUGINS=[\"@theia\/extension-manager\",\"\@theia\/python\"]

@ghost
Copy link

ghost commented Feb 13, 2018

@benoitf it was just easier to parse it :) Format and delivery method for plugins list is to be further discussed.

@benoitf
Copy link
Contributor

benoitf commented Feb 13, 2018

@eivantsov but humans are not good parsers :-)

@ghost
Copy link

ghost commented Feb 13, 2018

Easier for node script. So, we agreed to start with ["",""]

@evidolob
Copy link
Contributor Author

@benoitf We never consider that user will manually edit that property, we should get good UI in dashboard for that

@benoitf
Copy link
Contributor

benoitf commented Feb 13, 2018

let myPlugins="@theia/extension-manager,@theia/python"
var array = myPlugins.split(',');
var jsonString = JSON.stringify(array);
console.log('json array is', jsonString);

==> json array is ["@theia/extension-manager","@theia/python"]


Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
@evidolob
Copy link
Contributor Author

Now plugin list should provided as @benoitf proposed.
Example: -e THEIA_PLUGINS=@theia/python,@theia/go

@ghost
Copy link

ghost commented Feb 14, 2018

Even though this is a stack image, we will probably need it to be versioned and updated on a regular basis.

@l0rd
Copy link
Contributor

l0rd commented Feb 26, 2018

By the way @eivantsov it would be nice if all Che runtime stacks images could be versioned too. But that's another story :-)

@evidolob evidolob merged commit a5494dd into master Mar 5, 2018
@evidolob evidolob deleted the theia-launcher branch March 5, 2018 07:48
@benoitf 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 Mar 5, 2018
@benoitf benoitf added this to the 6.2.0 milestone Mar 5, 2018
hkolvenbach pushed a commit to hkolvenbach/che that referenced this pull request Mar 12, 2018
* eclipse-che#8701 create launcher for theia

Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants