Improve ports parsing #418
Conversation
Generate changelog in
|
|
This one is ready for review @CRogers! We need to re-trigger changelog-bot somehow, it should be |
| public static Ports parseFromDockerComposePs(String psOutput, String dockerMachineIp) { | ||
| Preconditions.checkArgument(!Strings.isNullOrEmpty(psOutput), "No container found"); | ||
| Matcher matcher = PORT_PATTERN.matcher(psOutput); | ||
| public static Ports parseFromPortInformation(String portInformation, String dockerMachineIp) { |
There was a problem hiding this comment.
Unfortunately nearly everything in DCR is public rather than package-private - so we should do some quick github searches to find out if anyone is using this internally or externally before making such a breaking change.
There was a problem hiding this comment.
Fortunately in this case it does look safe, but worth bearing in mind!
There was a problem hiding this comment.
Ah yeah, missed that one, sorry. Can include explicit comments about usage / impact of the break in the future
| return psOutput; | ||
| private static ErrorHandler swallowingServiceDoesNotExist() { | ||
| return (exitCode, output, commandName, commands) -> { | ||
| if (exitCode == 1 && output.isEmpty()) { |
There was a problem hiding this comment.
Why exit code 1 and not all exit codes? Will this silently fail if another exit code is thrown?
There was a problem hiding this comment.
Yeah this is pretty specific, its really only trying to add debug information for when docker can't find a service
~/code/foundry-academy-backend
$ docker-compose ps some_service
ERROR: No such service: some_service
~/code/foundry-academy-backend
$ echo $?
1
Seems it would be more proper for this function to check for various failure exit codes and throw an exception explicitly, while still providing output with suggestions around what the potential failure mode was?
| String serviceName = commands[commands.length - 1]; | ||
|
|
||
| log.warn("It looks like `{}` failed.", fullCommand); | ||
| log.warn("This probably happened because no `{}` service exists.", serviceName); |
There was a problem hiding this comment.
Shouldn't this throw rather than silently continue? I imagine we can't recover from this?
There was a problem hiding this comment.
If that output is produced, the command will have failed with empty output and then will trigger an exception right after it here: https://github.com/palantir/docker-compose-rule/pull/418/files#diff-cf7a91b7706a65a0c4cc650e69cf5d81R242
I agree that maybe a little misleading and we could clarify how swallowingServiceDoesNotExist handles the case
| this.command = new Command(rawExecutable, log::trace); | ||
| public DefaultDockerCompose( | ||
| DockerComposeExecutable dockerComposeRawExecutable, | ||
| DockerExecutable dockerRawExecutable, |
There was a problem hiding this comment.
This also seems safe from a brief github search (I think 1 person has used it externally and none internally)
There was a problem hiding this comment.
Yeah it seems reasonable enough, will add comments about break-impact next time!
| } | ||
|
|
||
| @Test | ||
| public void parse_the_ps_output_on_ports() throws IOException, InterruptedException { | ||
| public void get_correct_ports() throws IOException, InterruptedException { |
There was a problem hiding this comment.
Not sure about the value of this test - it mocks out a lot of behaviour and I wonder if an integeration test against docker or just testing the parsing directly would be more valuable.
There was a problem hiding this comment.
I could trade this one out for an integration test, it would be easy enough to create a docker-compose.yml with various port setups and then check all the parsing without mocking. Will add that
| } | ||
|
|
||
| @Test | ||
| public void attempt_to_get_ports_but_all_ports_are_open_for_container() throws IOException, InterruptedException { |
There was a problem hiding this comment.
Again here there is so much mocking I'm not sure what this is actually testing?
There was a problem hiding this comment.
Will move to an integration test, but this one checks if you specified no ports for your container. An integration test should make this more clear
|
Swapped out the ports unit tests for integration tests. Not sure why the Otherwise, will fix the error handling mentioned above with whatever we agree upon |
|
While I think this is nicer from a code point of view, it does have a couple of breaks (including using |
Before this PR
This PR fixes: #411
TLDR: Getting port information for various services depends heavily on well white-spaced output from
docker-compose ps <service_name>and regex, however this quickly breaks when thepscommand output gets truncated as more information gets added to thepsoutput (health checks, etc).After this PR
Instead of applying a regex to unreliable output from
docker-compose ps <service_name>to get port information, we now:docker-compose ps -q <service_name>docker ps --no-trunc --format {{.Ports}} --filter id=<container_id>This approach uses docker commands to do more heavily lifting and only return the specific information (ports) that we care about
==COMMIT_MSG==
Improve port parsing by leveraging more specific docker commands.
==COMMIT_MSG==
Possible downsides?
There is technically a dev break because I modified one of the
DefaultDockerComposeconstructors to take in aDockerExecutable.