From 8fbdb6fab5625137a8c989b1d1284f2d59bc5213 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20Sj=C3=B6gren?= Date: Wed, 25 Aug 2021 21:47:22 +0200 Subject: [PATCH 1/6] add tests and assume we handle the command output when we use register or args MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Thomas Sjögren --- .../rules/CommandHasChangesCheckRule.py | 105 ++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/src/ansiblelint/rules/CommandHasChangesCheckRule.py b/src/ansiblelint/rules/CommandHasChangesCheckRule.py index 27761687f0..dbc948c946 100644 --- a/src/ansiblelint/rules/CommandHasChangesCheckRule.py +++ b/src/ansiblelint/rules/CommandHasChangesCheckRule.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 TYPE_CHECKING, Any, Dict, Union from ansiblelint.rules import AnsibleLintRule @@ -53,5 +54,109 @@ def matchtask( and 'when' not in task and 'creates' not in task['action'] and 'removes' not in task['action'] + and not task.get("register") ) return False + + +if "pytest" in sys.modules: + import pytest + + NO_CHANGE_COMMAND_RC = ''' +- hosts: all + tasks: + - name: handle command output with _when + ansible.builtin.command: cat {{ myfile|quote }} + register: myoutput + changed_when: myoutput.rc != 0 + failed_when: myoutput.rc != 0 +''' + + NO_CHANGE_SHELL_RC = ''' +- hosts: all + tasks: + - name: handle shell output with _when + ansible.builtin.shell: cat {{ myfile|quote }} + register: myoutput + changed_when: myoutput.rc != 0 + failed_when: myoutput.rc != 0 +''' + + NO_CHANGE_REGISTER = ''' +- hosts: all + tasks: + - name: register command output, handle elsewhere + ansible.builtin.command: cat {{ myfile|quote }} + register: myoutput +''' + + NO_CHANGE_ARGS = ''' +- hosts: all + tasks: + - name: command with argument + command: createfile.sh + args: + creates: /tmp/????unknown_files???? +''' + + NO_CHANGE_COMMAND_FAIL = ''' +- hosts: all + tasks: + - name: basic command task, should fail + ansible.builtin.command: cat myfile +''' + + NO_CHANGE_SHELL_FAIL = ''' +- hosts: all + tasks: + - name: basic shell task, should fail + shell: cat myfile +''' + + @pytest.mark.parametrize( + 'rule_runner', (CommandHasChangesCheckRule,), indirect=['rule_runner'] + ) + def test_no_change_command_rc(rule_runner: Any) -> None: + """This should pass since *_when is used.""" + results = rule_runner.run_playbook(NO_CHANGE_COMMAND_RC) + assert len(results) == 0 + + @pytest.mark.parametrize( + 'rule_runner', (CommandHasChangesCheckRule,), indirect=['rule_runner'] + ) + def test_no_change_shell_rc(rule_runner: Any) -> None: + """This should pass since *_when is used.""" + results = rule_runner.run_playbook(NO_CHANGE_SHELL_RC) + assert len(results) == 0 + + @pytest.mark.parametrize( + 'rule_runner', (CommandHasChangesCheckRule,), indirect=['rule_runner'] + ) + def test_no_change_register(rule_runner: Any) -> None: + """This test should pass since we register the command output.""" + results = rule_runner.run_playbook(NO_CHANGE_REGISTER) + assert len(results) == 0 + + @pytest.mark.parametrize( + 'rule_runner', (CommandHasChangesCheckRule,), indirect=['rule_runner'] + ) + def test_no_change_args(rule_runner: Any) -> None: + """This test should pass since we manage output with create et al.""" + results = rule_runner.run_playbook(NO_CHANGE_ARGS) + assert len(results) == 0 + + @pytest.mark.parametrize( + 'rule_runner', (CommandHasChangesCheckRule,), indirect=['rule_runner'] + ) + def test_no_change_command_fail(rule_runner: Any) -> None: + """This test should fail because command isn't handled.""" + results = rule_runner.run_playbook(NO_CHANGE_COMMAND_FAIL) + assert len(results) == 1 + + @pytest.mark.parametrize( + 'rule_runner', (CommandHasChangesCheckRule,), indirect=['rule_runner'] + ) + def test_no_change_shell_fail(rule_runner: Any) -> None: + """This test should fail because shell isn't handled..""" + results = rule_runner.run_playbook(NO_CHANGE_SHELL_FAIL) + assert len(results) == 1 From 03923499f4c7f9f1c0b6ca55af897fdb96ef2c82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20Sj=C3=B6gren?= Date: Thu, 26 Aug 2021 10:52:19 +0200 Subject: [PATCH 2/6] commands should still be handled even if register is used MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Thomas Sjögren --- .../rules/CommandHasChangesCheckRule.py | 40 +++++++++++++------ 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/src/ansiblelint/rules/CommandHasChangesCheckRule.py b/src/ansiblelint/rules/CommandHasChangesCheckRule.py index dbc948c946..eca49a96f5 100644 --- a/src/ansiblelint/rules/CommandHasChangesCheckRule.py +++ b/src/ansiblelint/rules/CommandHasChangesCheckRule.py @@ -54,7 +54,6 @@ def matchtask( and 'when' not in task and 'creates' not in task['action'] and 'removes' not in task['action'] - and not task.get("register") ) return False @@ -69,7 +68,6 @@ def matchtask( ansible.builtin.command: cat {{ myfile|quote }} register: myoutput changed_when: myoutput.rc != 0 - failed_when: myoutput.rc != 0 ''' NO_CHANGE_SHELL_RC = ''' @@ -79,18 +77,26 @@ def matchtask( ansible.builtin.shell: cat {{ myfile|quote }} register: myoutput changed_when: myoutput.rc != 0 - failed_when: myoutput.rc != 0 ''' - NO_CHANGE_REGISTER = ''' + NO_CHANGE_SHELL_FALSE = ''' - hosts: all tasks: - - name: register command output, handle elsewhere + - name: handle shell output with changed_when + ansible.builtin.shell: cat {{ myfile|quote }} + register: myoutput + changed_when: false +''' + + NO_CHANGE_REGISTER_FAIL = ''' +- hosts: all + tasks: + - name: register command output, but cat still does not change anything ansible.builtin.command: cat {{ myfile|quote }} register: myoutput ''' - NO_CHANGE_ARGS = ''' + NO_CHANGE_ARGS_FAIL = ''' - hosts: all tasks: - name: command with argument @@ -132,18 +138,26 @@ def test_no_change_shell_rc(rule_runner: Any) -> None: @pytest.mark.parametrize( 'rule_runner', (CommandHasChangesCheckRule,), indirect=['rule_runner'] ) - def test_no_change_register(rule_runner: Any) -> None: - """This test should pass since we register the command output.""" - results = rule_runner.run_playbook(NO_CHANGE_REGISTER) + def test_no_change_shell_false(rule_runner: Any) -> None: + """This should pass since *_when is used.""" + results = rule_runner.run_playbook(NO_CHANGE_SHELL_FALSE) assert len(results) == 0 @pytest.mark.parametrize( 'rule_runner', (CommandHasChangesCheckRule,), indirect=['rule_runner'] ) - def test_no_change_args(rule_runner: Any) -> None: - """This test should pass since we manage output with create et al.""" - results = rule_runner.run_playbook(NO_CHANGE_ARGS) - assert len(results) == 0 + def test_no_change_register_fail(rule_runner: Any) -> None: + """This test should not pass since cat still doesn't do anything.""" + results = rule_runner.run_playbook(NO_CHANGE_REGISTER_FAIL) + assert len(results) == 1 + + @pytest.mark.parametrize( + 'rule_runner', (CommandHasChangesCheckRule,), indirect=['rule_runner'] + ) + def test_no_change_args_fail(rule_runner: Any) -> None: + """This test should not pass since the command doesn't do anything.""" + results = rule_runner.run_playbook(NO_CHANGE_ARGS_FAIL) + assert len(results) == 1 @pytest.mark.parametrize( 'rule_runner', (CommandHasChangesCheckRule,), indirect=['rule_runner'] From 38ddf6e66ad9373804ce51568b6af932ba8a3cd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20Sj=C3=B6gren?= Date: Thu, 26 Aug 2021 22:38:02 +0200 Subject: [PATCH 3/6] update test task names and args: should pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Thomas Sjögren --- .../rules/CommandHasChangesCheckRule.py | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/ansiblelint/rules/CommandHasChangesCheckRule.py b/src/ansiblelint/rules/CommandHasChangesCheckRule.py index eca49a96f5..94ee49dc65 100644 --- a/src/ansiblelint/rules/CommandHasChangesCheckRule.py +++ b/src/ansiblelint/rules/CommandHasChangesCheckRule.py @@ -64,7 +64,7 @@ def matchtask( NO_CHANGE_COMMAND_RC = ''' - hosts: all tasks: - - name: handle command output with _when + - name: handle command output with return code ansible.builtin.command: cat {{ myfile|quote }} register: myoutput changed_when: myoutput.rc != 0 @@ -73,7 +73,7 @@ def matchtask( NO_CHANGE_SHELL_RC = ''' - hosts: all tasks: - - name: handle shell output with _when + - name: handle shell output with return code ansible.builtin.shell: cat {{ myfile|quote }} register: myoutput changed_when: myoutput.rc != 0 @@ -82,21 +82,13 @@ def matchtask( NO_CHANGE_SHELL_FALSE = ''' - hosts: all tasks: - - name: handle shell output with changed_when + - name: handle shell output with false changed_when ansible.builtin.shell: cat {{ myfile|quote }} register: myoutput changed_when: false ''' - NO_CHANGE_REGISTER_FAIL = ''' -- hosts: all - tasks: - - name: register command output, but cat still does not change anything - ansible.builtin.command: cat {{ myfile|quote }} - register: myoutput -''' - - NO_CHANGE_ARGS_FAIL = ''' + NO_CHANGE_ARGS = ''' - hosts: all tasks: - name: command with argument @@ -105,6 +97,14 @@ def matchtask( creates: /tmp/????unknown_files???? ''' + NO_CHANGE_REGISTER_FAIL = ''' +- hosts: all + tasks: + - name: register command output, but cat still does not change anything + ansible.builtin.command: cat {{ myfile|quote }} + register: myoutput +''' + NO_CHANGE_COMMAND_FAIL = ''' - hosts: all tasks: @@ -146,17 +146,17 @@ def test_no_change_shell_false(rule_runner: Any) -> None: @pytest.mark.parametrize( 'rule_runner', (CommandHasChangesCheckRule,), indirect=['rule_runner'] ) - def test_no_change_register_fail(rule_runner: Any) -> None: - """This test should not pass since cat still doesn't do anything.""" - results = rule_runner.run_playbook(NO_CHANGE_REGISTER_FAIL) - assert len(results) == 1 + def test_no_change_args(rule_runner: Any) -> None: + """This test should not pass since the command doesn't do anything.""" + results = rule_runner.run_playbook(NO_CHANGE_ARGS) + assert len(results) == 0 @pytest.mark.parametrize( 'rule_runner', (CommandHasChangesCheckRule,), indirect=['rule_runner'] ) - def test_no_change_args_fail(rule_runner: Any) -> None: - """This test should not pass since the command doesn't do anything.""" - results = rule_runner.run_playbook(NO_CHANGE_ARGS_FAIL) + def test_no_change_register_fail(rule_runner: Any) -> None: + """This test should not pass since cat still doesn't do anything.""" + results = rule_runner.run_playbook(NO_CHANGE_REGISTER_FAIL) assert len(results) == 1 @pytest.mark.parametrize( From 79d5d63bd6f741e8f5cd6617be8439767d47d5d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20Sj=C3=B6gren?= Date: Fri, 27 Aug 2021 00:29:47 +0200 Subject: [PATCH 4/6] expand the rule description MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Thomas Sjögren --- .../rules/CommandHasChangesCheckRule.py | 30 +++++++++++++++---- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/src/ansiblelint/rules/CommandHasChangesCheckRule.py b/src/ansiblelint/rules/CommandHasChangesCheckRule.py index 94ee49dc65..4e4a99c295 100644 --- a/src/ansiblelint/rules/CommandHasChangesCheckRule.py +++ b/src/ansiblelint/rules/CommandHasChangesCheckRule.py @@ -32,12 +32,30 @@ class CommandHasChangesCheckRule(AnsibleLintRule): id = 'no-changed-when' shortdesc = 'Commands should not change things if nothing needs doing' - description = ( - 'Commands should either read information (and thus set ' - '``changed_when``) or not do something if it has already been ' - 'done (using creates/removes) or only do it if another ' - 'check has a particular result (``when``)' - ) + description = """ +Commands should either read information (and thus set +``changed_when``) or not do something if it has already been +done (using ``creates`` or ``removes`` argument) or only do it if another +check has a particular result (``when``). + +An example where the ``shell`` output is registered and the return code +is used to define when the task has changed. + +.. code:: yaml + + - name: handle shell output with return code + ansible.builtin.shell: cat {{ myfile|quote }} + register: myoutput + changed_when: myoutput.rc != 0 + +The following example will trigger the rule since the task does not +handle the output of the ``command``. + +.. code:: yaml + + - name: does not handle any output or return codes + ansible.builtin.command: cat {{ myfile|quote }} + """ severity = 'HIGH' tags = ['command-shell', 'idempotency'] version_added = 'historic' From afbbe460f6a07e5c0d7929b56b70236f0eaf5830 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20Sj=C3=B6gren?= Date: Fri, 27 Aug 2021 16:39:24 +0000 Subject: [PATCH 5/6] Update src/ansiblelint/rules/CommandHasChangesCheckRule.py Co-authored-by: Alicia Cozine <879121+acozine@users.noreply.github.com> --- src/ansiblelint/rules/CommandHasChangesCheckRule.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ansiblelint/rules/CommandHasChangesCheckRule.py b/src/ansiblelint/rules/CommandHasChangesCheckRule.py index 4e4a99c295..5c9727eb36 100644 --- a/src/ansiblelint/rules/CommandHasChangesCheckRule.py +++ b/src/ansiblelint/rules/CommandHasChangesCheckRule.py @@ -38,8 +38,8 @@ class CommandHasChangesCheckRule(AnsibleLintRule): done (using ``creates`` or ``removes`` argument) or only do it if another check has a particular result (``when``). -An example where the ``shell`` output is registered and the return code -is used to define when the task has changed. +For example, this task registers the ``shell`` output and uses the return code +to define when the task has changed. .. code:: yaml From 7c5b368adb636c5b5f9e725088e7a2b9553e35bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20Sj=C3=B6gren?= Date: Fri, 3 Sep 2021 11:37:23 +0200 Subject: [PATCH 6/6] update description wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Thomas Sjögren --- src/ansiblelint/rules/CommandHasChangesCheckRule.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ansiblelint/rules/CommandHasChangesCheckRule.py b/src/ansiblelint/rules/CommandHasChangesCheckRule.py index 5c9727eb36..9744541444 100644 --- a/src/ansiblelint/rules/CommandHasChangesCheckRule.py +++ b/src/ansiblelint/rules/CommandHasChangesCheckRule.py @@ -33,10 +33,10 @@ class CommandHasChangesCheckRule(AnsibleLintRule): id = 'no-changed-when' shortdesc = 'Commands should not change things if nothing needs doing' description = """ -Commands should either read information (and thus set -``changed_when``) or not do something if it has already been -done (using ``creates`` or ``removes`` argument) or only do it if another -check has a particular result (``when``). +Tasks should tell Ansible when to return ``changed``, unless the task only reads +information. To do this, set ``changed_when``, use the ``creates`` or +``removes`` argument, or use ``when`` to run the task only if another check has +a particular result. For example, this task registers the ``shell`` output and uses the return code to define when the task has changed.