Make sure the check module is idempotent

Payload for creating a check is large and contains quite a few lists
that serve as a set of things. All this makes comparing the payloads a
bit tougher that for the rest of the resources.

Another source of problems is the backend's tendency to treat null and
empty list as equivalent, adding yet another place where equality
checks can go wrong.

Changes in this commit try to fix all of the idempotency issues we
could think of, but there is a slight possibility that we still missed
something. But things should be in a much better shape now.

plugins/module_utils/utils.py

@@ -92,6 +92,14 @@ def dict_to_single_item_dicts(data):
     return [{k: v} for k, v in data.items()]
 
 
+def single_item_dicts_to_dict(data):
+    result = {}
+    for item in data:
+        (k, v), = item.items()
+        result[k] = v
+    return result
+
+
 def dict_to_key_value_strings(data):
     return ["{0}={1}".format(k, v) for k, v in data.items()]
 

plugins/modules/check.py

@@ -222,6 +222,56 @@ def validate_module_params(module):
         module.fail_json(msg='one of the following is required: interval, cron')
 
 
+def do_sets_differ(current, desired, key):
+    return set(current.get(key) or []) != set(desired.get(key) or [])
+
+
+def do_proxy_requests_differ(current, desired):
+    if 'proxy_requests' not in desired:
+        return False
+
+    current = current.get('proxy_requests') or {}
+    desired = desired['proxy_requests']
+
+    return (
+        (
+            'entity_attributes' in desired and
+            do_sets_differ(current, desired, 'entity_attributes')
+        ) or
+        utils.do_differ(current, desired, 'entity_attributes')
+    )
+
+
+def do_check_hooks_differ(current, desired):
+    if 'check_hooks' not in desired:
+        return False
+
+    current = utils.single_item_dicts_to_dict(current.get('check_hooks') or [])
+    current = {k: set(v) for k, v in current.items()}
+
+    desired = utils.single_item_dicts_to_dict(desired['check_hooks'])
+    desired = {k: set(v) for k, v in desired.items()}
+
+    return current != desired
+
+
+def do_differ(current, desired):
+    return (
+        utils.do_differ(
+            current, desired, 'proxy_requests', 'subscriptions', 'handlers',
+            'runtime_assets', 'check_hooks', 'output_metric_handlers',
+            'env_vars',
+        ) or
+        do_proxy_requests_differ(current, desired) or
+        do_sets_differ(current, desired, 'subscriptions') or
+        do_sets_differ(current, desired, 'handlers') or
+        do_sets_differ(current, desired, 'runtime_assets') or
+        do_check_hooks_differ(current, desired) or
+        do_sets_differ(current, desired, 'output_metric_handlers') or
+        do_sets_differ(current, desired, 'env_vars')
+    )
+
+
 def build_api_payload(params):
     payload = arguments.get_mutation_payload(
         params,
@@ -234,7 +284,6 @@ def build_api_payload(params):
         'output_metric_format',
         'output_metric_handlers',
         'proxy_entity_name',
-        'proxy_requests',
         'publish',
         'round_robin',
         'runtime_assets',
@@ -243,6 +292,13 @@ def build_api_payload(params):
         'timeout',
         'ttl'
     )
+
+    if params['proxy_requests']:
+        payload['proxy_requests'] = arguments.get_spec_payload(
+            params['proxy_requests'],
+            'entity_attributes', 'splay', 'splay_coverage',
+        )
+
     if params['check_hooks']:
         payload['check_hooks'] = utils.dict_to_single_item_dicts(params['check_hooks'])
 
@@ -340,6 +396,7 @@ def main():
     try:
         changed, check = utils.sync(
             module.params['state'], client, path, payload, module.check_mode,
+            do_differ,
         )
         module.exit_json(changed=changed, object=check)
     except errors.Error as e:

tests/integration/molecule/module_check/converge.yml

@@ -197,3 +197,58 @@
     - assert:
         that:
           - result.objects == []
