Better automate deploy #762

kousu · 2017-06-23T09:32:04Z

Changes proposed in this pull request:

Automate the frontend compilation steps inside Dockerfile
Make it so that docker-compose up is enough to get the project running (admittedly with an empty database.)
Tidy up the subsidiary docker-compose files by coalescing their common bits directly into Dockerfile or the main docker-compose.yml
Don't use docker-compose.override.yml, because explicit is better than implicit: instead name it docker-compose.dev.yml
Add copious comments to the Dockerfile.

Status

This was branched off #757 so please merge that first.

READY
HOLD
WIP (Work-In-Progress)

How to verify this change

Do a fresh checkout and a fresh build. See that it works with just docker-compose up. Try then with docker-compose -f docker-compose.yml -f docker-compose.yml up

Deployment notes and migration

Migration is needed for this change

Additional notes

Deployment has been a huge headache for me. INSTALL.md is pretty nicely fleshed out, but it's still really hard to knowing when and what order to run things, and I would rather move as many instructions into (commented) scripts instead of into instructions that humans need to follow manually.

There should only be one command to run to get any project going. Here, it's ``` docker-compose up ``` This also moves parts of the subsidiary docker-compose files directly into Dockerfile or the main docker-compose.yml, since they make more sense there, and relabels what's left of docker-compose.override.yml after that as docker-compose.dev.yml, since that's what it really is. This also adds copious comments to the Dockerfile.

kousu · 2017-06-23T09:33:28Z

^ this was a change to the Docker stuff so per #758 the CI check is misleading here. The only way to verify this is to do a fresh build on your own machine.

coveralls · 2017-06-23T09:40:20Z

Coverage decreased (-0.01%) to 95.338% when pulling f058291 on kousu:better-automate-deploy into 0dad083 on savoirfairelinux:dev.

kousu · 2017-06-23T09:48:35Z

Ah, actually, this isn't quite working. It puts the static files in src/static/ instead of src/sous_chef/static

It was added as a workaround for a semi-broken JS dependency, and its evidently unnecessary now. Fixes #759.

coveralls · 2017-06-27T18:34:10Z

Coverage increased (+0.03%) to 95.378% when pulling b9e039d on kousu:better-automate-deploy into 0dad083 on savoirfairelinux:dev.

See #755

…net. Instead, listen on localhost, forcing a proxy in (hopefully with TLS) in front if the sysadmin wants to expose Sous-Chef publically. Fixes #761.

coveralls · 2017-06-27T19:02:28Z

Coverage remained the same at 95.352% when pulling 382852b on kousu:better-automate-deploy into 0dad083 on savoirfairelinux:dev.

coveralls · 2017-06-27T19:43:06Z

Coverage increased (+0.01%) to 95.352% when pulling 166fced on kousu:better-automate-deploy into a7c7068 on savoirfairelinux:dev.

kousu · 2017-06-27T19:49:14Z

I'm probably not going to get to fixing this up this week. It shouldn't be tricky, but it just needs some repetition until it reliably puts all the files in the right paths, and docker rebuilds are really really slow and discouraging. So, this will happen, but you'll have to be patient with me. Thanks.

erozqba

I have opened a different PR #775 to test the project using Docker, we should probably unify this changes or merge the other PR first so we test this changes and verify that they are not breaking anything.

erozqba · 2017-07-01T18:15:57Z

.gitignore

+# the magic docker-compose override file is for overriding settings locally
+# it should not be a part of the repo. If you want per-environment settings,
+# use an explicit `docker-compose -f docker-compose.yml -f docker-compose.environment.yml`
+docker-compose.override.yml


@kousu If we are using docker-compose we should follow their conventions, and by default docker-compose command will read the configurations files "docker-compose.yml" and "docker-compose.override.yml". The main idea is to have in the "docker-compose.yml" the configurations that remain the same in all environements, use "docker-compose.override.yml" to the development environment that is used more frequently by developers and have separated configurations files for other environments like staging or prod.
For more detailed information check https://docs.docker.com/compose/extends/#example-use-case

