dockerfile: remove bootstrap call #91

dinosk · 2018-12-06T13:47:42Z

Refactors Dockerfiles in the image building sequence to just
copy the updated instance directory and run pip install, instead
of running the bootstrap script again.

Signed-off-by: Dinos Kousidis dinos.kousidis@cern.ch

{{cookiecutter.project_shortname}}/Dockerfile.base

dinosk · 2018-12-07T16:20:47Z

{{cookiecutter.project_shortname}}/Dockerfile


-RUN ./scripts/bootstrap
+RUN pipenv run pip install --upgrade marshmallow==2.16.3


Temporary fixes a marshmallow error with DateTime field

dinosk · 2018-12-07T16:21:53Z

{{cookiecutter.project_shortname}}/Pipfile

@@ -7,7 +7,6 @@ name = "pypi"
 Babel = ">=2.4.0"
 Flask-BabelEx = ">=0.9.3"
 invenio = { version = ">=3.1.0dev20181106", extras = ["base", "metadata", "{{ cookiecutter.database }}", "auth", {%- if cookiecutter.elasticsearch == "5" %} "elasticsearch5", {%- elif cookiecutter.elasticsearch == "6" %} "elasticsearch6" {%- endif %} ]}
-{{ cookiecutter.project_shortname }} = {editable = true, path = "."}


Removes the final application from Pipfile, so that it can be installed in the final step after with the dependencies has been built.

Since the application is not installed anymore, this should be reflected in the scripts/bootstrap, i.e. it should run pipenv run pip install -e . after the pipenv install ...

dinosk · 2018-12-07T16:23:12Z

{{cookiecutter.project_shortname}}/Dockerfile.dev.base

+
+USER ${USER_ID}
+
+RUN pipenv install --deploy --dev --pre


The --pre and --dev flags are moved here as they are meant for a dev environment. #87

slint · 2018-12-12T14:55:13Z

{{cookiecutter.project_shortname}}/Dockerfile.dev.base

+COPY ./docker/uwsgi/ ${INVENIO_INSTANCE_PATH}
+
+RUN pipenv install --deploy --dev
+RUN pipenv run pip install --upgrade marshmallow==2.16.3


I think this was an issue in case you do pipenv install --pre which will include package pre-releases. Doing the following should be fine:

$ pipenv install marshmallow ... $ pipenv run pip freeze marshmallow==2.16.3 # vs. $ pipenv install --pre marshmallow ... $ pipenv run pip freeze marshmallow==3.0.0rc1

slint · 2018-12-12T14:57:29Z

{{cookiecutter.project_shortname}}/docker/wait-for-services.sh


 _rabbit_check(){ docker-compose exec mq bash -c "rabbitmqctl status" &>/dev/null; }
 check_ready "RabbitMQ" _rabbit_check