+
+    - name: Create check for idempotency test of complex fields
+      check:
+        name: complex
+        command: sleep 10
+        interval: 10
+        subscriptions:
+          - sub1
+          - sub2
+        handlers: []
+        proxy_requests:
+          entity_attributes:
+            - "entity.entity_class == 'proxy'"
+            - "entity.entity_class == 'demo'"
+        check_hooks:
+          warning:
+            - h1
+            - h2
+            - h3
+          error:
+            - h4
+            - h2
+        env_vars:
+          var1: val1
+          var2: val2
+
+    - name: Test for idempotency test of complex fields
+      check:
+        name: complex
+        command: sleep 10
+        interval: 10
+        subscriptions:
+          - sub2
+          - sub1
+        runtime_assets: []
+        proxy_requests:
+          entity_attributes:
+            - "entity.entity_class == 'demo'"
+            - "entity.entity_class == 'proxy'"
+        check_hooks:
+          warning:
+            - h1
+            - h3
+            - h2
+          error:
+            - h2
+            - h4
+        env_vars:
+          var2: val2
+          var1: val1
+      register: result
+
+    - assert:
+        that:
+          - result is not changed

tests/unit/module_utils/test_utils.py

@@ -265,6 +265,13 @@ def test_conversion(self):
             assert item in result
 
 
+class TestSingleItemDictsToDict:
+    def test_conversion(self):
+        assert dict(a=3, b=4, c=5) == utils.single_item_dicts_to_dict(
+            [dict(a=3), dict(b=4), dict(c=5)]
+        )
+
+
 class TestDictToKeyValueString:
     def test_conversion(self):
         result = utils.dict_to_key_value_strings({"a": 0, 1: "b"})

tests/unit/modules/test_check.py

@@ -13,6 +13,155 @@
 )
 
 
