Feedback /config.json /healthcheck

pascalandy commented 6 years ago

Hey Marco, I tested properly tonight. Good news! It’s working as expected ⚡️⚡️⚡️

Few tweaks to consider.

config.production.json

I miss the way we were copying the file config.production.json via the Dockerfile as I use Mailgun as my mail provider. Many people will probably appreciate that they can easily edit config.production.json without crazy sed ninja commands :-p

Healthcheck

The health check should happen on http://$(hostname -i):2368 or http://localhost:2368 because the container takes way to long to start if we are doing the health check it’s absolute URL. In my case 6 minutes. Also, the proxy might cause an issue not related to the ghost container (this would also mess with the monitoring stats where you see a container restarting like crazy even when the container itself is healthy)

Also, the proxy (traefik) is having a hard time to configure itself as the container is restarting all the time.

For now, I bypass it

pascalandy commented 6 years ago

Dockerfile

Here I did few subtle comments and spacing clean up if you are interested.

pascalandy commented 6 years ago

Overall, I'm very impressed that you manage to drop the image size from 464 MO to 163 MO 🎉. Good job!

mmornati commented 6 years ago

Hey. Thanks for all these feedbacks.

config.production.json I'm agree. I will check how we can expose the configuration. What we can imagine is an external file provided to the docker container
Not sure to understand the helthcheck comment. Actually is making a check on localhost 2368. You are proposing the localhost or the docker ip... so it should be good, right? Maybe it is anything related to the netcut (nc) command?
Thanks for the Dockerfile cleanup. In the end you can even propose directly push the code without using gist ;)

Thanks again. I will check these points and get back to you

mmornati commented 6 years ago

I delivered a new version with a new way to override the configuration (it is described in the readme): you can provide your external configuration file via a volume:

docker run -d -p 2368:2368 -e WEB_URL=http://test.blog -v /opt/data:/var/lib/ghost/content -v /opt/myconfiguration.json:/var/lib/ghost/config.override.json mmornati/docker-ghostblog

I also changed the healthcheck with a full wget on localhost as requested. I let you test if it is working for your specific case

pascalandy commented 6 years ago

I decided to move sed -i "s|http://localhost:2368/|$WEB_URL|g" config.production.json after the config override.

screen shot 2017-11-09 at 11 00 16 pm

In my case, I still want to provide the WEB_URL variable on top of the config override.

Still testing.

mmornati commented 6 years ago

Lol. I was thinking to the same thing when I delivered. But it will work only if you leave http://localhost:2368 as url in the external file. Maybe it is better to put a placeholder instead. I will propose a merge request to you to review

pascalandy commented 6 years ago

Looks we can create a .dockerignore as well. Still testing.

mmornati commented 6 years ago

I delivered the version with the configuration override at the beginning as you suggested

mmornati commented 6 years ago

About the dockerignore, what is your target? The ignore is to prevent copying file when you are starting building it. Not sure it will help...

pascalandy commented 6 years ago

Just testing if we the build would save us few bytes from node_librairies

mmornati commented 6 years ago

Ah yeah. No, I really think it is related to the build folder. When you run a docker build, the first step is to copy all the files from the Dockerfile folder to the Docker machine... If you want to exclude some of them you can add a dockerignore.

pascalandy commented 6 years ago

Oh I see! I saw in the node:6 image that there is a node_module directory and I was wondering if we could build the image without it.

pascalandy commented 6 years ago

EDIT: It works the way you wrote it, but I feel it should throw an error.

Issue with knex-migrator-migrate.

Based on this, we should use knex-migrator migrate

I found it here:

if [ ! -s "$(awk '/"filename": "(.*)"/ {print $2}' $CONFIG | sed -e s/\"//g)" ]; then
        echo "Empty database. Initializing..."
        knex-migrator-migrate --init --mgpath "$GHOST_INSTALL/current"
else
        echo "Database already exists. Executing migration (if needed)"
        knex-migrator migrate --mgpath "$GHOST_INSTALL/current"
fi

mmornati / docker-ghostblog