Thanks for the link. Having the laser focus there really helps move the debate forward, and I agree that you're doing it just as they suggest.

I still think it's wrong. docker-compose.yml can have dev settings in it by default which are then overridden by docker-compose.prod.yml. "override" says to me "these are settings which are custom to this specific instance, which I need in order"; if they meant "override" to always mean "dev" then they should have called it "dev". Emphatically I think that name means they must not be stored as part of the codebase, and not be in the repo, be .gitignore'd.

@kousu I agree with you personally. I don't like the use of the word "override" of docker-compose either and it brought me some troubles and confusion... but I think this is their convention according to their doc, so I would follow the usage of "override" so that we work on the same page with docker-experienced developers.

I think it is important to mention the doc link somewhere in the project, in order to help developers like me and @kousu who don't know this convention of docker-compose a priori.

erozqba · 2017-07-01T18:18:16Z

Dockerfile

+# the default entrypoint for the container
+# when this ends, the container ends, then restarts
+# can be overridden in docker-compose.yml files
+CMD python3 src/manage.py runserver 0.0.0.0:8000


We should not use this server by default, this is a development server that should be used only for development if we are going to use this image in production, we should use by default gunicorn and override it with the development server in the docker-compose.override.yml configuration file, or not have any entry point at all.

I thought you must have an entry point. Without one, the container shuts down immediately.
I would love it if docker let you just bring up a container with no master process. It lets you docker exec arbitary side commands in a single container, so why do any of them need to be key?

Resolved in #775:

https://github.com/savoirfairelinux/sous-chef/pull/775/files#diff-3254677a7917c6c01f55212f86c57fbfR28
https://github.com/savoirfairelinux/sous-chef/pull/775/files#diff-f772bd5403017e9075de6e9455c26521R10

erozqba · 2017-07-01T18:18:51Z

Dockerfile

+# the 'rebuild node-sass' is a patch over a glitch which showed up recently;
+# the tip that fixed it is from https://github.com/sass/node-sass/issues/1387#issuecomment-185451183
+# perhaps it is not truly necessary.
+run mkdir /code/src/sous_chef/static/


If this is not truly necessary we should not include it, so we need to check it.

I think this is resolved in #775.

erozqba · 2017-07-01T18:21:34Z

docker-compose.dev.yml

+services:
+  web:
+    volumes:
+      - .:/code


We don't need this file and we should keep using docker-compose.override.yml because for developers is easier to do docker-compose up -d than docker-compose -f docker-compose.yml -f docker-compose.dev.yml

Resolved in #775.

erozqba · 2017-07-01T18:21:46Z

docker-compose.override.yml

@@ -1,8 +0,0 @@
-version: '3'


Keep this file.

Resolved in #775.

erozqba · 2017-07-01T18:22:34Z

docker-compose.yml

@@ -14,6 +14,8 @@ services:
      - backend

  web:
+    ports:
+      - "127.0.0.1:8000:8000"
    environment:


I don't think we need this binding why you have added it?

I moved this binding from docker-compose.override.yml, and added the fix for #761 at the same time.

Seems to me that it's needed? I didn't see in #775.

@lingxiaoyang if we are only using this binding in development mode, in the docker-compose.override.yml we don't relly need to take care of security and promiscuous ports because we are using the development server anyway.

@lingxiaoyang @kousu but we should change the binding in the nginx container to add the IP
127.0.0.1:80:80

@erozqba Got your idea.

lingxiaoyang · 2017-07-05T17:34:21Z

I think we need to reconcile this PR with #775, as both are working on the same files.

lingxiaoyang · 2017-07-11T17:57:39Z

INSTALL.md


