docker_container: fix handling of command and entrypoint in a backwards-compatible way

[felixfontein]

SUMMARY

Fixes #168.

Since this would be a breaking change, I'm adding a new option whose current default value is deprecated and will change in 2.0.0. The deprecation warning is only emitted if there would actually be a difference in behavior.

ISSUE TYPE

• Bugfix Pull Request
• Feature Pull Request

COMPONENT NAME

docker_container

[ChristianCiach]

When this is set to C(correct), these options are converted to strings considering shell quoting rules, and an empty value or empty list will be handled for idempotency checks, with the meaning "use default value from image".

Maybe I do not understand this correctly, but does that mean that it will be impossible to set the entrypoint or command to an actual empty value? Overriding the default entrypoint of an image with an empty value is a real use case.

[felixfontein]

When this is set to C(correct), these options are converted to strings considering shell quoting rules, and an empty value or empty list will be handled for idempotency checks, with the meaning "use default value from image".

Maybe I do not understand this correctly, but does that mean that it will be impossible to set the entrypoint or command to an actual empty value? Overriding the default entrypoint of an image with an empty value is a real use case.

I think my statement is wrong. Empty values should do what you expect. From how I understand the Docker daemon code, the behavior is as follows:

1. Container config values for command and entrypoint can be nil or a string array (https://github.com/docker/engine/blob/f07e53e0bb97c6364032bb14a020c50118eb7394/api/types/container/config.go#L55-L61, https://github.com/moby/moby/blob/f07e53e0bb97c6364032bb14a020c50118eb7394/api/types/strslice/strslice.go);
2. On merge (https://github.com/docker/engine/blob/f07e53e0bb97c6364032bb14a020c50118eb7394/daemon/commit.go#L68-L76):
   • If entrypoint is non-empty, both command and entrypoint are used.
   • If both entrypoint and command are nil or empty, the image's command is used.
   • If entrypoint is nil, the image's entrypoint is used.
3. After merging, if the entrypoint has length 1 and contains only an empty string, it is set to nil (https://github.com/docker/engine/blob/f07e53e0bb97c6364032bb14a020c50118eb7394/daemon/create.go#L277-L280).

(If someone is interested, https://github.com/docker/engine/blob/f07e53e0bb97c6364032bb14a020c50118eb7394/daemon/container.go#L190-L195 combines entrypoint and command to what is actually executed.)

So the code in the PR still allows to pass an empty list / string to docker daemon (that was possible before), and as opposed to the old code, it handles empty list / string correctly for idempotency (by not interpreting it as "not specified").

[ChristianCiach]

Excellent. This is exactly how I think this should behave. Thanks!

[felixfontein]

3. After merging, if the entrypoint has length 1 and contains only an empty string, it is set to nil (https://github.com/docker/engine/blob/f07e53e0bb97c6364032bb14a020c50118eb7394/daemon/create.go#L277-L280).

This case got me thinking, since right now it is not possible to pass this on to the Docker daemon, since the module converts everything to strings first.

I think I'll change it so that it works with lists instead, and does the list → string → list conversion for command_handling=compatibility for backwards compatibility.

[felixfontein]

ready_for_review

[briantist]

This looks good to me! The change in behavior makes sense, the documentation is clear. There's a way forward to pre-emptively choose the "correct" behavior in anticipation of a new version and advance warning of the default change. Tests look great and seem to cover all the bases.

As discussed in IRC, Ill leave it up to you to decide when to make the breaking change to the default value.

[briantist]

My only other small suggestion with these asserts is to maybe split them up and put them near the executions themselves. It does make it a bit messier, but it would relate the assert (and the expected outcome) better to the test being performed. As is, someone unfamiliar with the tests has to jump back and forth.

Or, rename the entrypoint register variables to something more verbose and specific, to help identify them down here.

But this is a weak suggestion and doesn't have to be done in this PR or at all; whatever you have here should (continue to) work well for the maintainers who actually work on it!

[felixfontein]

I'm tending to keep the tests as-is. If someone wants to rework all the docker_container tests, I won't stop them. Warning: it's a lot of work :)

[felixfontein]

@ChristianCiach @briantist thanks for your comments and reviews!