docker_container - adding publish_all_ports option #162

Ajpantuso · 2021-06-25T22:37:17Z

SUMMARY

Adding publish_all_ports option to accurately reflect Docker CLI behavior.
Fixes #8

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

plugins/modules/docker_container.py

ADDITIONAL INFORMATION

The Docker API and Docker CLI combine the use of ports and publish_all_ports similarly, but the module logic was treating these as mutually exclusive values. This change simply splits the options such that the old way of publishing all ports is available until deprecation, but the new way will work as intended.

The following task:

 - hosts: localhost
   collections:
     - community.docker
   tasks:
     - docker_container:
         name: "nginx"
         image: nginx
         pull: yes
         publish_all_ports: true
         ports:
           - 8080:8080
         state: started
       become: true

now results in:

CONTAINER ID        IMAGE               COMMAND                  CREATED             STATUS              PORTS                                           NAMES
94798f1bf0bf        nginx               "/docker-entrypoint.…"   4 seconds ago       Up 3 seconds        0.0.0.0:8080->8080/tcp, 0.0.0.0:32793->80/tcp   nginx

felixfontein · 2021-06-26T08:53:50Z

You also have to update the deprecation message:

community.docker/plugins/modules/docker_container.py

Lines 1757 to 1763 in 18e6bd2

    
           self.client.module.deprecate( 
        
               'Specifying "all" in published_ports together with port mappings is not properly ' 
        
               'supported by the module. The port mappings are currently ignored. Please specify ' 
        
               'only port mappings, or the value "all". The behavior for mixed usage will either ' 
        
               'be forbidden in version 2.0.0, or properly handled. In any case, the way you ' 
        
               'currently use the module will change in a breaking way', 
        
               collection_name='community.docker', version='2.0.0')

.

Also using all in published_ports should not be allowed if publish_all_ports is set to true.

Ajpantuso · 2021-06-26T12:32:20Z

You also have to update the deprecation message:

Done.

Also using all in published_ports should not be allowed if publish_all_ports is set to true.

Module will now fail in this scenario.

felixfontein · 2021-06-26T12:57:44Z

Cool, thanks!

Can you add a test to tests/integration/targets/docker_container/tasks/tests/ports.yml which makes sure that published_ports: all is equivalent to publish_all_ports=true? (i.e. one more task in the published_ports: all series)

felixfontein · 2021-06-26T13:07:15Z

plugins/modules/docker_container.py

+      - Publish all ports to the host.
+      - Any specified port bindings from I(published_ports) will remain intact when C(true).
+    type: bool
+    default: false


Right now I'm not really happy with the default for publish_all_ports. This is only partially a problem with this PR though, since that behavior was already there before.

