Add environment variable substitution to docker compose parsing - #3628
Add environment variable substitution to docker compose parsing#3628mpdn wants to merge 2 commits into
Conversation
rnorth
changed the title
rnorth
left a comment
rnorth
left a comment
There was a problem hiding this comment.
Thanks for this, I think it makes sense but I've suggested a couple of things.
Just to note, we wanted to keep our parsing of docker-compose/Dockerfiles as barebones as possible, as we really don't want to have a lot of implementation in parallel with the Docker userland tools. From my perspective this PR is OK but about as far as we should go.
| this.composeFileContent = testContent; | ||
| ParsedDockerComposeFile(Map<String, Object> testContent, Map<String, String> env) { | ||
| //noinspection unchecked | ||
| this.composeFileContent = (Map<String, Object>) substituteEnv(testContent, env); |
There was a problem hiding this comment.
Would it be possible to use Apache Commons StringSubstitutor instead?
At a glance your implementation of substituteEnv looks decent, but it would be good to avoid having our own implementation of placeholder substitution from a maintenance/verification perspective.
There was a problem hiding this comment.
Using StringSubtitutor could be possible, but it seems to have a number of differences:
- It behaves like
-(replacing if the variable is set, even if empty). You can't really get the same behavior as:-as described in the docker docs. - Matching multiple different operators seem possible, albeit slightly weird.
| Assertions | ||
| .assertThatThrownBy(() -> { | ||
| new ParsedDockerComposeFile(file); | ||
| new ParsedDockerComposeFile(file, emptyMap()); |
There was a problem hiding this comment.
Perhaps it would be worth having a constructor that omits the Map parameter, at least for testing purposes?
There was a problem hiding this comment.
I'm not sure. Having it as a parameter forces the user of the class to consider the env. vars (if any) to substitute with. A developer might not otherwise consider that parsing a compose file depends on environment variables.
But I can add a constructor variant defaulting to no env. vars if you disagree with this.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this. |
2 participants
Adds environment variable substitution (as described here) to
ParsedDockerComposeFile.This also means compose file parsing is deferred until the container is started.