search

hackoregon / backend-examplar-2018

an example dockerized geo-aware django backend
MIT License
4 stars 5 forks source link

Remove unnecessary installs from DOCKERFILE.api.production #63

Open MikeTheCanuck opened 6 years ago

MikeTheCanuck commented 6 years ago

There are three separate enhancements in the production DOCKERFILE that I don't believe are necessary for every project:

bhgrant8 commented 6 years ago

1, So the use of the stretch variant comes down to the question of the use of the wait-for-it script to test whether the container is able to make a connection with the postgres client prior to completing the rest of the bootup cycle. The stretch variant seemed to be necessary as per @znmeb for the use of of the required postgres client necessary to connect to the current database.

The root of this usage is from this recommendation from Docker Compose: https://docs.docker.com/compose/startup-order/

It does improve on the local dev container experience as usually the api container will attempt to startup prior to the database being booted. We may have adequate healthchecks on the database and api container through aws that this may not be necessary?

  1. Lines 5-13 are packages related to 2 concerns:

    • the wait for it script described in 1. - postgresql-client-9.6 \ and i assume qqy?
    • for any project making use of postgis/geodjango stack then they will require the geospatial libraries to be installed in the container. 
      binutils \
gdal-bin \
libproj-dev \

      see ref in the comments of the docker file: https://docs.djangoproject.com/en/2.0/ref/contrib/gis/install/geolibs/

  2. Line 22 Nope, hence why is separate req file, can be left out from projects not needing it, could be possibly made more automated. See: https://github.com/hackoregon/backend-examplar-2018/issues/22

MikeTheCanuck commented 6 years ago

Thanks Brian - this is great insight, and thanks as always to the docs links!

  1. I'm having trouble imagining how this will improve things in either Travis build/test or in the actual spin-up of a Docker container image in ECS - in the former case, if it fails to connect in the usual timeout, it's unlikely to come back during the build/test cycle (that'll probably mean the production DB is down, since it's otherwise never falling down - due to the evidence over the last year that they've never required manual intervention, nor have they ever rebooted) and if it fails the solution is to restart the Travis Build; in the latter case, there is so little we can do with a Docker container in ECS once it starts, that if there's a failure there we can do nothing but restart the container - which the ECS services will do for us automatically anyway.
  2. While the wait-for-it script doesn't seem useful here, the postgis/geodjango stack support sounds useful and would be handy to have already laid out here. Would you mind if we added some comments for goons like me who aren't connecting the dots when looking at this Dockerfile?
  3. This too would benefit from a comment for goons like me who might be tempted to delete stuff that doesn't immediately look "normal".
bhgrant8 commented 6 years ago
  1. yeah, i was always a bit on the fence on this being in there
  2. sure
  3. sure, "self-documenting" code is only ever documenting to the self that wrote it. sometimes not even then 😜
znmeb commented 6 years ago
  1. A bunch of stuff breaks in development if one container is running "stretch" and another running "jessie". I believe they're all on stretch now and should remain that way.
  2. There's no penalty for using "stretch" in both development and production, and for consistency I think it's a good idea - less cognitive load.
MikeTheCanuck commented 6 years ago

Addressing some of item (2) with #71