Skip to content

Make theia Docker image be able to use yarn cache on Openshift#10116

Merged
mmorhun merged 1 commit intomasterfrom
CHE-9978
Jun 21, 2018
Merged

Make theia Docker image be able to use yarn cache on Openshift#10116
mmorhun merged 1 commit intomasterfrom
CHE-9978

Conversation

@mmorhun
Copy link
Copy Markdown
Contributor

@mmorhun mmorhun commented Jun 20, 2018

What does this PR do?

Adds settings and corresponding permissions into docker image with Theia to be able to use yarn cache on openshift

What issues does this PR fix or reference?

#9978

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
cd /home/default/theia && \
yarn && \
yarn theia build
RUN find ${YARN_CACHE_FOLDER} -exec sh -c "chgrp 0 {}; chmod g+rwX {}" \;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we group all RUN instructions together ?

Copy link
Copy Markdown
Contributor Author

@mmorhun mmorhun Jun 20, 2018

Choose a reason for hiding this comment

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

Do not think so. It could be helpful if we add something and then delete it at the end of RUN. But in this case it still remains we cannot reduce image size. What we will have - a single layer with all the changes, which is not so good, because it makes using caches harder.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

well I still think using one layer will be better as BTW each time package.json is changed it will update yarn cache and then it will be invalidated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But it will happen anyway and doesn't depend on number of layers...

@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/bug Outline of a bug - must adhere to the bug report template. labels Jun 20, 2018
@benoitf
Copy link
Copy Markdown
Contributor

benoitf commented Jun 20, 2018

I think it doesn't solve issue on write access on some folder
some of fixes seem not be backported from https://github.com/theia-demo-plugins/wiptheia-docker-image/blob/master/Dockerfile

@mmorhun
Copy link
Copy Markdown
Contributor Author

mmorhun commented Jun 20, 2018

@benoitf it is not backport of https://github.com/theia-demo-plugins/wiptheia-docker-image/blob/master/Dockerfile. This PR solves problems with caches on openshift.

@benoitf
Copy link
Copy Markdown
Contributor

benoitf commented Jun 20, 2018

@mmorhun I mean that it solves one issue but other issues on openshift remain

@benoitf
Copy link
Copy Markdown
Contributor

benoitf commented Jun 20, 2018

also now that plugin's model has been merged this image will be replaced by the wip docker image.

@mmorhun
Copy link
Copy Markdown
Contributor Author

mmorhun commented Jun 20, 2018

We shouldn't replace this dockerfile with wiptheia one. Otherwise we'll lose fix for caches problem. I think we should merge them but I would like to do it in a separate PR.

@benoitf
Copy link
Copy Markdown
Contributor

benoitf commented Jun 20, 2018

@mmorhun yes when I said "replaced" it was more "merge"
ok in a separate PR

@benoitf
Copy link
Copy Markdown
Contributor

benoitf commented Jun 20, 2018

but my meaning was more that we could have fixed wip dockerimage and then drop this one and use the new one. As this image is more broken than the wip-dockerimage

@mmorhun
Copy link
Copy Markdown
Contributor Author

mmorhun commented Jun 20, 2018

Yes, I agree that we have better image in wiptheia than here in Che and it definitely should be considered.

Copy link
Copy Markdown
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

would be better by merging RUN but can be reworked with the upcoming merge

Copy link
Copy Markdown

@garagatyi garagatyi left a comment

Choose a reason for hiding this comment

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

Looks good!

@mmorhun mmorhun merged commit 93f495f into master Jun 21, 2018
@mmorhun mmorhun deleted the CHE-9978 branch June 21, 2018 06:41
@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 Jun 21, 2018
@benoitf benoitf added this to the 6.8.0 milestone Jun 21, 2018
hbhargav pushed a commit to hbhargav/che that referenced this pull request Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Outline of a bug - must adhere to the bug report template.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants