vladfaust / crystalworld

RealWorld back-end API implementation 👍
https://realworld.io
MIT License
43 stars 5 forks source link

feat: simplified deployment process #8

Closed dscottboggs closed 5 years ago

dscottboggs commented 5 years ago

I added a docker-entrypoint.sh which provides a default for the database url and JWT secret. This allows deployment to be simplified to a one liner like git clone ... && cd crystalworld && docker-compose up ...

Additionally, (perhaps this should be a separate PR) if you deploy it in docker by default it's not available outside the docker network because Onyx only binds to localhost. This allows overriding the host and port values from environment variables.

vladfaust commented 5 years ago

Hey, thanks! Gonna sleep right now and thoroughly check later. Please fix the build issue 👍

vladfaust commented 5 years ago

Alright, I'm against this PR, because:

However, your point about the host is absolutely correct. You can propose a PR which will bind the server to "0.0.0.0" by default. But please, call the environment variables HOST and PORT accordingly.

dscottboggs commented 5 years ago

Hello, just noticed your comments, I'll get on the changes you requested.

couple discussion points:

* Docker Compose is not native to Docker, it usually should be installed separately (and currently the version which supports `3.5` is not that easy to install, at least it's not available on my Ubuntu 16.04 via default `apt`)

all fair points. I withdraw my suggestion to change the readme to default to that. If you'd like I can write a more generally-compatible docker-compose file to include, or that could just be left out :man_shrugging:

* `docker-entrypoint.sh` brings unnecessary complexity to the project:

  * If a developer needs to bind the database to a host file, it can be simply done with `-v`

If this is the case, then why does the developer need to specify the location of the database at all? Why not just have an optional -v /host/path:/app/crystalworld.db setup option, and have it default?

  * JWT generation is overhead, it can be achieved with one-line `openssl rand -hex 16` command

Oh, ok, I didn't find that command when I was trying to do that. thanks for the tip

I still think there should be a docker-entrypoint.sh which

  1. provides defaults for the random token and the database location (these steps could be done on launch instead of in a docker-entrypoint.sh)
  2. migrates the database if there isn't one already.

The major reason I wrote the script was because it seems unnecessary to have someone manually migrate the databases.

vladfaust commented 5 years ago

Closed by 1f7d0d89ef5c56e77ef0c2a2366d267961b11009 and 5c61d6ab0d4c9342a55d86326b4b58ac9cd7cfb9