From 61a826c9979b3063b79f67bc767f87131dd8a36d Mon Sep 17 00:00:00 2001 From: Ashwini Mhatre Date: Tue, 3 Aug 2021 13:34:33 +0530 Subject: [PATCH] Update validate to use 2.11 ArgumentSpecValidator if available (#86) Update validate to use 2.11 ArgumentSpecValidator if available SUMMARY fixes: #85 ISSUE TYPE Bugfix Pull Request COMPONENT NAME ADDITIONAL INFORMATION Reviewed-by: Bradley A. Thornton Reviewed-by: Ashwini Mhatre Reviewed-by: None --- .../fragments/85_update_validate_plugin.yaml | 3 ++ .../module_utils/common/argspec_validate.py | 34 ++++++++++++++++--- .../targets/cli_parse/tasks/argspec.yaml | 16 ++++++--- .../fact_diff/tasks/include/argspec.yaml | 12 +++++++ .../get_path/tasks/include/argspec.yaml | 2 +- .../module_utils/test_argspec_validate.py | 24 ++++++++++--- tests/unit/plugins/action/test_cli_parse.py | 4 +-- tests/unit/plugins/action/test_validate.py | 8 +++-- tests/unit/plugins/test/test_validate.py | 4 +-- 9 files changed, 85 insertions(+), 22 deletions(-) create mode 100644 changelogs/fragments/85_update_validate_plugin.yaml diff --git a/changelogs/fragments/85_update_validate_plugin.yaml b/changelogs/fragments/85_update_validate_plugin.yaml new file mode 100644 index 0000000..94bfb3c --- /dev/null +++ b/changelogs/fragments/85_update_validate_plugin.yaml @@ -0,0 +1,3 @@ +--- +bugfixes: + - Update validate to use 2.11 ArgumentSpecValidator if available. diff --git a/plugins/module_utils/common/argspec_validate.py b/plugins/module_utils/common/argspec_validate.py index b71e3fd..2c2afc3 100644 --- a/plugins/module_utils/common/argspec_validate.py +++ b/plugins/module_utils/common/argspec_validate.py @@ -46,9 +46,7 @@ except ImportError: # ansible-base 2.11 should expose argspec validation outside of the # ansiblemodule class try: - from ansible.module_utils.somefile import ( # noqa: F401 - FutureBaseArgspecValidator, - ) + from ansible.module_utils.common.arg_spec import ArgumentSpecValidator HAS_ANSIBLE_ARG_SPEC_VALIDATOR = True except ImportError: @@ -238,7 +236,35 @@ class AnsibleArgSpecValidator: that is coming in 2.11, change the check according above """ if HAS_ANSIBLE_ARG_SPEC_VALIDATOR: - return self._validate() + if self._schema_format == "doc": + self._convert_doc_to_schema() + if self._schema_conditionals is not None: + self._schema = dict_merge( + self._schema, self._schema_conditionals + ) + invalid_keys = [ + k + for k in self._schema.keys() + if k not in VALID_ANSIBLEMODULE_ARGS + ] + if invalid_keys: + valid = False + errors = [ + "Invalid schema. Invalid keys found: {ikeys}".format( + ikeys=",".join(invalid_keys) + ) + ] + updated_data = {} + return valid, errors, updated_data + else: + validator = ArgumentSpecValidator(**self._schema) + result = validator.validate(self._data) + valid = not bool(result.error_messages) + return ( + valid, + result.error_messages, + result.validated_parameters, + ) else: return self._validate() diff --git a/tests/integration/targets/cli_parse/tasks/argspec.yaml b/tests/integration/targets/cli_parse/tasks/argspec.yaml index 400a3b2..c53dad6 100644 --- a/tests/integration/targets/cli_parse/tasks/argspec.yaml +++ b/tests/integration/targets/cli_parse/tasks/argspec.yaml @@ -11,7 +11,9 @@ - name: "{{ parser }} Check argspec fail" assert: - that: "argfail['errors'] == 'parameters are mutually exclusive: command|template_path found in parser'" + that: "{{ msg in argfail.errors }}" + vars: + msg: "parameters are mutually exclusive: command|template_path found in parser" - name: "{{ parser }} validate argspec" ansible.utils.cli_parse: @@ -25,7 +27,9 @@ - name: "{{ parser }} Check argspec fail" assert: - that: "argfail['errors'] == 'parameters are mutually exclusive: command|text'" + that: "{{ msg in argfail.errors }}" + vars: + msg: "parameters are mutually exclusive: command|text" - name: "{{ parser }} validate argspec" ansible.utils.cli_parse: @@ -37,7 +41,9 @@ - name: "{{ parser }} Check argspec fail" assert: - that: "argfail['errors'] == 'one of the following is required: command, text'" + that: "{{ msg in argfail.errors }}" + vars: + msg: "one of the following is required: command, text" - name: "{{ parser }} validate argspec" @@ -51,4 +57,6 @@ - name: "{{ parser }} Check arspec fail" assert: - that: "argfail['msg'] == 'Parser name should be provided as a full name including collection'" + that: "{{ msg in argfail.msg }}" + vars: + msg: "Parser name should be provided as a full name including collection" diff --git a/tests/integration/targets/fact_diff/tasks/include/argspec.yaml b/tests/integration/targets/fact_diff/tasks/include/argspec.yaml index 6b72b21..5606c5b 100644 --- a/tests/integration/targets/fact_diff/tasks/include/argspec.yaml +++ b/tests/integration/targets/fact_diff/tasks/include/argspec.yaml @@ -12,6 +12,13 @@ - after loop_control: loop_var: string + when: "{{result.msg|type_debug != 'list'}}" + +- assert: + that: "{{ msg in result.msg }}" + vars: + msg: "missing required arguments: after, before" + when: "{{result.msg|type_debug == 'list'}}" - name: Check argspec validation, skip_lines must be a dict ansible.utils.fact_diff: @@ -26,3 +33,8 @@ - assert: that: "{{ 'unable to convert to list' in result.msg }}" + when: "{{result.msg|type_debug != 'list'}}" + +- assert: + that: "{{ 'unable to convert to list' in result.msg[0] }}" + when: "{{result.msg|type_debug == 'list'}}" diff --git a/tests/integration/targets/get_path/tasks/include/argspec.yaml b/tests/integration/targets/get_path/tasks/include/argspec.yaml index 56728e0..d0c0138 100644 --- a/tests/integration/targets/get_path/tasks/include/argspec.yaml +++ b/tests/integration/targets/get_path/tasks/include/argspec.yaml @@ -14,7 +14,7 @@ register: result - assert: - that: "{{ result.msg == msg }}" + that: "{{ msg in result.msg }}" vars: msg: "missing required arguments: path" diff --git a/tests/unit/module_utils/test_argspec_validate.py b/tests/unit/module_utils/test_argspec_validate.py index 57ce54c..fc8a343 100644 --- a/tests/unit/module_utils/test_argspec_validate.py +++ b/tests/unit/module_utils/test_argspec_validate.py @@ -27,6 +27,8 @@ class TestSortList(unittest.TestCase): ) valid, errors, _updated_data = aav.validate() self.assertTrue(valid) + if not errors: + errors = None self.assertEqual(errors, None) def test_simple_defaults(self): @@ -46,6 +48,8 @@ class TestSortList(unittest.TestCase): } valid, errors, updated_data = aav.validate() self.assertTrue(valid) + if not errors: + errors = None self.assertEqual(errors, None) self.assertEqual(expected, updated_data) @@ -84,6 +88,8 @@ class TestSortList(unittest.TestCase): ) valid, errors, _updated_data = aav.validate() self.assertTrue(valid) + if not errors: + errors = None self.assertEqual(errors, None) def test_schema_conditional(self): @@ -114,10 +120,16 @@ class TestSortList(unittest.TestCase): ) valid, errors, _updated_data = aav.validate() self.assertFalse(valid) - self.assertIn( - "Unsupported parameters for 'test_action' module: not_valid", - errors, - ) + if isinstance(errors, list): + # for ansibleargspecvalidator 2.11 its returning errors as list + self.assertIn( + "not_valid. Supported parameters include:", errors[0] + ) + else: + self.assertIn( + "Unsupported parameters for 'test_action' module: not_valid", + errors, + ) def test_other_args(self): data = {"param_str": "string"} @@ -130,6 +142,8 @@ class TestSortList(unittest.TestCase): ) valid, errors, _updated_data = aav.validate() self.assertTrue(valid) + if not errors: + errors = None self.assertIsNone(errors) def test_invalid_spec(self): @@ -143,4 +157,4 @@ class TestSortList(unittest.TestCase): ) valid, errors, _updated_data = aav.validate() self.assertFalse(valid) - self.assertIn("Invalid keys found: not_valid", errors) + self.assertIn("Invalid schema. Invalid keys found: not_valid", errors) diff --git a/tests/unit/plugins/action/test_cli_parse.py b/tests/unit/plugins/action/test_cli_parse.py index eb5771f..901033e 100644 --- a/tests/unit/plugins/action/test_cli_parse.py +++ b/tests/unit/plugins/action/test_cli_parse.py @@ -117,7 +117,7 @@ class TestCli_Parse(unittest.TestCase): **kwargs ) - self.assertEqual( + self.assertIn( "one of the following is required: command, text", result["errors"] ) @@ -131,7 +131,7 @@ class TestCli_Parse(unittest.TestCase): schema_conditionals=ARGSPEC_CONDITIONALS, **kwargs ) - self.assertEqual( + self.assertIn( "missing required arguments: name found in parser", result["errors"], ) diff --git a/tests/unit/plugins/action/test_validate.py b/tests/unit/plugins/action/test_validate.py index c7b42f6..6a95b40 100644 --- a/tests/unit/plugins/action/test_validate.py +++ b/tests/unit/plugins/action/test_validate.py @@ -144,9 +144,11 @@ class TestValidate(unittest.TestCase): # missing required arguments self._plugin._task.args = {"engine": "ansible.utils.jsonschema"} result = self._plugin.run(task_vars=None) - msg = "missing required arguments criteria data" - for word in msg.split(): - self.assertIn(word, result["errors"]) + msg = "missing required arguments:" + if isinstance(result["errors"], list): + self.assertIn(msg, result["errors"][0]) + else: + self.assertIn(msg, result["errors"]) # invalid engine option value self._plugin._task.args = { diff --git a/tests/unit/plugins/test/test_validate.py b/tests/unit/plugins/test/test_validate.py index 63fef9b..aecb43c 100644 --- a/tests/unit/plugins/test/test_validate.py +++ b/tests/unit/plugins/test/test_validate.py @@ -112,9 +112,7 @@ class TestValidate(unittest.TestCase): with self.assertRaises(AnsibleError) as error: validate(*args, **kwargs) msg = "missing required arguments: criteria" - self.assertTrue( - set(str(error.exception).split()).issuperset(set(msg.split())) - ) + self.assertIn(msg, str(error.exception)) kwargs = { "criteria": CRITERIA_IN_RATE_CHECK,