From 0be3f381318dba69727895fe72fd675de19c97e4 Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Mon, 22 Feb 2021 16:21:23 +0000 Subject: [PATCH] Refactored no-handler rule (#1402) - moved tests alongside rule definition - extend tests to include 2 more cases - included the newer "is changed" in the list of matching strings - made the description more verbose - included link to handlers documentation --- examples/playbooks/role-with-handler.yml | 4 --- examples/playbooks/rule-no-handler.yml | 20 +++++++++++ examples/roles/a-role/handlers/main.yml | 5 --- examples/roles/a-role/tasks/main.yml | 6 ++++ src/ansiblelint/generate_docs.py | 2 +- .../UseHandlerRatherThanWhenChangedRule.py | 36 +++++++++++++++++-- test/TestRoleHandlers.py | 20 ----------- 7 files changed, 61 insertions(+), 32 deletions(-) delete mode 100644 examples/playbooks/role-with-handler.yml create mode 100644 examples/playbooks/rule-no-handler.yml delete mode 100644 examples/roles/a-role/handlers/main.yml create mode 100644 examples/roles/a-role/tasks/main.yml delete mode 100644 test/TestRoleHandlers.py diff --git a/examples/playbooks/role-with-handler.yml b/examples/playbooks/role-with-handler.yml deleted file mode 100644 index 0a8a960b82..0000000000 --- a/examples/playbooks/role-with-handler.yml +++ /dev/null @@ -1,4 +0,0 @@ -- hosts: localhost - name: foo - roles: - - {role: a-role} diff --git a/examples/playbooks/rule-no-handler.yml b/examples/playbooks/rule-no-handler.yml new file mode 100644 index 0000000000..8866fae874 --- /dev/null +++ b/examples/playbooks/rule-no-handler.yml @@ -0,0 +1,20 @@ +# should trigger no-handler rule 3 times +- hosts: localhost + name: foo + roles: + - {role: a-role} # 1st from within included role + + tasks: + + - name: execute something + command: echo 123 + register: result + changed_when: true + + - name: this should trigger no-handler rule # 2nd + command: echo could be done better + when: result is changed + + - name: this should trigger no-handler rule # 3rd + command: echo could be done better too + when: result is changed diff --git a/examples/roles/a-role/handlers/main.yml b/examples/roles/a-role/handlers/main.yml deleted file mode 100644 index 59ae800935..0000000000 --- a/examples/roles/a-role/handlers/main.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -- name: do anything - shell: echo merp | cat - when: - - something.changed diff --git a/examples/roles/a-role/tasks/main.yml b/examples/roles/a-role/tasks/main.yml new file mode 100644 index 0000000000..8593ada723 --- /dev/null +++ b/examples/roles/a-role/tasks/main.yml @@ -0,0 +1,6 @@ +--- +# should trigger no-handler at line 3 +- name: do anything + command: echo 123 + when: + - something.changed diff --git a/src/ansiblelint/generate_docs.py b/src/ansiblelint/generate_docs.py index a9af1d3582..c8d98c9d04 100644 --- a/src/ansiblelint/generate_docs.py +++ b/src/ansiblelint/generate_docs.py @@ -35,7 +35,7 @@ def rules_as_rst(rules: RulesCollection) -> str: description = d.description if d.link: - description += " `(more) <%s>`_" % d.link + description += " `(more) <%s>`__" % d.link r += f"\n\n.. _{d.id}:\n\n{title}\n{'*' * len(title)}\n\n{d.shortdesc}\n\n{description}" diff --git a/src/ansiblelint/rules/UseHandlerRatherThanWhenChangedRule.py b/src/ansiblelint/rules/UseHandlerRatherThanWhenChangedRule.py index aa2884373e..ae564aafd1 100644 --- a/src/ansiblelint/rules/UseHandlerRatherThanWhenChangedRule.py +++ b/src/ansiblelint/rules/UseHandlerRatherThanWhenChangedRule.py @@ -18,6 +18,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN # THE SOFTWARE. +import sys from typing import Any, Dict, Union from ansiblelint.rules import AnsibleLintRule @@ -28,7 +29,13 @@ def _changed_in_when(item: str) -> bool: return False return any( changed in item - for changed in ['.changed', '|changed', '["changed"]', "['changed']"] + for changed in [ + '.changed', + '|changed', + '["changed"]', + "['changed']", + "is changed", + ] ) @@ -37,8 +44,10 @@ class UseHandlerRatherThanWhenChangedRule(AnsibleLintRule): shortdesc = 'Tasks that run when changed should likely be handlers' description = ( 'If a task has a ``when: result.changed`` setting, it is effectively ' - 'acting as a handler' + 'acting as a handler. You could use notify and move that task to ' + 'handlers.' ) + link = "https://docs.ansible.com/ansible/latest/user_guide/playbooks_handlers.html" severity = 'MEDIUM' tags = ['idiom'] version_added = 'historic' @@ -55,3 +64,26 @@ def matchtask(self, task: Dict[str, Any]) -> Union[bool, str]: if isinstance(when, str): return _changed_in_when(when) return False + + +if 'pytest' in sys.modules: + + from ansiblelint.file_utils import Lintable + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner + + def test_rule_no_handler() -> None: + """Verify rule.""" + collection = RulesCollection() + collection.register(UseHandlerRatherThanWhenChangedRule()) + + lintable = Lintable('examples/playbooks/rule-no-handler.yml') + results = Runner(lintable, rules=collection).run() + + assert len(results) == 3 + assert results[0].filename == 'examples/playbooks/roles/a-role/tasks/main.yml' + assert results[0].linenumber == 3 + assert results[1].filename == 'examples/playbooks/rule-no-handler.yml' + assert results[1].linenumber == 14 + assert results[2].filename == 'examples/playbooks/rule-no-handler.yml' + assert results[2].linenumber == 18 diff --git a/test/TestRoleHandlers.py b/test/TestRoleHandlers.py deleted file mode 100644 index 83ad5bbcb7..0000000000 --- a/test/TestRoleHandlers.py +++ /dev/null @@ -1,20 +0,0 @@ -# pylint: disable=preferred-module # FIXME: remove once migrated per GH-725 -import unittest - -from ansiblelint.rules import RulesCollection -from ansiblelint.rules.UseHandlerRatherThanWhenChangedRule import ( - UseHandlerRatherThanWhenChangedRule, -) -from ansiblelint.runner import Runner - - -class TestRoleHandlers(unittest.TestCase): - collection = RulesCollection() - - def setUp(self): - self.collection.register(UseHandlerRatherThanWhenChangedRule()) - - def test_role_handler_positive(self): - success = 'examples/playbooks/role-with-handler.yml' - good_runner = Runner(success, rules=self.collection) - self.assertEqual([], good_runner.run())