Dockerfile Rework #1205
Dockerfile Rework #1205
Conversation
| with: | ||
| service: autoupdate | ||
|
|
||
| readme-validation: |
There was a problem hiding this comment.
Is anything beneath needed? I created the workflows/readme.yml to test commands in the readme. But the readme needs to be cleaned up. I can do this after this PR is merged. Afterwards, I will cleanup this part to be the same as in the readme. Is this ok for your concept?
There was a problem hiding this comment.
Whether readme-validation is needed or not I can't evaluate. The goal with this PR is mainly to standardize Dockerfiles and their associated Files. Since readme-validation also builds Images based on these Images, I had to make changes here too. While at it I aggregated all CI related Workflows into a single Workflow File for better overview, since they had obscure names before that didn't properly reflect their purpose.
The readme and therefore the readme-validation may very well needs to be updated and you are free to do so and change whatever you want about the contents of this workflow after this PR merged, though I'd like to review these changes to some degree if possible
| @@ -1,46 +1,65 @@ | |||
| ARG CONTEXT=prod | |||
There was a problem hiding this comment.
This line has no effect. It is in the global scope and gets reset at FROM. See https://docs.docker.com/build/building/variables/#scoping
There was a problem hiding this comment.
That is not entirely true, it sets the default value for CONTEXT to be prod, so that the default target context is the production image. It will get reset with the next FROM, but because of the Line 6 'ARG CONTEXT' this global variable gets 'consumed' and is available for the build stage.
This was implemented with the intention that multiple different build stages can point to the same default value defined at the start of the Dockerfile instead of having multiple different default value declarations in the Dockerfile for each build stage. It may be unnecessarily verbose and is not specifically necessary for autoupdate, but I still do it like so here to uphold the same Dockerfile structure across all Services
| ARG CONTEXT | ||
|
|
||
| WORKDIR /app/openslides-autoupdate-service | ||
| ENV APP_CONTEXT=${CONTEXT} |
There was a problem hiding this comment.
Why is this necessary? The autoupdate-service does not read this environment variable
There was a problem hiding this comment.
This is a new standard variable that is set in every Dockerfile of each Service. It can be used to 'know' in what context you currently operate it and should absolve environment variables like OPENSLIDES_DEVELOPMENT.
It might not be necessary for autoupdate in specific, but it should still be there to have it available if needed.
| # Test build. | ||
| FROM base as testing | ||
| ## Healthcheck | ||
| HEALTHCHECK CMD ["/app/openslides-autoupdate-service/openslides-autoupdate-service", "health"] |
There was a problem hiding this comment.
The LABEL, EXPOSE and HEALTHCHECK defintions are confusing. In this current implementation, they are inherited to dev and tests but not to prod. tests does not need them, so the definition here is actually only used in dev.
Please move them to dev to make be more clear
|
|
||
| # Productive build | ||
| FROM scratch | ||
| WORKDIR / |
There was a problem hiding this comment.
This line has no effect and should be removed.
The prod image is FROM scratch. It does not inherit any values from other images. The default value for WORKDIR is /
There was a problem hiding this comment.
It is best practice to always declare the Workdir, even if it is /
| # Productive build | ||
| FROM scratch | ||
| WORKDIR / | ||
| ENV APP_CONTEXT=prod |
There was a problem hiding this comment.
Please remove this line. The autoupdate service never reads this variable
| build-dev: | ||
| docker build . --target development --tag openslides-autoupdate-dev | ||
| bash ../dev/scripts/makefile/build-service.sh $(SERVICE) dev |
There was a problem hiding this comment.
I really don't like this. The autoupdate-service is its own repo. This line expects to be called from the main repo. But if this line can only be called from the context of the main repo, this file should only exist in the main repo.
Please change the Makefile, so it also works, if only the autoupdate-service is checked out
mkdir only-autoupdate
cd only-autoupdate
git clone https://github.com/OpenSlides/openslides-autoupdate-service.git
cd openslides-autoupdate-service
make build-devThe same for every other command in this file
| @@ -0,0 +1,22 @@ | |||
| #!/bin/bash | |||
|
|
|||
| # Executes all tests. Should errors occur, CATCH will be set to 1, causing an erroneous exit code. | |||
There was a problem hiding this comment.
This comment is wrong. This file does not call all tests but only the tests, that does not need postgres.
Please make sure, that this script should not be used, if all tests should run
There was a problem hiding this comment.
This file is intended to run all tests. Can you please tell me which test commands are missing so that I can add them?
Reworked Dockerfile & Makefiles, so that there is one singular Dockerfile that supports development, testing and production images.
Also adding features like healthchecks and use of restricted app users, as well as fixing some bugs