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

Add dockerfile to execute E2E Che 7 typescript tests. #13542

Merged
merged 4 commits into from
Jul 2, 2019

Conversation

Katka92
Copy link
Contributor

@Katka92 Katka92 commented Jun 14, 2019

What does this PR do?

Add Docker image with pre-installed dependencies of E2E tests. This image downloads all dependencies needed to run tests and runs npm i to install all dependencies to node_modules of a project.
This PR includes README.md with description of available possibilities how to run this Docker image.
If you want to try this image, you can build it on your own or use my public image deployed on quay: quay.io/kkanova/upstream_e2e.

@Katka92 Katka92 requested a review from benoitf as a code owner June 14, 2019 07:06
@che-bot
Copy link
Contributor

che-bot commented Jun 14, 2019

Can one of the admins verify this patch?

@musienko-maxim
Copy link
Contributor

Can one of the admins verify this PR?

@Ohrimenko1988
Copy link
Contributor

Great job, just some suggestions, maybe better to put dockerfile into E2E folder? And the second one - we use particular version of chromedriver, and it doesn't assume often changes, so maybe better hardcode particulat version oh chrome which compatible with chromedriver and remove this logic from dockerfile?

@rhopp
Copy link
Contributor

rhopp commented Jun 14, 2019

@Ohrimenko1988 What do you mean we use particular version of chromedriver? You have to have chromedriver compatible with the chrome browser you are using, right? And I think we want to use latest stable version of browser, and thus the logic is IMHO needed there.

And as for the place of this dockerfile. Maybe it would be worth investigating how are other images in dockerfiles folder being built and make sure our image is built as part of this process. This way we wouldn't have to create our own build/publish job.

@benoitf
Copy link
Contributor

benoitf commented Jun 14, 2019

hello, should this Dockerfile be part of che repository, AFAIK there is nothing related to che inside the docker image

@Katka92
Copy link
Contributor Author

Katka92 commented Jun 14, 2019

@benoitf Hi, thank you for your comment. Typescript E2E tests are part of eclipse/che repo, so I though it is good to have code + image together. Where would you recommend to place that Dockerfile?

@benoitf
Copy link
Contributor

benoitf commented Jun 14, 2019

we have also https://github.com/eclipse/che-dockerfiles repository but up to you, if you think it's the same lifecycle. It's just that I don't see any related che binaries inside the docker image, so that I'm thinking it's used by che but it's not so tied to it.

@rhopp
Copy link
Contributor

rhopp commented Jun 14, 2019

@benoitf che-dockerfiles repo seems to contain only dockerfiles for stacks images, thus it doesn't seem like the best place to put this dockerfile.

@benoitf
Copy link
Contributor

benoitf commented Jun 14, 2019

@rhopp it could be incubated there https://github.com/che-incubator/

@dmytro-ndp
Copy link
Contributor

dmytro-ndp commented Jun 14, 2019

@benoitf: this PR is about adding tools, which are required to run Eclipse Che E2E Happy Path Tests, into the directory dedicated to E2E tests. Those tools are related to Che as well as Eclipse Che E2E Happy Path Tests related to Eclipse Che.

@benoitf
Copy link
Contributor

benoitf commented Jun 14, 2019

ok reading more the Dockerfile, it seems there is a clone of che inside
I'm then -1 to have this inside the proposed dockerfile (we may not have the right branch, good commits, etc)

@Ohrimenko1988 Ohrimenko1988 self-requested a review June 14, 2019 09:27
Copy link
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.

it seems that this Dockerfile is related to the files that are inside https://github.com/eclipse/che/tree/master/e2e but it's cloning the files while you're aiming to be in the same repository

AFAIK you should mimic https://github.com/eclipse/che/blob/master/dockerfiles/che/build.sh#L15-L31

so, copy the local e2e files to be in the scope of Docker build context, then run pure COPY instructions (and not use clone)

