feat: add Dockerfile and Docker build Workflow #14
Conversation
|
I don't have a lot of docker experience, so this is appreciated. Would it make sense to add a docker system test? If you have a look at then run the docker image and confirm queries work. I guess that would require all devs who run the tests to have docker installed? Not great? What is good practice here? also using the binary package means we are not testing the latest code. That means this "docker test" is a containerisation test only, not a code test. Therefore should be a separate test, which is run separately from |
|
Hi! In that case, you are testing the binary build itself; packing it into Docker will behave exactly as built, so running tests will just generate the same duplicated result, no differences. There are multiple approaches to follow as you want:
Right now, the Docker implementation just picks the |
|
Yes understand all that. The code is well tested on a range of platforms. The only thing that needs additional test is the containerisation. What is your intended use case for the docker image? is there a sensible way to test that the docker image does what it says on the tin? I guess writing the docs for what the docker image does, should specify what the test should be? |
|
With f271258 now it will build locally, requires having |
|
The idea of having it containerized, is basically to |
|
Let me prepare a couple of scripts then... |
|
right ok.. I agree with that. I als agree that raspberry pi and also firewall/openwall/router type devices are interesting deployment platforms. raspberry pi is arm, so the x64 .deb won't work, so recompiling from source is required. But would it not make more sense to add raspberry pi as a supported hardware platform first, on which the github actions build in CI, and then add a docker wrapper? dealing with build errors through another layer of abstraction prob doesn't help. So it comes back to what is the purpose of the docker image. Which platforms etc.. ? What's your view? |
|
The workflow now will build the Docker image on merge (push to There is currently a build issue with |
|
arm build should now hopefully work just update your fork |
|
|
duhow
changed the title
There was a problem hiding this comment.
overall this is looking great. thanks.
it will make deploying hibp-server much easier on a range of platforms, which is what we want.
if you could address the line specific comments/questions, and maybe add some readme docs in the "getting started and/or build" section. that would be great/
| # Capture TERM signal and execute function | ||
| trap _term SIGTERM | ||
|
|
||
| hibp-server --bind-address 0.0.0.0 ${ARGS} ${EXTRA_ARGS} & |
There was a problem hiding this comment.
I don't think we should be binding to all addresses...
this is http without SSL. I think by default it should bind to localhost (which is the default for hibp-server).
|
|
||
| # Partial database download | ||
| if [ ! -f "$PWD/data/hibp_all.sha1.bin" ]; then | ||
| $DOCKER_RUN $IMAGE_NAME hibp-download hibp_all.sha1.bin --limit 256 --no-progress |
There was a problem hiding this comment.
OK, I like that it's partial (--limit 256) for a test.
as I tried to explain earlier, we have a "tiny local cache" already committed to the repo. It is used in test/system_test.sh it is served with a custom restinio server
that cache takes the place of the online API from api.pwnedpasswords.com
it can be started as seen in system_test.sh and the download then needs to have --testing on it.
do you agree this is preferable as the docker test is then not reliant on some online resource and can be rune entirely locally?
| @@ -0,0 +1,67 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
I assume the idea that this will not be run during Ctest (at end of cmake build), because it it probably a bit heavy for that?
Build a Docker image using the
.debinstaller, and add a small entrypoint script to start the server, and download the DB if not previously available.