301 docker #318

jpigree · 2018-05-19T19:17:50Z

Adds Dockerfile from which you can create a Jarvis Docker image from the currently checked out Jarvis code.
I faced multiple issues mainly because in container distribution have many differences from the host ones (systemd, dbus, ...).

A Makefile is also here to document how to build the Dockerfile and run the image.

As a side note, I refactored a bit the setup.sh to pass more cleanly the Python version used.

gucharbon · 2018-09-03T20:51:32Z

Dockerfile

+            notification-daemon \
+            sudo \
+            xorg
+


Remove apt cache after installing depndencies as mentioned in dockerfile best practices (https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#run)

RUN apt-get update && \ apt-get -y install \ dbus-x11 \ libdbus-1-dev \ libespeak1 \ libnotify-cil-dev \ notification-daemon \ sudo \ xorg && \ rm -rf /var/lib/apt/lists/*

gucharbon · 2018-09-03T20:52:55Z

Dockerfile

+            sudo \
+            xorg
+
+RUN pip3 install virtualenv


Use pip without cache:

RUN pip3 install --no-cache-dir virtualenv

gucharbon · 2018-09-03T20:55:53Z

Dockerfile

+COPY . /app
+WORKDIR /app
+
+RUN sudo /bin/bash ./docker.sh


Why use sudo ? Are you not still root user at this point ?
In any way, best pratice is to reduce number of docker layers. Lines 20 to 24 could be added in single layer.

RUN /bin/bash ./docker.sh && \ useradd -Um jarvis && \ echo "jarvis ALL=(ALL) NOPASSWD:ALL" >> /etc/sudoers && \ chown -R jarvis /app

After writing this I realize that you add a line into /etc/sudoers so you're definetely root at this point

gucharbon · 2018-09-03T21:15:19Z

Dockerfile

+RUN chown -R jarvis /app
+USER jarvis
+
+RUN sudo /bin/bash ./setup.sh --python3


It may not be worth the effort, but I think that if you create a docker image for jarvis, it should not use the setup.sh script since environment is well controlled compared to bare metal machine.

Instead, iI suggest you install deps in Dockerfile, and provide several Dockerfiles if you wish to support several distributions (which may not be necessary). This way you will reduce drastically image size (no virtualenv and no cache when installing with apt or pip, remove archives when installing phantomjs, etc..).

jpigree · 2018-09-10T17:24:56Z

Hi @gucharbon, thank you for your review. I am working on the fixes you asked. I just have issues making the image work again (I rebased on top of the mainline). I will keep you updated.

gucharbon · 2018-09-12T12:11:17Z

Dockerfile

@@ -0,0 +1,58 @@
+FROM python:3.6.5-jessie
+
+env DEBIAN_FRONTEND noninteractive


Docker linters do not recognize env if it's in lower case most of the time. If you can just change to
ENV DEBIAN_FRONTEND="noninteractive"
it would be more readable

Hi @gucharbon, thank you for your review. I am working on the fixes you asked. I just have issues making the image work again (I rebased on top of the mainline). I will keep you updated.

I mentioned two little things that could still be changed, but you did a really nice job with this new Dockerfile !

But in any case I'm not reviewer or maintainer of this project, so even if I review it you will still need approval of a maintainer :)

Thanks even more for the review. But how can you be assigned as a reviewer of this PR if you are not a maintainer/reviewer of the project?

Oh Okay, I missread the Github interface. It just says that you reviewed my PR but you are not assigned.

Yep, just Killing some time :)

gucharbon · 2018-09-12T12:16:49Z

Dockerfile

+RUN wget -qO - https://bitbucket.org/ariya/phantomjs/downloads/phantomjs-2.1.1-linux-x86_64.tar.bz2 | tar xjf - && \
+  ln -s /usr/local/share/phantomjs-2.1.1-linux-x86_64/bin/phantomjs /usr/local/bin/phantomjs
+
+COPY . /app


I'm not sure if this will set recursively, but you can set ownership during copy:
COPY --chown=jarvis:jarvis . /app

A big advantage is that since docker layers are read only, when copying first and then setting permission, you add twice the size of the content you copy. We noticed that when building image with pretty large content (>1GB) and when modifying ownership in another layer it added 1 GB in the image size. See https://medium.com/@lmakarov/the-backlash-of-chmod-chown-mv-in-your-dockerfile-f12fe08c0b55 and moby/moby#34263 (comment)

gucharbon · 2018-09-12T12:23:40Z

Hi @gucharbon, thank you for your review. I am working on the fixes you asked. I just have issues making the image work again (I rebased on top of the mainline). I will keep you updated.

I mentioned two little things that could still be changed, but you did a really nice job with this new Dockerfile !

But in any case I'm not reviewer or maintainer of this project, so even if I review it you will still need approval of a maintainer :)

gucharbon · 2018-09-12T12:56:00Z

Dockerfile

            libdbus-1-dev \
+            libdbus-glib-1-dev \


You start from python image with version 3.6.5 installed but this line install python3.5 as dependency. You could start from debian instead of python if you do not want to have several python installed (increase size for nothing). You can check that with
apt install --dry-run libdbus-glib-1-dev

gucharbon · 2018-09-12T12:58:51Z

Dockerfile

            notification-daemon \
+            python3-dbus \


Like before, either build libraries which need python from source or use debian base image instead of python. This will install python3 as debian still uses python3.5 instead of python3.6 by default

Oh. I did not realize this. Thanks. This should decrease the image size.

gucharbon · 2018-09-12T13:07:32Z

Dockerfile

            notification-daemon \
+            python3-dbus \
+            python-dbus \


Remove this line, it will install python2, I don't think you need it if you are using python3.
The same goes for python-notify2 . You can install python3-notify2 instead.
I don't know how you can handle pyhton-imdbpy though, maybe with pip

StefanosChaliasos · 2018-09-13T14:52:34Z

@jpigree When you are ready please update the README and publish the image to docker hub. It would be great if you could maintain the image.

pnhofmann · 2019-01-18T21:13:37Z

So - what's the progress?

Noticed some dependencies (chromedriver, python3-dbus, build-essential, phantomjs) are obsolete by now (refere #336).

I don't really understand why sudo is configured. Can't think of any Jarvis-functionality, which requires root.

But furthermore, this Dockerfile looks ready for me.

aditisrinivas97 · 2019-01-18T21:36:02Z

@pnhofmann sudo needs to be configured because the setup.sh file uses it on line 70. If its not configured, you get the following error message :

./setup.sh: line 70: sudo: command not found

pnhofmann · 2019-01-19T09:38:01Z

Setup.sh isn't used for Docker.

At last in the latest version of this Dockerfile - sudo looks like leftover from first version which did call setup. Especially since the added sudo-line is removed with 'sed' 2 lines below.

aditisrinivas97 · 2019-01-19T09:50:41Z

Right! It's in the first version. Thanks!

jpigree · 2019-01-21T17:54:52Z

Hi. @StefanosChaliasos I just saw your comment today. Sorry. It has been a long time.
Are you still interested into putting this in dockerhub? If I do it myself, this means I will create (and own) the Dockerhub organization "Jarvis" then create the repository for the Docker image. I think this would be unwise as I am not even a contributor of the project yet. This is my first contribution and I don't even use Jarvis for now. What I can do is do all the setup work then hand it over a collaborator/admin of this repository.

@pnhofmann this PR was done in May 2018. I just looked at the history, a lot of commits have been merged since so it needs to be fully retested and adjusted. I will also look at the sudo matter. Give me a few days to rework this.

Hope it will be merged soon afterwards. Don't want to redo the same work again and again. :)

StefanosChaliasos · 2019-01-22T08:15:46Z

@jpigree Hello, please create a Dockerfile for the last commit on master branch. We remove most of the dependencies so it's must be simpler.

- Create a Dockerfile which builds a Jarvis container - Reworked the setup.sh script in order to be able to choose Python version with flags

…is container

Removed unused dependencies and simplified greatly the Dockerfile.

jpigree · 2019-01-23T20:05:29Z

I updated the Dockerfile. However, from what I see,

@StefanosChaliasos Jarvis does not require the service X anymore?
If so, I will remove the X socket mount in the Makefile command "run_docker".
Then we should be good. We can merge it. For Dockerhub, somebody wants to be owner? I think it will be best to have multiple owners.

StefanosChaliasos · 2019-01-24T13:36:18Z

@jpigree you can push it on Dockerhub

jpigree force-pushed the 301-Docker branch from a5dbd81 to a108177 Compare May 19, 2018 19:27

jpigree mentioned this pull request May 19, 2018

Create Dockerfile #301

Closed

jpigree force-pushed the 301-Docker branch 2 times, most recently from 0c5545c to 0b76e45 Compare May 27, 2018 20:51

gucharbon reviewed Sep 3, 2018

View reviewed changes

Dockerfile Outdated

sudo \

xorg

RUN pip3 install virtualenv

Copy link

gucharbon Sep 3, 2018

Choose a reason for hiding this comment

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

Use pip without cache:

RUN pip3 install --no-cache-dir virtualenv

gucharbon reviewed Sep 3, 2018

View reviewed changes

jpigree force-pushed the 301-Docker branch from 0b76e45 to 7be7c69 Compare September 11, 2018 21:06

gucharbon reviewed Sep 12, 2018

View reviewed changes

appi147 mentioned this pull request Oct 30, 2018

Add Dockerfile - Issue #301 #364

Closed

pnhofmann mentioned this pull request Nov 4, 2018

Show valid error message when no cricket matches are being played. #369

Closed

jpigree added 4 commits January 22, 2019 07:42

(301) Add Dockerfile

3436092

- Create a Dockerfile which builds a Jarvis container - Reworked the setup.sh script in order to be able to choose Python version with flags

(301) Add a makefile to document the commands to build and run a Jarv…

cfbd39c

…is container

(301) Take into account the code review from gucharbon

890471c

Updated Dockerfile to install the current Jarvis version

2a06e57

Removed unused dependencies and simplified greatly the Dockerfile.

jpigree force-pushed the 301-Docker branch from 7be7c69 to 2a06e57 Compare January 23, 2019 19:33

StefanosChaliasos merged commit 1d67bb4 into sukeesh:master Jan 24, 2019

jpigree deleted the 301-Docker branch January 24, 2019 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

301 docker #318

301 docker #318

jpigree commented May 19, 2018

gucharbon Sep 3, 2018

gucharbon Sep 3, 2018

gucharbon Sep 3, 2018 •

edited

Loading

gucharbon Sep 3, 2018

jpigree commented Sep 10, 2018

gucharbon Sep 12, 2018

jpigree Sep 13, 2018

jpigree Sep 13, 2018

gucharbon Sep 13, 2018

gucharbon Sep 12, 2018

gucharbon commented Sep 12, 2018

gucharbon Sep 12, 2018

gucharbon Sep 12, 2018

jpigree Sep 12, 2018

gucharbon Sep 12, 2018 •

edited

Loading

StefanosChaliasos commented Sep 13, 2018

pnhofmann commented Jan 18, 2019

aditisrinivas97 commented Jan 18, 2019

pnhofmann commented Jan 19, 2019

aditisrinivas97 commented Jan 19, 2019

jpigree commented Jan 21, 2019

StefanosChaliasos commented Jan 22, 2019

jpigree commented Jan 23, 2019

StefanosChaliasos commented Jan 24, 2019

		@@ -0,0 +1,58 @@
		FROM python:3.6.5-jessie

		env DEBIAN_FRONTEND noninteractive

301 docker #318

301 docker #318

Conversation

jpigree commented May 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gucharbon Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpigree commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gucharbon commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gucharbon Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

StefanosChaliasos commented Sep 13, 2018

pnhofmann commented Jan 18, 2019

aditisrinivas97 commented Jan 18, 2019

pnhofmann commented Jan 19, 2019

aditisrinivas97 commented Jan 19, 2019

jpigree commented Jan 21, 2019

StefanosChaliasos commented Jan 22, 2019

jpigree commented Jan 23, 2019

StefanosChaliasos commented Jan 24, 2019

gucharbon Sep 3, 2018 •

edited

Loading

gucharbon Sep 12, 2018 •

edited

Loading