or use what @Ohrimenko1988 is proposing: to have the file inside the e2e folder (like it's done for dashboard: https://github.com/eclipse/che/blob/master/dashboard/Dockerfile)

@rhopp
Copy link
Contributor

rhopp commented Jun 14, 2019

so, copy the local e2e files to be in the scope of Docker build context, then run pure COPY instructions (and not use clone)

Agree. @Katka92 could you take a look at this approach?

@benoitf
Copy link
Contributor

benoitf commented Jun 14, 2019

@rhopp yes. Else we have inconsistency. Besides there was no invalidate cache instruction so for example it won't do git clone each time even if master is changed a lot.
Also, when developing or want to evolve this Dockerfile, you want to change your local e2e package.json, not that it pulls from master

@Katka92
Copy link
Contributor Author

Katka92 commented Jun 21, 2019

@rhopp @benoitf Could you please review if I made correct changes? Thank you

@benoitf benoitf dismissed their stale review June 21, 2019 04:58

there were some changes


COPY google-chrome.repo /etc/yum.repos.d/google-chrome.repo
COPY e2e /root/e2e
RUN yum install --assumeyes epel-release && \
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move all yum commands before the copy of files to keep Docker layers the same

Then copy only package*.json and run npm install and then copy all other e2e files

cd e2e
fi

npm run test
Copy link
Contributor

Choose a reason for hiding this comment

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

also please fixe the missing end of files

@che-bot che-bot added kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Jun 27, 2019
WORKDIR /root/
COPY docker-entrypoint.sh /root/

ENTRYPOINT ["/root/docker-entrypoint.sh"]
Copy link
Member

Choose a reason for hiding this comment

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

no new line in the end

Signed-off-by: kkanova <kkanova@redhat.com>
Signed-off-by: kkanova <kkanova@redhat.com>
Signed-off-by: kkanova <kkanova@redhat.com>
@Katka92
Copy link
Contributor Author

Katka92 commented Jul 2, 2019

@benoitf I tried to address your ideas - could you verify please?

Copy link
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.

@Katka92 yes thanks 👍
there are only few comments

baseurl=http://dl.google.com/linux/chrome/rpm/stable/$basearch
enabled=1
gpgcheck=1
gpgkey=https://dl-ssl.google.com/linux/linux_signing_key.pub
Copy link
Contributor

Choose a reason for hiding this comment

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

minor thing: missing end of file return line

@@ -0,0 +1,25 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

copyright headers are missing

@@ -0,0 +1,25 @@
#!/bin/bash

Copy link
Contributor

Choose a reason for hiding this comment

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

this script is not passing at https://www.shellcheck.net/

$ shellcheck myscript
 
Line 3:
if [ -z $TS_SELENIUM_BASE_URL ]; then
        ^-- SC2086: Double quote to prevent globbing and word splitting.

Did you mean: (apply this, apply all SC2086)
if [ -z "$TS_SELENIUM_BASE_URL" ]; then
 
Line 18:
        cd local_tests
        ^-- SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

Did you mean: (apply this, apply all SC2164)
        cd local_tests || exit
 
Line 22:
        cd e2e
        ^-- SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

Did you mean: (apply this, apply all SC2164)
        cd e2e || exit

$ 

Signed-off-by: kkanova <kkanova@redhat.com>
@Katka92
Copy link
Contributor Author

Katka92 commented Jul 2, 2019

@benoitf Changed.

Copy link
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.

thx @Katka92 👍

@Katka92
Copy link
Contributor Author

Katka92 commented Jul 2, 2019

Thank you very much for valuable review. I'll wait till check passes and merge that. 👍

@benoitf
Copy link
Contributor

benoitf commented Jul 2, 2019

@vparfonov @riuvshin please note that there is a new image to add on the CI jobs for nightlies / and also when doing the release

@rhopp rhopp merged commit 47e59c2 into eclipse-che:master Jul 2, 2019
@riuvshin
Copy link
Contributor

riuvshin commented Jul 2, 2019

@benoitf @vparfonov I've updated corresponding jobs

@Katka92 Katka92 deleted the che7_dockerfile branch March 3, 2020 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants