Add More Config Options to Docker Contrib Module #1456
Conversation
Add exposedPorts, exposedUdpPorts, volumes, envVars and user as overridable methods to DockerConfig trait.
|
|
||
| object DockerModuleTest extends TestSuite { | ||
|
|
||
| object Docker extends TestUtil.BaseModule with JavaModule with DockerModule { |
There was a problem hiding this comment.
I've written these based on the other contrib module test suites, let me know if there's anything untoward here.
There was a problem hiding this comment.
Thank you for your contribution! Your changes look good to me. About the tests: I personally find the call to docker a rather hard requirement to pass the tests. Additionally nowadays, using podman is quite common, which is a daemon-less alternative to docker. As these test won't pass on Windows either, it's probably a good idea to add a test which only checks the expected Dockerfile correctness, and make the actual docker call test conditional, e.g. only when a docker (or podman) executable can be found.
LaurenceWarne
force-pushed
the
add-more-docker-config-options2
branch
from
4d03bdf to
ba5e846
Compare
|
Thanks for taking a look! I've added tests against the dockerfile contents in ee6e601 and disabled the tests which invoke the docker executable in the case where it's not found.
Do you think we need to check for podman as well? https://podman.io/ recommends that you |
| val testModuleSourcesPath: Path = | ||
| os.pwd / "contrib" / "docker" / "test" / "resources" / "docker" | ||
|
|
||
| private def isInstalled(executable: String): Boolean = { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Do you think we need to check for podman as well? https://podman.io/ recommends that you
alias docker=podman, so the tests would still be ran in that case?
The alias only works in a shell, so either we explicitly check and implement support for podman, or not. I leave it to you to decide. But if we support it, this is probably worth a dedicated target, which is then also user configurable (in case both tools are present).
| os.makeDir.all(m.millSourcePath / os.up) | ||
| os.copy(testModuleSourcesPath, m.millSourcePath) | ||
| t(eval) | ||
| } else assert(true) |
There was a problem hiding this comment.
I'd like to see a println() indicating that docker tests are skipped here.
Add tests for a docker config with defaults (no options) and all options specified by building them (though only on machines with docker installed). Add tests which check the contents of the generated dockerfiles as a fallback.
LaurenceWarne
force-pushed
the
add-more-docker-config-options2
branch
from
ba5e846 to
9af2def
Compare
It's no problem, I've added a dedicated target in 9af2def. I've ran the tests against podman, docker and an environment without either, and everything looks ok 👍 |
|
I see a couple of the windows builds failed, currently 👀 |
LaurenceWarne
force-pushed
the
add-more-docker-config-options2
branch
from
9af2def to
2b70bdf
Compare
|
|
|
Disabling docker live tests on Windows is perfectly fine, I think. |
LaurenceWarne
force-pushed
the
add-more-docker-config-options2
branch
from
2b70bdf to
b7e35f7
Compare
LaurenceWarne
force-pushed
the
add-more-docker-config-options2
branch
from
048ee0f to
744c68d
Compare
|
Apologies, it's been a bit of a pain debugging some of the failures on the windows platforms, I think it should be good now though. |
|
@LaurenceWarne Don't worry, it's the same for me. That's one of the reasons we have CI on Windows. |
|
Thanks! |
Hi! This PR adds some more options (pretty much correspond 1-1 with Dockerfile instructions) that can be used in
DockerConfig. It's not totally comprehensive, but hopefully it catches the most common ones so there's less of a need to overrideDockerConfig.build.I've added some tests which validate the new options by calling
docker build- I could possibly replace these with tests against the generatedDockerfilestrings if you think this is too much.Thanks, I hope this is helpful.