docker_swarm_service: Remove configs and secrets defaults (#54361)

* Remove defaults

* Skip redundant casting

* Indentation fix

* Use generic compare functions

* Add tests for compare functions

* Remove extra whitespace

* Add changelog fragment

changelogs/fragments/54361-docker_swarm_service-remove-secrets-configs-defaults.yaml

@@ -0,0 +1,2 @@
+bugfixes:
+  - "docker_swarm_service - Removed redundant defaults for ``uid``, ``gid``, and ``mode`` from ``configs`` and ``secrets``."

lib/ansible/modules/cloud/docker/docker_swarm_service.py

@@ -59,17 +59,14 @@ options:
         description:
           - UID of the config file's owner.
         type: int
-        default: 0
       gid:
         description:
           - GID of the config file's group.
         type: int
-        default: 0
       mode:
         description:
           - File access mode inside the container.
         type: str
-        default: "0o444"
   constraints:
     description:
       - List of the service constraints.
@@ -615,17 +612,14 @@ options:
         description:
           - UID of the secret file's owner.
         type: int
-        default: 0
       gid:
         description:
           - GID of the secret file's group.
         type: int
-        default: 0
       mode:
         description:
           - File access mode inside the container.
         type: int
-        default: 0o444
   state:
     description:
       - Service state.
@@ -1148,6 +1142,42 @@ def get_value(key, values, default=None):
     return value if value is not None else default
 
 
+def has_dict_changed(new_dict, old_dict):
+    """
+    Check if new_dict has differences compared to old_dict while
+    ignoring keys in old_dict which are None in new_dict.
+    """
+    if new_dict is None:
+        return False
+    if not new_dict and old_dict:
+        return True
+    if not old_dict and new_dict:
+        return True
+    defined_options = dict(
+        (option, value) for option, value in new_dict.items()
+        if value is not None
+    )
+    for option, value in defined_options.items():
+        if value != old_dict.get(option):
+            return True
+    return False
+
+
+def has_list_of_dicts_changed(new_list, old_list):
+    """
+    Check two lists of dicts has differences.
+    """
+    if new_list is None:
+        return False
+    old_list = old_list or []
+    if len(new_list) != len(old_list):
+        return True
+    for new_item, old_item in zip(new_list, old_list):
+        if has_dict_changed(new_item, old_item):
+            return True
+    return False
+
+
 class DockerService(DockerBaseClass):
     def __init__(self):
         super(DockerService, self).__init__()
@@ -1620,11 +1650,11 @@ class DockerService(DockerBaseClass):
         if self.mode != os.mode:
             needs_rebuild = True
             differences.add('mode', parameter=self.mode, active=os.mode)
-        if self.mounts is not None and self.mounts != (os.mounts or []):
+        if has_list_of_dicts_changed(self.mounts, os.mounts):
             differences.add('mounts', parameter=self.mounts, active=os.mounts)
-        if self.configs is not None and self.configs != (os.configs or []):
+        if has_list_of_dicts_changed(self.configs, os.configs):
             differences.add('configs', parameter=self.configs, active=os.configs)
-        if self.secrets is not None and self.secrets != (os.secrets or []):
+        if has_list_of_dicts_changed(self.secrets, os.secrets):
             differences.add('secrets', parameter=self.secrets, active=os.secrets)
         if self.networks is not None and self.networks != (os.networks or []):
             differences.add('networks', parameter=self.networks, active=os.networks)
@@ -1669,7 +1699,7 @@ class DockerService(DockerBaseClass):
             differences.add('restart_policy_delay', parameter=self.restart_policy_delay, active=os.restart_policy_delay)
         if self.restart_policy_window is not None and self.restart_policy_window != os.restart_policy_window:
             differences.add('restart_policy_window', parameter=self.restart_policy_window, active=os.restart_policy_window)
-        if self.rollback_config is not None and self.has_rollback_config_changed(os.rollback_config):
+        if has_dict_changed(self.rollback_config, os.rollback_config):
             differences.add('rollback_config', parameter=self.rollback_config, active=os.rollback_config)
         if self.update_delay is not None and self.update_delay != os.update_delay:
             differences.add('update_delay', parameter=self.update_delay, active=os.update_delay)
@@ -1744,18 +1774,6 @@ class DockerService(DockerBaseClass):
             old_image = old_image.split('@')[0]
         return self.image != old_image, old_image
 
-    def has_rollback_config_changed(self, old_rollback_config):
-        if old_rollback_config is None and self.rollback_config:
-            return True
-        defined_options = dict(
-            (option, value) for option, value in self.rollback_config.items()
-            if value is not None
-        )
-        for option, value in defined_options.items():
-            if value != old_rollback_config.get(option):
-                return True
-        return False
-
     def __str__(self):
         return str({
             'mode': self.mode,
@@ -1997,14 +2015,14 @@ class DockerService(DockerBaseClass):
             ports = {}
             for port in self.publish:
                 if port.get('mode'):
-                    ports[int(port['published_port'])] = (
+                    ports[port['published_port']] = (
-                        int(port['target_port']),
+                        port['target_port'],
                         port['protocol'],
                         port['mode'],
                     )
                 else:
-                    ports[int(port['published_port'])] = (
+                    ports[port['published_port']] = (
-                        int(port['target_port']),
+                        port['target_port'],
                         port['protocol'],
                     )
             endpoint_spec_args['ports'] = ports
@@ -2470,17 +2488,17 @@ def main():
             config_id=dict(type='str', required=True),
             config_name=dict(type='str', required=True),
             filename=dict(type='str'),
-            uid=dict(type='int', default=0),
+            uid=dict(type='int'),
-            gid=dict(type='int', default=0),
+            gid=dict(type='int'),
-            mode=dict(type='int', default=0o444),
+            mode=dict(type='int'),
         )),
         secrets=dict(type='list', elements='dict', options=dict(
             secret_id=dict(type='str', required=True),
             secret_name=dict(type='str', required=True),
             filename=dict(type='str'),
-            uid=dict(type='int', default=0),
+            uid=dict(type='int'),
-            gid=dict(type='int', default=0),
+            gid=dict(type='int'),
-            mode=dict(type='int', default=0o444),
+            mode=dict(type='int'),
         )),
         networks=dict(type='list', elements='str'),
         command=dict(type='raw'),

test/integration/targets/docker_swarm_service/tasks/tests/options.yml

@@ -1062,7 +1062,7 @@
     command: '/bin/sh -v -c "sleep 10m"'
     healthcheck:
       test:
-      - NONE
+        - NONE
   register: healthcheck_5
   ignore_errors: yes
 

test/units/modules/cloud/docker/test_docker_swarm_service.py

@@ -112,3 +112,163 @@ def test_get_nanoseconds_from_raw_option(docker_swarm_service):
 
     with pytest.raises(ValueError):
         docker_swarm_service.get_nanoseconds_from_raw_option('test', [])
+
+
+def test_has_dict_changed(docker_swarm_service):
+    assert not docker_swarm_service.has_dict_changed(
+        {"a": 1},
+        {"a": 1},
+    )
+    assert not docker_swarm_service.has_dict_changed(
+        {"a": 1},
+        {"a": 1, "b": 2}
+    )
+    assert docker_swarm_service.has_dict_changed(
+        {"a": 1},
+        {"a": 2, "b": 2}
+    )
+    assert docker_swarm_service.has_dict_changed(
+        {"a": 1, "b": 1},
+        {"a": 1}
+    )
+    assert not docker_swarm_service.has_dict_changed(
+        None,
+        {"a": 2, "b": 2}
+    )
+    assert docker_swarm_service.has_dict_changed(
+        {},
+        {"a": 2, "b": 2}
+    )
+    assert docker_swarm_service.has_dict_changed(
+        {"a": 1},
+        {}
+    )
+    assert docker_swarm_service.has_dict_changed(
+        {"a": 1},
+        None
+    )
+    assert not docker_swarm_service.has_dict_changed(
+        {},
+        {}
+    )
+    assert not docker_swarm_service.has_dict_changed(
+        None,
+        None
+    )
+    assert not docker_swarm_service.has_dict_changed(
+        {},
+        None
+    )
+    assert not docker_swarm_service.has_dict_changed(
+        None,
+        {}
+    )
+
+
+def test_has_list_of_dicts_changed(docker_swarm_service):
+    assert docker_swarm_service.has_list_of_dicts_changed(
+        [
+            {"a": 1},
+            {"b": 1}
+        ],
+        [
+            {"a": 1}
+        ]
+    )
+    assert docker_swarm_service.has_list_of_dicts_changed(
+        [
+            {"a": 1},
+        ],
+        [
+            {"a": 1},
+            {"b": 1},
+        ]
+    )
+    assert not docker_swarm_service.has_list_of_dicts_changed(
+        [
+            {"a": 1},
+            {"b": 1},
+        ],
+        [
+            {"a": 1},
+            {"b": 1}
+        ]
+    )
+    assert not docker_swarm_service.has_list_of_dicts_changed(
+        None,
+        [
+            {"b": 1},
+            {"a": 1}
+        ]
+    )
+    assert docker_swarm_service.has_list_of_dicts_changed(
+        [],
+        [
+            {"b": 1},
+            {"a": 1}
+        ]
+    )
+    assert not docker_swarm_service.has_list_of_dicts_changed(
+        None,
+        None
+    )
+    assert not docker_swarm_service.has_list_of_dicts_changed(
+        [],
+        None
+    )
+    assert not docker_swarm_service.has_list_of_dicts_changed(
+        None,
+        []
+    )
+    assert not docker_swarm_service.has_list_of_dicts_changed(
+        [
+            {"src": 1, "dst": 2},
+            {"src": 1, "dst": 2, "protocol": "udp"},
+        ],
+        [
+            {"src": 1, "dst": 2, "protocol": "tcp"},
+            {"src": 1, "dst": 2, "protocol": "udp"},
+        ]
+    )
+    assert not docker_swarm_service.has_list_of_dicts_changed(
+        [
+            {"src": 1, "dst": 2, "protocol": "udp"},
+            {"src": 1, "dst": 3, "protocol": "tcp"},
+        ],
+        [
+            {"src": 1, "dst": 2, "protocol": "udp"},
+            {"src": 1, "dst": 3, "protocol": "tcp"},
+        ]
+    )
+    assert docker_swarm_service.has_list_of_dicts_changed(
+        [
+            {"src": 1, "dst": 2, "protocol": "udp"},
+            {"src": 1, "dst": 2},
+            {"src": 3, "dst": 4},
+        ],
+        [
+            {"src": 1, "dst": 3, "protocol": "udp"},
+            {"src": 1, "dst": 2, "protocol": "tcp"},
+            {"src": 3, "dst": 4, "protocol": "tcp"},
+        ]
+    )
+    assert docker_swarm_service.has_list_of_dicts_changed(
+        [
+            {"src": 1, "dst": 3, "protocol": "tcp"},
+            {"src": 1, "dst": 2, "protocol": "udp"},
+        ],
+        [
+            {"src": 1, "dst": 2, "protocol": "tcp"},
+            {"src": 1, "dst": 2, "protocol": "udp"},
+        ]
+    )
+    assert docker_swarm_service.has_list_of_dicts_changed(
+        [
+            {"src": 1, "dst": 2, "protocol": "udp"},
+            {"src": 1, "dst": 2, "protocol": "tcp", "extra": {"test": "foo"}},
+        ],
+        [
+            {"src": 1, "dst": 2, "protocol": "udp"},
+            {"src": 1, "dst": 2, "protocol": "tcp"},
+        ]
+    )