+[collectstatic](https://docs.djangoproject.com/en/1.11/ref/contrib/staticfiles/#collectstatic) is similar, but built into Django.
+We are not using it to manage our own static files, but some of our Django dependencies might need this to be run.


@kousu Good work! We keep this file.

erozqba · 2017-07-28T13:15:44Z

@kousu could you rebase this branch?

Update the documentation from changes proposed in savoirfairelinux/pull/762.

This was referenced Jun 26, 2017

--unsafe-perm #759

Closed

CI server doesn't test docker #758

Closed

Remove --unsafe-perm.

b9e039d

It was added as a workaround for a semi-broken JS dependency, and its evidently unnecessary now. Fixes #759.

kousu added 2 commits June 27, 2017 18:43

Use COPY instead of ADD in Dockerfile

052980e

See #755

Don't expose the docker container for sous-chef directly to the inter…

382852b

…net. Instead, listen on localhost, forcing a proxy in (hopefully with TLS) in front if the sysadmin wants to expose Sous-Chef publically. Fixes #761.

Merge branch 'dev' into better-automate-deploy

166fced

manumilou requested a review from erozqba June 28, 2017 14:32

erozqba reviewed Jul 1, 2017

View reviewed changes

lingxiaoyang mentioned this pull request Jul 5, 2017

Run test in Travis-CI using Docker #775

Merged

5 tasks

lingxiaoyang reviewed Jul 11, 2017

View reviewed changes

erozqba added the needs-rebase label Jul 11, 2017

guillaumep pushed a commit to santropolroulant/sous-chef that referenced this pull request Nov 23, 2019

Update INSTALL.md

e3aeb11

Update the documentation from changes proposed in savoirfairelinux/pull/762.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better automate deploy #762

Better automate deploy #762

kousu commented Jun 23, 2017 •

edited

Loading

kousu commented Jun 23, 2017

coveralls commented Jun 23, 2017

kousu commented Jun 23, 2017

coveralls commented Jun 27, 2017

coveralls commented Jun 27, 2017

coveralls commented Jun 27, 2017

kousu commented Jun 27, 2017

erozqba left a comment

erozqba Jul 1, 2017

kousu Jul 6, 2017

lingxiaoyang Jul 11, 2017 •

edited

Loading

erozqba Jul 1, 2017

kousu Jul 6, 2017

lingxiaoyang Jul 11, 2017

erozqba Jul 1, 2017

lingxiaoyang Jul 11, 2017

erozqba Jul 1, 2017

lingxiaoyang Jul 11, 2017

erozqba Jul 1, 2017

lingxiaoyang Jul 11, 2017

erozqba Jul 1, 2017

kousu Jul 6, 2017

lingxiaoyang Jul 11, 2017

erozqba Jul 11, 2017 •

edited

Loading

lingxiaoyang Jul 12, 2017

lingxiaoyang commented Jul 5, 2017

lingxiaoyang Jul 11, 2017

erozqba commented Jul 28, 2017


		[collectstatic](https://docs.djangoproject.com/en/1.11/ref/contrib/staticfiles/#collectstatic) is similar, but built into Django.
		We are not using it to manage our own static files, but some of our Django dependencies might need this to be run.

Better automate deploy #762

Are you sure you want to change the base?

Better automate deploy #762

Conversation

kousu commented Jun 23, 2017 • edited Loading

Changes proposed in this pull request:

Status

How to verify this change

Deployment notes and migration

Additional notes

kousu commented Jun 23, 2017

coveralls commented Jun 23, 2017

kousu commented Jun 23, 2017

coveralls commented Jun 27, 2017

coveralls commented Jun 27, 2017

coveralls commented Jun 27, 2017

kousu commented Jun 27, 2017

erozqba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lingxiaoyang Jul 11, 2017 • 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

erozqba Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lingxiaoyang commented Jul 5, 2017

Choose a reason for hiding this comment

erozqba commented Jul 28, 2017

kousu commented Jun 23, 2017 •

edited

Loading

lingxiaoyang Jul 11, 2017 •

edited

Loading

erozqba Jul 11, 2017 •

edited

Loading