The problem is that right now, if the user does not specify published_ports, the published ports are not checked for idempotence - but the publish_all_ports flag is! (If a container has it set, and docker_container's published_ports isn't specified, the container will be recreated since there's a config mismatch!) I would argue that the current behavior is a bug, but potentially people depend on it...

The correct behavior would be (before this PR) to set publish_all_ports = None, and only set it to True/False if published_ports is passed to the module. With the new option it get's a bit more complicated, but it should also not be a problem.

The main problem is what do we do about the old problem. I'm currently tending to treat it as a bug and simply fix it. This changes behavior, but since it prevents container recreation in some situations, I think it's more acceptable than it would be the other way around. What do you think?

I tend to agree with you. Any predictable side-effect could be misunderstood as a feature, but the module already has a feature (recreate) to let users force recreation in a more obvious way if desired. In other words if they really needed the container to be recreated they should use the intended option otherwise the module is falsely reporting config changes which is a bug.

felixfontein

I think we should also have a set of integration tests for idempotency (and changed status) of publish_all_ports combined with published_ports.

felixfontein · 2021-06-26T13:39:15Z

plugins/modules/docker_container.py

        if self.published_ports == 'all':
-            self.publish_all_ports = True
-            self.published_ports = None
+            if not self.publish_all_ports:


Suggested change

if not self.publish_all_ports:

if self.publish_all_ports is None:

felixfontein · 2021-06-26T13:39:27Z

plugins/modules/docker_container.py

+                self.publish_all_ports = True
+                self.published_ports = None
+            else:
+                self.fail('"all" is not a valid value for "published_ports" when "publish_all_ports" is "true"')


Suggested change

self.fail('"all" is not a valid value for "published_ports" when "publish_all_ports" is "true"')

self.fail('"all" is not a valid value for "published_ports" when "publish_all_ports" is specified')

Ajpantuso · 2021-06-26T15:38:42Z

I think we should also have a set of integration tests for idempotency (and changed status) of publish_all_ports combined with published_ports.

Not sure if I understood fully, but I added tests that cover transitions between different combinations of these options.

felixfontein

Right now, you're only testing publish_all_ports not specified after publish_all_ports: false. You must also test publish_all_ports not specified after publish_all_ports: true, since that's the actually interesting and important case.

felixfontein · 2021-06-27T08:28:43Z

plugins/modules/docker_container.py

-                    'currently use the module will change in a breaking way',
+                    'supported by the module. The port mappings are currently ignored. Set publish_all_ports '
+                    'to "true" to randomly assign port mappings for those not specified by published_ports. '
+                    'The use of "all" in published_ports will be removed in version 2.0.0.',


Suggested change

'The use of "all" in published_ports will be removed in version 2.0.0.',

'The use of "all" in published_ports next to other values will be removed in version 2.0.0.',

all will still be allowed in 2.0.0. What we will do in 2.0.0 is deprecate all (with removal in 3.0.0). But that shouldn't be mentioned here.

felixfontein · 2021-06-27T08:31:45Z

tests/integration/targets/docker_container/tasks/tests/ports.yml

+    command: '/bin/sh -c "sleep 10m"'
+    name: "{{ cname }}"
+    state: started
+    publish_all_ports: false


Suggested change

publish_all_ports: false

Ajpantuso · 2021-06-27T14:04:59Z

Right now, you're only testing publish_all_ports not specified after publish_all_ports: false. You must also test publish_all_ports not specified after publish_all_ports: true, since that's the actually interesting and important case.

Ok, made the tests easier to read this time and 'true to unspecified' is now covered.

felixfontein · 2021-06-27T14:12:55Z

tests/integration/targets/docker_container/tasks/tests/ports.yml

-      - publish_all_ports_10 is not changed
-      - publish_all_ports_11 is not changed
+      - publish_all_ports.results[index].changed == publish_all_ports_test_cases[index].changed
+  loop: "{{ range(0, publish_all_ports_test_cases | length) | list }}"


For things like this, the indexed_items lookup is also pretty useful (https://docs.ansible.com/ansible/latest/collections/ansible/builtin/indexed_items_lookup.html#ansible-collections-ansible-builtin-indexed-items-lookup).

Thanks, I updated it to the loop analogue.

felixfontein · 2021-06-27T14:51:45Z

@Ajpantuso thanks a lot for implementing this!

Ajpantuso added 2 commits June 25, 2021 18:22

Initial commit

c2ac18f

Adding changelog fragment

18e6bd2

Ajpantuso marked this pull request as ready for review June 25, 2021 23:18

Updating deprecation notice

cf90e54

Adding integration test

7956550

felixfontein reviewed Jun 26, 2021

View reviewed changes

Applying second round of review suggestions

823207a

felixfontein reviewed Jun 27, 2021

View reviewed changes

Updating docs and cleaning up integration tests

9d5ec3c

felixfontein reviewed Jun 27, 2021

View reviewed changes

felixfontein approved these changes Jun 27, 2021

View reviewed changes

Updating test loop logic

db3486e

felixfontein approved these changes Jun 27, 2021

View reviewed changes

felixfontein merged commit 49cb513 into ansible-collections:main Jun 27, 2021

Ajpantuso deleted the docker_container_publish_all branch June 27, 2021 14:56

Ajpantuso mentioned this pull request Jun 30, 2021

docker-compose - Adding validated state and no_interpolate option #169

Closed

renovate bot mentioned this pull request Aug 3, 2021

chore(deps): update dependency community.docker to v1.9.0 onedr0p/home-ops#1846

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docker_container - adding publish_all_ports option #162

docker_container - adding publish_all_ports option #162

Ajpantuso commented Jun 25, 2021

felixfontein commented Jun 26, 2021

Ajpantuso commented Jun 26, 2021

felixfontein commented Jun 26, 2021

felixfontein Jun 26, 2021

Ajpantuso Jun 26, 2021

felixfontein left a comment

felixfontein Jun 26, 2021

felixfontein Jun 26, 2021

Ajpantuso commented Jun 26, 2021

felixfontein left a comment

felixfontein Jun 27, 2021

felixfontein Jun 27, 2021

Ajpantuso commented Jun 27, 2021

felixfontein Jun 27, 2021

Ajpantuso Jun 27, 2021 •

edited

Loading

felixfontein commented Jun 27, 2021

	if not self.publish_all_ports:
	if self.publish_all_ports is None:

	self.fail('"all" is not a valid value for "published_ports" when "publish_all_ports" is "true"')
	self.fail('"all" is not a valid value for "published_ports" when "publish_all_ports" is specified')

	'The use of "all" in published_ports will be removed in version 2.0.0.',
	'The use of "all" in published_ports next to other values will be removed in version 2.0.0.',

docker_container - adding publish_all_ports option #162

docker_container - adding publish_all_ports option #162

Conversation

Ajpantuso commented Jun 25, 2021

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

felixfontein commented Jun 26, 2021

Ajpantuso commented Jun 26, 2021

felixfontein commented Jun 26, 2021

felixfontein Jun 26, 2021

Choose a reason for hiding this comment

Ajpantuso Jun 26, 2021

Choose a reason for hiding this comment

felixfontein left a comment

Choose a reason for hiding this comment

felixfontein Jun 26, 2021

Choose a reason for hiding this comment

felixfontein Jun 26, 2021

Choose a reason for hiding this comment

Ajpantuso commented Jun 26, 2021

felixfontein left a comment

Choose a reason for hiding this comment

felixfontein Jun 27, 2021

Choose a reason for hiding this comment

felixfontein Jun 27, 2021

Choose a reason for hiding this comment

Ajpantuso commented Jun 27, 2021

felixfontein Jun 27, 2021

Choose a reason for hiding this comment

Ajpantuso Jun 27, 2021 • edited Loading

Choose a reason for hiding this comment

felixfontein commented Jun 27, 2021

Ajpantuso Jun 27, 2021 •

edited

Loading