amzn / oss-dashboard

A dashboard for viewing many GitHub organizations at once.
Apache License 2.0
159 stars 40 forks source link

Make the dashboard service wait until the db is available. #117

Closed gesellix closed 6 years ago

gesellix commented 6 years ago

Description of changes:

This commit introduces a script (wait-for) in the Dashboard Dockerfile to delay the dashboard startup until the Postgres db is available. It is already added to the docker-compose.yml.

Additionally, I reordered some Dockerfile statements to leverage the implicit Docker layer cache. This allows a better roundtrip when developing locally.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

gesellix commented 6 years ago

I also introduced more symbolic configuration of networks and volumes in the docker-compose file.

gesellix commented 6 years ago

The docker-compose.yml now allows to pass GH_ACCESS_TOKEN and OCTOKIT_API_ENDPOINT from the user's environment instead of setting them to an empty value.

hyandell commented 6 years ago

Thank you Tobias! :)

It looks good. I want to do a manual test before merging, and I also need to look at gesellix/wait-for to see what that is licensed under.

hyandell commented 6 years ago

Noting that wait-for is MIT licensed.

My Docker inexperience is a bit confused though, what happens when the current FROM ruby and the FROM geelix are providing different linuxes? Alpine vs Debian.

gesellix commented 6 years ago

@hyandell The multiple FROM statements are a feature called multi-stage builds, where you more or less can describe independent builds inside your Dockerfile. The only "shared" resource is the filesystem, which enables you to copy files from one image's filesystem to another image. The different platforms are only relevant when copying binaries compiled for different platforms (or against different dynamic libs). In this special case the wait-for ~lib~ file is only a shell script, so the platform isn't relevant.

While writing this I become aware that we could simply replace the multi-stage build with a RUN statement in which you only download the wait-for script. If you prefer such a solution, just leave a note here and I'll change it.

gesellix commented 6 years ago

Please also note that I'm not the actual author of the wait-for script, but only forked from https://github.com/Eficode/wait-for, to add an enhanced Dockerfile.

gesellix commented 6 years ago

Now regarding the license: would wait-for with MIT be an issue?

hyandell commented 6 years ago

MIT is good. Doing the simpler RUN statement sounds better to me.

gesellix commented 6 years ago

@hyandell alright, done :)

If you prefer, I can also squash the commits.

hyandell commented 6 years ago

Thanks Tobias.

I did a bit of chatting internally; the one thing that stands out is the curl -o for the wait-for code. This gives your repo a very direct injection point to any of these docker instances being setup, whereas the rest of it is based on trust in Debian's release processes.

What do you think to copying the shell script in as part of the patch? Also, you should probably have an MIT license on the top of the file.

gesellix commented 6 years ago

@hyandell You're right, using curl breaks the current barrier of trust. I fully agree that including the script in this repo makes sense. As I mentioned, I'm not the actual author of that script, so do we need to ask the people at https://github.com/Eficode/wait-for? I'm not very experienced in such legal aspects, but technically I can certainly add the wait-for script in the patch and also include its license at its top.

gesellix commented 6 years ago

I've added wait-for including its license to the PR.

hyandell commented 6 years ago

Thanks @gesellix - putting the license at the top like that is perfect. The original is MIT, so that fits.

hyandell commented 6 years ago

PR merged :)

gesellix commented 6 years ago

thanks :)