+
+_web_server_check_css(){


Isn't it enough to only wait for a valid HTTP response (as done below), since the assets are built before the server is actually started?

This covers the case where the link to the CSS file is there but returns 404.
In the webpack buildall step, the docker build doesn't fail so it won't actually show

I can't write under the comment above for some reason, (for marshmallow==2.16.3) this is indeed the case, we could remove the --pre when invenio 3.1.0 is not in prerelease and this won't be needed, except if I'm missing a better way

slint · 2018-12-12T14:59:50Z

{{cookiecutter.project_shortname}}/Pipfile.lock

@@ -0,0 +1,1650 @@
+{


I don't think we should be checking-in version control Pipfile.lock for the cookiecutter. The purpose of the file is similar to what was done before when generating the requirements.txt via pip-compile. Probably something that should be added in the Invenio docs under "Getting started" -> "Next steps".

The point is to make the build faster, if you are installing the default configuration (e.g. db and es version) there's one less step, otherwise one would have to run it again as it is done in .travis.yml for the different environments, and this could be documented. If one follows the docs it would be ok, otherwise I see that it adds a complication. Generally this is recommended pypa/pipenv#598 https://pipenv.readthedocs.io/en/latest/basics/#pipfile-lock-security-features

I think you're correct (so let's keep the Pipfile.lock), but for different reasons. Doing ./scripts/bootstrap in the beginning runs pipenv install --dev --skip-lock which is fast since it doesn't resolve and lock dependencies in Pipfile.lock. The problem is that because of the lack of resolving dependencies it might produce a version-conflicted environment (which it actually does at the moment).

I would say that the correct practice would be to actually install dependencies from the Pipfile.lock in bootstrap, which is equivalent to the old way of first installing from requirements.txt before starting to developing the application. This is not slow since it installs pinned dependencies, and produces as stable build. The command for this would be pipenv install --dev --ignore-pipfile (or pipenv sync --dev)

Upgrading dependencies should be a separate, conscious step that is initiated by the maintainer of the instance as part of its development process (and probably documented in the official docs)

lnielsen · 2018-12-14T13:18:23Z

{{cookiecutter.project_shortname}}/Dockerfile.base

@@ -1,7 +1,7 @@
 {% include 'misc/header.py' %}
 FROM inveniosoftware/centos7-python:3.6

-COPY ./ .
+COPY Pipfile ./


Pipfile.lock?

It will be generated in the RUN pipenv install --deploy step

* Refactors last Dockerfile in the image building sequence to just copy the updated instance directory and run pip install, instead of running the bootstrap script again. Signed-off-by: Dinos Kousidis <dinos.kousidis@cern.ch>

* Adds Dockerfile.dev.base to install dev packages and pre releases. * Refactors Dockerfiles to create a first image containing the application dependencies, and a second one to install the application, and collect and build its assets with webpack. * Adds a Docker build process in run-tests.sh, to create the final instance image and bring up the container cluster from docker-compose.full.yml. Tests if web server is accessible with HTTP, HTTPS, and has loaded CSS. Signed-off-by: Dinos Kousidis <dinos.kousidis@cern.ch>

lnielsen · 2019-02-12T10:42:56Z

Comments have been ticktized so merging

dinosk commented Dec 6, 2018

View reviewed changes

{{cookiecutter.project_shortname}}/Dockerfile.base Outdated Show resolved Hide resolved

dinosk force-pushed the Dockerfile-base branch from f19e96d to c8f3f0c Compare December 6, 2018 17:44

dinosk added this to Backlog in Invenio Sprint Dec 3 - Dec 14 2018 via automation Dec 6, 2018

dinosk self-assigned this Dec 6, 2018

dinosk moved this from Backlog to In progress in Invenio Sprint Dec 3 - Dec 14 2018 Dec 6, 2018

roksys mentioned this pull request Dec 7, 2018

ext: setting request_id in global vars inveniosoftware/invenio-app#40

Merged

dinosk commented Dec 7, 2018

View reviewed changes

dinosk mentioned this pull request Dec 7, 2018

scripts: pipenv flags for bootstrap #87

Closed

dinosk force-pushed the Dockerfile-base branch 19 times, most recently from f5a620f to 1ed800f Compare December 10, 2018 15:36

dinosk moved this from In progress to Pending review in Invenio Sprint Dec 3 - Dec 14 2018 Dec 10, 2018

lnielsen assigned dinosk and kpsherva and unassigned kpsherva Dec 12, 2018

slint approved these changes Dec 12, 2018

View reviewed changes

dinosk force-pushed the Dockerfile-base branch 8 times, most recently from 66c3f5e to ae96a7f Compare December 13, 2018 14:53

dinosk moved this from In progress to Pending review in Invenio Sprint Dec 3 - Dec 14 2018 Dec 13, 2018

dinosk force-pushed the Dockerfile-base branch from ae96a7f to 6186b39 Compare December 13, 2018 16:04

dinosk removed their assignment Dec 13, 2018

dinosk moved this from Pending review to Pending merge in Invenio Sprint Dec 3 - Dec 14 2018 Dec 14, 2018

lnielsen reviewed Dec 14, 2018

View reviewed changes

slint self-assigned this Dec 14, 2018

dinosk and others added 8 commits December 14, 2018 18:42

Dockerfile: remove bootstrap call

1fa61a6

* Refactors last Dockerfile in the image building sequence to just copy the updated instance directory and run pip install, instead of running the bootstrap script again. Signed-off-by: Dinos Kousidis <dinos.kousidis@cern.ch>

docker: same image for application containers

fcf774b

docker: use dependencies hash for tagging

ba18bed

installation: cleanup after datamodel move

3289700

docker: add images build script

292589d

scripts: use pipenv lock and sync

b06e660

tests: adapt to new scripts

497a7bc

slint force-pushed the Dockerfile-base branch from 6186b39 to 497a7bc Compare December 17, 2018 09:46

lnielsen mentioned this pull request Feb 12, 2019

Dockerfile: use bootstrap call in Dockerfile #116

Closed

lnielsen merged commit a0e20ec into inveniosoftware:master Feb 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dockerfile: remove bootstrap call #91

dockerfile: remove bootstrap call #91

dinosk commented Dec 6, 2018 •

edited

Loading

dinosk Dec 7, 2018

dinosk Dec 7, 2018

slint Dec 12, 2018

dinosk Dec 7, 2018

slint Dec 12, 2018

slint Dec 12, 2018

dinosk Dec 12, 2018

dinosk Dec 12, 2018

slint Dec 12, 2018

dinosk Dec 12, 2018

slint Dec 13, 2018

lnielsen Dec 14, 2018

dinosk Dec 14, 2018

lnielsen commented Feb 12, 2019


		RUN ./scripts/bootstrap
		RUN pipenv run pip install --upgrade marshmallow==2.16.3

dockerfile: remove bootstrap call #91

dockerfile: remove bootstrap call #91

Conversation

dinosk commented Dec 6, 2018 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lnielsen commented Feb 12, 2019

dinosk commented Dec 6, 2018 •

edited

Loading