+class TestDoSetsDiffer:
+    @pytest.mark.parametrize("current,desired,diff", [
+        ([1, 2, 3], [1, 2, 3], False),
+        ([1, 2, 3], [3, 2, 1], False),
+        ([1, 2], [1, 2, 3], True),
+        ([1, 3], [2, 4], True),
+    ])
+    def test_comparison(self, current, desired, diff):
+        c = dict(k=current)
+        d = dict(k=desired)
+
+        assert check.do_sets_differ(c, d, "k") is diff
+
+    def test_missing_keys_are_treated_as_empty_sets(self):
+        current = dict(a=[])
+        desired = dict()
+
+        assert check.do_sets_differ(current, desired, "a") is False
+        assert check.do_sets_differ(desired, current, "a") is False
+
+    def test_nulls_are_treated_as_empty_sets(self):
+        current = dict(a=None)
+        desired = dict(a=[])
+
+        assert check.do_sets_differ(current, desired, "a") is False
+        assert check.do_sets_differ(desired, current, "a") is False
+
+
+class TestDoProxyRequestsDiffer:
+    def test_missing_proxy_requests_in_desired_is_ignored(self):
+        current = dict(proxy_requests=dict(entity_attributes=["a", "b"]))
+        desired = dict()
+
+        assert check.do_proxy_requests_differ(current, desired) is False
+
+    @pytest.mark.parametrize("current,desired,diff", [
+        (["a", "b"], ["a", "b"], False),
+        (["a", "b"], ["b", "a"], False),
+        (None, [], False),
+        (["a", "b"], ["c", "a"], True),
+        (["a", "b"], ["a", "b", "c"], True),
+    ])
+    def test_treat_entity_attributes_as_a_set(self, current, desired, diff):
+        c = dict(proxy_requests=dict(entity_attributes=current))
+        d = dict(proxy_requests=dict(entity_attributes=desired))
+
+        assert check.do_proxy_requests_differ(c, d) is diff
+
+    def test_ignore_missing_entity_attributes_in_desired(self):
+        current = dict(proxy_requests=dict(entity_attributes=["a", "b"]))
+        desired = dict(proxy_requests=dict())
+
+        assert check.do_proxy_requests_differ(current, desired) is False
+
+    @pytest.mark.parametrize("current,desired,diff", [
+        (dict(splay=False), dict(splay=False), False),
+        (dict(splay=False), dict(), False),
+        (dict(splay=False), dict(splay=True), True),
+        (dict(), dict(splay=True), True),
+    ])
+    def test_other_stuff_is_compared_as_usual(self, current, desired, diff):
+        c = dict(proxy_requests=current)
+        d = dict(proxy_requests=desired)
+
+        assert check.do_proxy_requests_differ(c, d) is diff
+
+
+class TestDoCheckHooksDiffer:
+    def test_missing_check_hooks_in_desired_is_ignored(self):
+        current = dict(check_hooks=[dict(warning=["a"])])
+        desired = dict()
+
+        assert check.do_check_hooks_differ(current, desired) is False
+
+    @pytest.mark.parametrize("current,desired,diff", [
+        (["a", "b"], ["a", "b"], False),
+        (["a", "b"], ["b", "a"], False),
+        (["a", "b"], ["c", "a"], True),
+        (["a", "b"], ["a", "b", "c"], True),
+    ])
+    def test_treat_hooks_as_a_set(self, current, desired, diff):
+        c = dict(check_hooks=[dict(warning=current)])
+        d = dict(check_hooks=[dict(warning=desired)])
+
+        assert check.do_check_hooks_differ(c, d) is diff
+
+
+class TestDoDiffer:
+    def test_no_difference(self):
+        assert not check.do_differ(
+            dict(
+                command="sleep",
+                subscriptions=["sub1", "sub2"],
+                handlers=["ha1", "ha2", "ha3"],
+                interval=123,
+                cron="* * * 3 2",
+                publish=False,
+                timeout=30,
+                ttl=60,
+                stdin=True,
+                low_flap_threshold=2,
+                high_flap_threshold=10,
+                runtime_assets=["asset1", "asset2"],
+                check_hooks=[
+                    dict(warning=["hook0-1", "hook0-2"]),
+                    dict(critical=["hook1-1", "hook1-2"]),
+                ],
+                proxy_entity_name="name",
+                proxy_requests=dict(
+                    entity_attributes=["a1", "a2", "a3"],
+                    splay=True,
+                    splay_coverage=10,
+                ),
+                output_metric_format="influxdb_line",
+                output_metric_handlers=["mhandler1", "mhandler2"],
+                round_robin=False,
+                env_vars=["k1=v1", "k2=v2"],
+            ),
+            dict(
+                command="sleep",
+                subscriptions=["sub2", "sub1"],
+                handlers=["ha3", "ha1", "ha2"],
+                interval=123,
+                cron="* * * 3 2",
+                publish=False,
+                timeout=30,
+                ttl=60,
+                stdin=True,
+                low_flap_threshold=2,
+                high_flap_threshold=10,
+                runtime_assets=["asset2", "asset1"],
+                check_hooks=[
+                    dict(critical=["hook1-2", "hook1-1"]),
+                    dict(warning=["hook0-2", "hook0-1"]),
+                ],
+                proxy_entity_name="name",
+                proxy_requests=dict(
+                    splay=True,
+                    entity_attributes=["a3", "a2", "a1"],
+                    splay_coverage=10,
+                ),
+                output_metric_format="influxdb_line",
+                output_metric_handlers=["mhandler2", "mhandler1"],
+                round_robin=False,
+                env_vars=["k2=v2", "k1=v1"],
+            )
+        )
+
+
 class TestSensuGoCheck(ModuleTestCase):
     def test_minimal_check_parameters(self, mocker):
         sync_mock = mocker.patch.object(utils, "sync")
@@ -27,7 +176,7 @@ def test_minimal_check_parameters(self, mocker):
         with pytest.raises(AnsibleExitJson):
             check.main()
 
-        state, _client, path, payload, check_mode = sync_mock.call_args[0]
+        state, _client, path, payload, check_mode, _d = sync_mock.call_args[0]
         assert state == "present"
         assert path == "/api/core/v2/namespaces/default/checks/test_check"
         assert payload == dict(
@@ -74,7 +223,7 @@ def test_all_check_parameters(self, mocker):
         with pytest.raises(AnsibleExitJson):
             check.main()
 
-        state, _client, path, payload, check_mode = sync_mock.call_args[0]
+        state, _client, path, payload, check_mode, _d = sync_mock.call_args[0]
         assert state == "absent"
         assert path == "/api/core/v2/namespaces/my/checks/test_check"
         assert payload == dict(