Fix additional container options#19
Conversation
This adds a new spec test that will mount a local directory and have the docker container launched copy the `/etc/hosts` file to that directory. This is to ensure that volume mounts still work as expected (see #17) and that any custom @container_options passed in to Hoosegow that use the `:HostConfig` key get properly configured. In this case, we're testing that the `:ExtraHost` option properly adds the list of hostname/IP mappings to the containers `/etc/hosts` file.
This lets us take advantage of the ActiveSupport's `deep_merge` hash method to recursively merge hashes and not overwrite existing keys.
Activesupport's `deep_merge` hash method will recursively merge hashes so that duplicate keys will not overwrite each other. #17 introduced a change where we now configure volume binds at create time by using the `:HostConfig` option to do this. This makes it so that any other custom docker option that uses `:HostConfig` (such as `:ExtraHosts`) will get overwritten in the .merge in favor of the default volume `:Bind` configuration.
spraints
left a comment
spraints
left a comment
There was a problem hiding this comment.
Can this bug be fixed without bringing in activesupport? Adding it as a dependency will complicate the dependencies for pages and porter.
|
|
||
| it "configures ExtraHosts option" do | ||
| config = CONFIG.merge( | ||
| :Entrypoint => ['/bin/cp', '/etc/hosts', '/volume-test/hosts'], |
There was a problem hiding this comment.
Suggestion: I'm curious if the intent of this test is getting obscured by /etc/hosts and related config. Is /etc/hosts used as an example file so that the test can verify that something comes out on the volume? If so, would it make sense to use something that's easier to directly control? e.g. ['/bin/sh', '-c', 'echo THISISAMAGICTOKEN > /volume-test/out.txt'] or ['/bin/cp', '/hoosegow/inmate/inmate.rb', '/volume-test/inmate.rb'].
There was a problem hiding this comment.
Is /etc/hosts used as an example file so that the test can verify that something comes out on the volume?
The test serves two purposes: that the volume bind configuration introduced in #17 is not broken (we can copy any file into the mounted volume and read it outside of the container) and that we have successfully configured the :ExtraHosts option in the #create-a-container docker API. Below is the description for this option for the docker API under the :HostConfig key:
ExtraHosts - A list of hostnames/IP mappings to add to the container’s /etc/hosts file. Specified in the form ["hostname:IP"].
Another way to verify that the :ExtraHosts option is no longer being ignored and being properly set is to introduce a container_info method to directly check the running containers config:
# Public: Get more information about the running container
#
# Returns response body or nil if no container is running.
def container_info
return unless @container
@container.json
end
This would give us docker inspect functionality and we can easily check to see that any configuration we've set is present:
Can this bug be fixed without bringing in activesupport?
Definitely! Selectively merging into an existing :HostConfig option would be the way to do this. Let me try a few things and get something up shortly.
There was a problem hiding this comment.
The test serves two purposes: that the volume bind configuration introduced in #17 is not broken (we can copy any file into the mounted volume and read it outside of the container) and that we have successfully configured the :ExtraHosts option in the #create-a-container docker API.
Ah, word. I didn't realize there were two purposes. From a test purity perspective, it's better to check one thing per test. But for stuff like this sometimes it's easier to check just one thing. 👍 on using /etc/hosts if you'd like to keep just one test here.
- Pull out default container options into seperate method - Persist additional `:HostConfig` options by selectively merging into default `:HostConfig` options
This reverts commit 3eeb427.
- Remove volume bind check as we already test this in unit tests above - Instead of reading off of /etc/hosts, use getent to verify that we have added the entries indirectly. Docker can decide to change how they plug into the /etc/hosts file with a different format down the road.
2 participants
This PR fixes a bug where any additional container options passed in to Hoosegow that use the
:HostConfigkey gets ignored due to changes introduced in #17. #17 started using the:HostConfigkey for the:Bindsconfiguration option. If a user passes in another:HostConfigconfiguration, such asoptions = {:HostConfig => { :ExtraHosts => "test:123"}}, these options will be ignored in favor of the default:Bindsconfig when we.mergeagainst the default container options being used.