alphagov / govuk-docker

GOV.UK development environment using Docker 🐳
MIT License
82 stars 22 forks source link

Signon improvements #681

Closed chrisroos closed 1 year ago

chrisroos commented 1 year ago
chrisroos commented 1 year ago

@floehopper and I discussed this yesterday. Notes below:

The signon-worker change looks good to me - it looks as if there is precedent for doing this in other apps.

I'm a bit less sure about the mysql client change:

  1. There doesn't look as if there's much precedent for using a Dockerfile other than Dockerfile.govuk-base. And the ones that do, seem to be very different apps, e.g. apps using the Go language.

There is precedent in the asset-manager and whitehall Dockerfiles, which only differ from the base by the packages they install (clamav for asset-manager and mysql and ghostscript for whitehall).

  1. Also presumably this will add the mysql client to the Signon images used in production which might not be necessary/desirable.

As discussed, this is only used in development so there's no concern about them appearing in production.

  1. It doesn't seem great to duplicate most of Dockerfile.govuk-base. Given that it already includes postgresql-client for Signon (which doesn't need it), perhaps we could just add default-mysql-client to Dockerfile.govuk-base itself (even though it's not needed by many other apps). Or maybe there's another way to "extend"/"parameterize" the image rather than duplicate most of it. My Docker fu is rusty, so not sure whether/how this is possible.

I've added the mysql client to the base Dockerfile in the commit titled, "Install mysql client in Dockerfile.govuk-base".

I agree that it would be an improvement if we were able to parameterize the Dockerfiles as we would be able to remove the duplication between asset-manager and whitehall Dockerfiles and the base Dockerfile. I'm not planning to do anything about that at this time.

@floehopper - can you re-review this PR, please?

chrisroos commented 1 year ago

Thanks, @floehopper :+1: