From 73533d3fc2b4be0a07ea602c8461f426de3a1225 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Tue, 30 Oct 2018 09:50:34 +0100 Subject: [PATCH] docker_* modules: simplify idempotency comparisons (#47709) * More generic comparison code from docker_container to docker_common. * More flexibility if a is None and method is allow_to_present. Note that this odes not affect docker_container, as there a is never None. * Update docker_secret and docker_config: simplify labels comparison. * Added unit tests. * Use proper subsequence test for allow_more_present for lists. Note that this does not affect existing code in docker_container, since lists don't use allow_more_present. Using allow_more_present will only be possible in Ansible 2.8. * pep8 --- lib/ansible/module_utils/docker_common.py | 105 ++++ .../modules/cloud/docker/docker_config.py | 20 +- .../modules/cloud/docker/docker_container.py | 71 +-- .../modules/cloud/docker/docker_secret.py | 20 +- test/units/module_utils/test_docker_common.py | 464 ++++++++++++++++++ 5 files changed, 577 insertions(+), 103 deletions(-) create mode 100644 test/units/module_utils/test_docker_common.py diff --git a/lib/ansible/module_utils/docker_common.py b/lib/ansible/module_utils/docker_common.py index bac7630c614..3743dd271c1 100644 --- a/lib/ansible/module_utils/docker_common.py +++ b/lib/ansible/module_utils/docker_common.py @@ -541,3 +541,108 @@ class AnsibleDockerClient(Client): new_tag = self.find_image(name, tag) return new_tag, old_tag == new_tag + + +def compare_dict_allow_more_present(av, bv): + ''' + Compare two dictionaries for whether every entry of the first is in the second. + ''' + for key, value in av.items(): + if key not in bv: + return False + if bv[key] != value: + return False + return True + + +def compare_generic(a, b, method, type): + ''' + Compare values a and b as described by method and type. + + Returns ``True`` if the values compare equal, and ``False`` if not. + + ``a`` is usually the module's parameter, while ``b`` is a property + of the current object. ``a`` must not be ``None`` (except for + ``type == 'value'``). + + Valid values for ``method`` are: + - ``ignore`` (always compare as equal); + - ``strict`` (only compare if really equal) + - ``allow_more_present`` (allow b to have elements which a does not have). + + Valid values for ``type`` are: + - ``value``: for simple values (strings, numbers, ...); + - ``list``: for ``list``s or ``tuple``s where order matters; + - ``set``: for ``list``s, ``tuple``s or ``set``s where order does not + matter; + - ``set(dict)``: for ``list``s, ``tuple``s or ``sets`` where order does + not matter and which contain ``dict``s; ``allow_more_present`` is used + for the ``dict``s, and these are assumed to be dictionaries of values; + - ``dict``: for dictionaries of values. + ''' + if method == 'ignore': + return True + # If a or b is None: + if a is None or b is None: + # If both are None: equality + if a == b: + return True + # Otherwise, not equal for values, and equal + # if the other is empty for set/list/dict + if type == 'value': + return False + # For allow_more_present, allow a to be None + if method == 'allow_more_present' and a is None: + return True + # Otherwise, the iterable object which is not None must have length 0 + return len(b if a is None else a) == 0 + # Do proper comparison (both objects not None) + if type == 'value': + return a == b + elif type == 'list': + if method == 'strict': + return a == b + else: + i = 0 + for v in a: + while i < len(b) and b[i] != v: + i += 1 + if i == len(b): + return False + i += 1 + return True + elif type == 'dict': + if method == 'strict': + return a == b + else: + return compare_dict_allow_more_present(a, b) + elif type == 'set': + set_a = set(a) + set_b = set(b) + if method == 'strict': + return set_a == set_b + else: + return set_b >= set_a + elif type == 'set(dict)': + for av in a: + found = False + for bv in b: + if compare_dict_allow_more_present(av, bv): + found = True + break + if not found: + return False + if method == 'strict': + # If we would know that both a and b do not contain duplicates, + # we could simply compare len(a) to len(b) to finish this test. + # We can assume that b has no duplicates (as it is returned by + # docker), but we don't know for a. + for bv in b: + found = False + for av in a: + if compare_dict_allow_more_present(av, bv): + found = True + break + if not found: + return False + return True diff --git a/lib/ansible/modules/cloud/docker/docker_config.py b/lib/ansible/modules/cloud/docker/docker_config.py index 453ba0eab42..f123f253ca6 100644 --- a/lib/ansible/modules/cloud/docker/docker_config.py +++ b/lib/ansible/modules/cloud/docker/docker_config.py @@ -152,7 +152,7 @@ except ImportError: # missing docker-py handled in ansible.module_utils.docker pass -from ansible.module_utils.docker_common import AnsibleDockerClient, DockerBaseClass +from ansible.module_utils.docker_common import AnsibleDockerClient, DockerBaseClass, compare_generic from ansible.module_utils._text import to_native, to_bytes @@ -224,22 +224,8 @@ class ConfigManager(DockerBaseClass): if attrs.get('Labels', {}).get('ansible_key'): if attrs['Labels']['ansible_key'] != self.data_key: data_changed = True - labels_changed = False - if self.labels and attrs.get('Labels'): - # check if user requested a label change - for label in attrs['Labels']: - if self.labels.get(label) and self.labels[label] != attrs['Labels'][label]: - labels_changed = True - # check if user added a label - labels_added = False - if self.labels: - if attrs.get('Labels'): - for label in self.labels: - if label not in attrs['Labels']: - labels_added = True - else: - labels_added = True - if data_changed or labels_added or labels_changed or self.force: + labels_changed = not compare_generic(self.labels, attrs.get('Labels'), 'allow_more_present', 'dict') + if data_changed or labels_changed or self.force: # if something changed or force, delete and re-create the config self.absent() config_id = self.create_config() diff --git a/lib/ansible/modules/cloud/docker/docker_container.py b/lib/ansible/modules/cloud/docker/docker_container.py index 5b1453d8900..573c28228d9 100644 --- a/lib/ansible/modules/cloud/docker/docker_container.py +++ b/lib/ansible/modules/cloud/docker/docker_container.py @@ -753,6 +753,7 @@ from ansible.module_utils.basic import human_to_bytes from ansible.module_utils.docker_common import ( HAS_DOCKER_PY_2, HAS_DOCKER_PY_3, AnsibleDockerClient, DockerBaseClass, sanitize_result, is_image_name_id, + compare_generic, ) from ansible.module_utils.six import string_types @@ -1537,79 +1538,11 @@ class Container(DockerBaseClass): return True return False - def _compare_dict_allow_more_present(self, av, bv): - ''' - Compare two dictionaries for whether every entry of the first is in the second. - ''' - for key, value in av.items(): - if key not in bv: - return False - if bv[key] != value: - return False - return True - def _compare(self, a, b, compare): ''' Compare values a and b as described in compare. ''' - method = compare['comparison'] - if method == 'ignore': - return True - # If a or b is None: - if a is None or b is None: - # If both are None: equality - if a == b: - return True - # Otherwise, not equal for values, and equal - # if the other is empty for set/list/dict - if compare['type'] == 'value': - return False - return len(b if a is None else a) == 0 - # Do proper comparison (both objects not None) - if compare['type'] == 'value': - return a == b - elif compare['type'] == 'list': - if method == 'strict': - return a == b - else: - set_a = set(a) - set_b = set(b) - return set_b >= set_a - elif compare['type'] == 'dict': - if method == 'strict': - return a == b - else: - return self._compare_dict_allow_more_present(a, b) - elif compare['type'] == 'set': - set_a = set(a) - set_b = set(b) - if method == 'strict': - return set_a == set_b - else: - return set_b >= set_a - elif compare['type'] == 'set(dict)': - for av in a: - found = False - for bv in b: - if self._compare_dict_allow_more_present(av, bv): - found = True - break - if not found: - return False - if method == 'strict': - # If we would know that both a and b do not contain duplicates, - # we could simply compare len(a) to len(b) to finish this test. - # We can assume that b has no duplicates (as it is returned by - # docker), but we don't know for a. - for bv in b: - found = False - for av in a: - if self._compare_dict_allow_more_present(av, bv): - found = True - break - if not found: - return False - return True + return compare_generic(a, b, compare['comparison'], compare['type']) def has_different_configuration(self, image): ''' diff --git a/lib/ansible/modules/cloud/docker/docker_secret.py b/lib/ansible/modules/cloud/docker/docker_secret.py index 1e307a242b2..9a9af055e7c 100644 --- a/lib/ansible/modules/cloud/docker/docker_secret.py +++ b/lib/ansible/modules/cloud/docker/docker_secret.py @@ -147,7 +147,7 @@ except ImportError: # missing docker-py handled in ansible.module_utils.docker_common pass -from ansible.module_utils.docker_common import AnsibleDockerClient, DockerBaseClass +from ansible.module_utils.docker_common import AnsibleDockerClient, DockerBaseClass, compare_generic from ansible.module_utils._text import to_native, to_bytes @@ -219,22 +219,8 @@ class SecretManager(DockerBaseClass): if attrs.get('Labels', {}).get('ansible_key'): if attrs['Labels']['ansible_key'] != self.data_key: data_changed = True - labels_changed = False - if self.labels and attrs.get('Labels'): - # check if user requested a label change - for label in attrs['Labels']: - if self.labels.get(label) and self.labels[label] != attrs['Labels'][label]: - labels_changed = True - # check if user added a label - labels_added = False - if self.labels: - if attrs.get('Labels'): - for label in self.labels: - if label not in attrs['Labels']: - labels_added = True - else: - labels_added = True - if data_changed or labels_added or labels_changed or self.force: + labels_changed = not compare_generic(self.labels, attrs.get('Labels'), 'allow_more_present', 'dict') + if data_changed or labels_changed or self.force: # if something changed or force, delete and re-create the secret self.absent() secret_id = self.create_secret() diff --git a/test/units/module_utils/test_docker_common.py b/test/units/module_utils/test_docker_common.py new file mode 100644 index 00000000000..7e4e9e24390 --- /dev/null +++ b/test/units/module_utils/test_docker_common.py @@ -0,0 +1,464 @@ +import pytest + +from ansible.module_utils.docker_common import ( + compare_dict_allow_more_present, + compare_generic, +) + +DICT_ALLOW_MORE_PRESENT = ( + { + 'av': {}, + 'bv': {'a': 1}, + 'result': True + }, + { + 'av': {'a': 1}, + 'bv': {'a': 1, 'b': 2}, + 'result': True + }, + { + 'av': {'a': 1}, + 'bv': {'b': 2}, + 'result': False + }, + { + 'av': {'a': 1}, + 'bv': {'a': None, 'b': 1}, + 'result': False + }, + { + 'av': {'a': None}, + 'bv': {'b': 1}, + 'result': False + }, +) + +COMPARE_GENERIC = [ + ######################################################################################## + # value + { + 'a': 1, + 'b': 2, + 'method': 'strict', + 'type': 'value', + 'result': False + }, + { + 'a': 'hello', + 'b': 'hello', + 'method': 'strict', + 'type': 'value', + 'result': True + }, + { + 'a': None, + 'b': 'hello', + 'method': 'strict', + 'type': 'value', + 'result': False + }, + { + 'a': None, + 'b': None, + 'method': 'strict', + 'type': 'value', + 'result': True + }, + { + 'a': 1, + 'b': 2, + 'method': 'ignore', + 'type': 'value', + 'result': True + }, + { + 'a': None, + 'b': 2, + 'method': 'ignore', + 'type': 'value', + 'result': True + }, + ######################################################################################## + # list + { + 'a': [ + 'x', + ], + 'b': [ + 'y', + ], + 'method': 'strict', + 'type': 'list', + 'result': False + }, + { + 'a': [ + 'x', + ], + 'b': [ + 'x', + 'x', + ], + 'method': 'strict', + 'type': 'list', + 'result': False + }, + { + 'a': [ + 'x', + 'y', + ], + 'b': [ + 'x', + 'y', + ], + 'method': 'strict', + 'type': 'list', + 'result': True + }, + { + 'a': [ + 'x', + 'y', + ], + 'b': [ + 'y', + 'x', + ], + 'method': 'strict', + 'type': 'list', + 'result': False + }, + { + 'a': [ + 'x', + 'y', + ], + 'b': [ + 'x', + ], + 'method': 'allow_more_present', + 'type': 'list', + 'result': False + }, + { + 'a': [ + 'x', + ], + 'b': [ + 'x', + 'y', + ], + 'method': 'allow_more_present', + 'type': 'list', + 'result': True + }, + { + 'a': [ + 'x', + 'x', + 'y', + ], + 'b': [ + 'x', + 'y', + ], + 'method': 'allow_more_present', + 'type': 'list', + 'result': False + }, + { + 'a': [ + 'x', + 'z', + ], + 'b': [ + 'x', + 'y', + 'x', + 'z', + ], + 'method': 'allow_more_present', + 'type': 'list', + 'result': True + }, + { + 'a': [ + 'x', + 'y', + ], + 'b': [ + 'y', + 'x', + ], + 'method': 'ignore', + 'type': 'list', + 'result': True + }, + ######################################################################################## + # set + { + 'a': [ + 'x', + ], + 'b': [ + 'y', + ], + 'method': 'strict', + 'type': 'set', + 'result': False + }, + { + 'a': [ + 'x', + ], + 'b': [ + 'x', + 'x', + ], + 'method': 'strict', + 'type': 'set', + 'result': True + }, + { + 'a': [ + 'x', + 'y', + ], + 'b': [ + 'x', + 'y', + ], + 'method': 'strict', + 'type': 'set', + 'result': True + }, + { + 'a': [ + 'x', + 'y', + ], + 'b': [ + 'y', + 'x', + ], + 'method': 'strict', + 'type': 'set', + 'result': True + }, + { + 'a': [ + 'x', + 'y', + ], + 'b': [ + 'x', + ], + 'method': 'allow_more_present', + 'type': 'set', + 'result': False + }, + { + 'a': [ + 'x', + ], + 'b': [ + 'x', + 'y', + ], + 'method': 'allow_more_present', + 'type': 'set', + 'result': True + }, + { + 'a': [ + 'x', + 'x', + 'y', + ], + 'b': [ + 'x', + 'y', + ], + 'method': 'allow_more_present', + 'type': 'set', + 'result': True + }, + { + 'a': [ + 'x', + 'z', + ], + 'b': [ + 'x', + 'y', + 'x', + 'z', + ], + 'method': 'allow_more_present', + 'type': 'set', + 'result': True + }, + { + 'a': [ + 'x', + 'a', + ], + 'b': [ + 'y', + 'z', + ], + 'method': 'ignore', + 'type': 'set', + 'result': True + }, + ######################################################################################## + # set(dict) + { + 'a': [ + {'x': 1}, + ], + 'b': [ + {'y': 1}, + ], + 'method': 'strict', + 'type': 'set(dict)', + 'result': False + }, + { + 'a': [ + {'x': 1}, + ], + 'b': [ + {'x': 1}, + ], + 'method': 'strict', + 'type': 'set(dict)', + 'result': True + }, + { + 'a': [ + {'x': 1}, + ], + 'b': [ + {'x': 1, 'y': 2}, + ], + 'method': 'strict', + 'type': 'set(dict)', + 'result': True + }, + { + 'a': [ + {'x': 1}, + {'x': 2, 'y': 3}, + ], + 'b': [ + {'x': 1}, + {'x': 2, 'y': 3}, + ], + 'method': 'strict', + 'type': 'set(dict)', + 'result': True + }, + { + 'a': [ + {'x': 1}, + ], + 'b': [ + {'x': 1, 'z': 2}, + {'x': 2, 'y': 3}, + ], + 'method': 'allow_more_present', + 'type': 'set(dict)', + 'result': True + }, + { + 'a': [ + {'x': 1, 'y': 2}, + ], + 'b': [ + {'x': 1}, + {'x': 2, 'y': 3}, + ], + 'method': 'allow_more_present', + 'type': 'set(dict)', + 'result': False + }, + { + 'a': [ + {'x': 1, 'y': 3}, + ], + 'b': [ + {'x': 1}, + {'x': 1, 'y': 3, 'z': 4}, + ], + 'method': 'allow_more_present', + 'type': 'set(dict)', + 'result': True + }, + { + 'a': [ + {'x': 1}, + {'x': 2, 'y': 3}, + ], + 'b': [ + {'x': 1}, + ], + 'method': 'ignore', + 'type': 'set(dict)', + 'result': True + }, + ######################################################################################## + # dict + { + 'a': {'x': 1}, + 'b': {'y': 1}, + 'method': 'strict', + 'type': 'dict', + 'result': False + }, + { + 'a': {'x': 1}, + 'b': {'x': 1, 'y': 2}, + 'method': 'strict', + 'type': 'dict', + 'result': False + }, + { + 'a': {'x': 1}, + 'b': {'x': 1}, + 'method': 'strict', + 'type': 'dict', + 'result': True + }, + { + 'a': {'x': 1, 'z': 2}, + 'b': {'x': 1, 'y': 2}, + 'method': 'strict', + 'type': 'dict', + 'result': False + }, + { + 'a': {'x': 1, 'z': 2}, + 'b': {'x': 1, 'y': 2}, + 'method': 'ignore', + 'type': 'dict', + 'result': True + }, +] + [{ + 'a': entry['av'], + 'b': entry['bv'], + 'method': 'allow_more_present', + 'type': 'dict', + 'result': entry['result'] +} for entry in DICT_ALLOW_MORE_PRESENT] + + +@pytest.mark.parametrize("entry", DICT_ALLOW_MORE_PRESENT) +def test_dict_allow_more_present(entry): + assert compare_dict_allow_more_present(entry['av'], entry['bv']) == entry['result'] + + +@pytest.mark.parametrize("entry", COMPARE_GENERIC) +def test_compare_generic(entry): + assert compare_generic(entry['a'], entry['b'], entry['method'], entry['type']) == entry['result']