Repository: ansible/ansible
Pull request: adding quiet option to assert (ansible#27124)
##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Adds quiet boolean option to assert.
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
HINT: Fixes #27124

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Feature Pull Request

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
assert
##### ADDITIONAL INFORMATION
<!--- Include additional information to help people understand the change here -->
assert always produces verbose output, and sometimes this isn't desired (e.g., with with_items).  This change allows one to add quiet: True to assert to eliminate the verbose output.
<!--- A step-by-step reproduction of the problem is helpful if there is no related issue -->

<!--- Paste verbatim command output below, e.g. before and after your change -->
```paste below
Before:
$ cat test2.yml 
---
- hosts: localhost
  tasks:
  - assert:
      that: "{{ item }} is defined"
#      quiet: True
    with_items:
      - item_A
      - item_B

$ ansible-playbook -i "localhost," -c local test2.yml -e'item_A=aaa'

PLAY [localhost] ********************************************************************************

TASK [Gathering Facts] **************************************************************************
ok: [localhost]

TASK [assert] ***********************************************************************************
ok: [localhost] => (item=item_A) => {
    "changed": false, 
    "item": "item_A", 
    "msg": "All assertions passed"
}
failed: [localhost] (item=item_B) => {
    "assertion": "item_B is defined", 
    "changed": false, 
    "evaluated_to": false, 
    "item": "item_B", 
    "msg": "Assertion failed"
}

PLAY RECAP **************************************************************************************
localhost                  : ok=1    changed=0    unreachable=0    failed=1   

After (i.e., with quiet: True):
$ cat test2.yml 
---
- hosts: localhost
  tasks:
  - assert:
      that: "{{ item }} is defined"
      quiet: True
    with_items:
      - item_A
      - item_B

$ ansible-playbook -i "localhost," -c local test2.yml -e'item_A=aaa'

PLAY [localhost] ********************************************************************************

TASK [Gathering Facts] **************************************************************************
ok: [localhost]

TASK [assert] ***********************************************************************************
ok: [localhost] => (item=item_A)
failed: [localhost] (item=item_B) => {"assertion": "item_B is defined", "changed": false, "evaluated_to": false, "item": "item_B", "msg": "Assertion failed"}

PLAY RECAP **************************************************************************************
localhost                  : ok=1    changed=0    unreachable=0    failed=1   

```


Problem statement:
Assert module should not always be verbose
##### ISSUE TYPE
 - Bug Report

##### COMPONENT NAME
assert module

##### ANSIBLE VERSION
```
ansible 2.3.1.0
  config file = <redacted>/ansible.cfg
  configured module search path = Default w/o overrides
  python version = 2.7.12+ (default, Sep 17 2016, 12:08:02) [GCC 6.2.0 20160914]
```

##### CONFIGURATION
Nothing relevant.

##### OS / ENVIRONMENT
N/A

##### SUMMARY
When using the assert module, full output is produced, which clutters the normal Ansible output and can potentially reveal sensitive information. Using `no_log` blocks all useful information, so that's not a preferred option.

##### STEPS TO REPRODUCE
Use assert

<!--- Paste example playbooks or commands between quotes below -->
```yaml
- name: "check_configuration : Ensure CLOUD_ENABLED is boolean"
  assert:
    that:
     - "CLOUD_ENABLED is defined"
     - "CLOUD_ENABLED|string in ['True', 'False']"
    msg: "Must be boolean: {{ CLOUD_ENABLED|default('undefined') }}"
```

##### EXPECTED RESULTS
Something like:
```
TASK [common : check_configuration : Ensure CLOUD_ENABLED is a boolean] *******************************************************************************************************************
ok: [some_host]
```

##### ACTUAL RESULTS
```
TASK [common : check_configuration : Ensure CLOUD_ENABLED is a boolean] *******************************************************************************************************************
ok: [some_host] => {
    "changed": false, 
    "msg": "All assertions passed"
}
```

It's even worse when using constructs like `with_items`, where it also shows the full item information.

This is all caused by the following line in plugins/action/assert.py:
```
result['_ansible_verbose_always'] = True
```

This should either be removed, or be configurable through an argument to the module.

Hints from issue discussion:
Hrm, this was added explicitly in #17654, but removing it still seems to display the failure message fine on both default and minimal stdout plugins. Not sure why we'd always want to display the success output- @tima, can you shed any light?

If what bothers you is 'changed' and other fields aside from message, we can do the same as we do with `debug` and filter those out.

As per item, the `loop_control: label ` feature should help

I would reclassify this as 'feature request' `quiet=no|yes`

I think fixing the overly verbose output is a bug, but certainly adding a new flag would be a feature request. For me personally, now that I think about it, I don't see any value in the quiet flag at all as it really adds nothing (if you want more details, just run with extra verbosity).
It's been a while and unfortunately that issue didn't capture exactly why that was a good idea. I know I didn't come up with it in a vacuum and probably worked that out on IRC or something before sending the PR. 

I seem to recall it had to do with getting a message to know which assertion had failed when multiple assertion clauses where in use. It also struck me as odd that you defined a `msg` and is didn't display unless you where running in verbose mode. I think the compromise to avoiding special logic in the callback plugin was to just throw it into verbose. 

I could be mistaken, but I believe it was something like that. Things have changed a bit since then that there may be a better way now. 
@tima I think things have changed. I commented out the relevant line, and it behaves exactly as I would expect it to. That is, when there is no failure, it produces an "ok" line as other tasks would. When there is a failure, it dumps all of the relevant information, as other tasks would.
What does the output of a failure look like? What is included in a dump of all?
@tima, I don't have access to my work computer right now, but my memory is that it shows the JSON output of the assert module in red. It does not include newlines, but it does include all of the fields (e.g., "changed", "failure", "msg").
Is `assertion` in the return when it fails?

```
fatal: [localhost]: FAILED! => {
    "assertion": "CLOUD_ENABLED is defined", 
    "changed": false, 
    "evaluated_to": false, 
    "failed": true, 
    "msg": "Must be boolean: undefined"
}
```

That module's docs probably needs some clarification that the `msg` is an error/failure message since it doesn't get displayed on success. It would be more helpful if the value of `assertion` could be used in the `msg`. Also noticed in testing this that only the first assertion that failed is listed even with `ignore_errors: yes`.
When I'm writing these, I write follow-up assertions that assume the earlier ones succeeded. Multiple failure messages would not be useful to me. But whatever way it works, it should be documented.

That is what I see when it fails.
I woudl also check the `fail` module just to make sure they are congruent

Commit message: fixing CI complaints

Patch to review:
diff --git a/changelogs/fragments/52032-add_quiet_option_to_assert.yml b/changelogs/fragments/52032-add_quiet_option_to_assert.yml
new file mode 100644
index 00000000000000..e640aea5e69d8f
--- /dev/null
+++ b/changelogs/fragments/52032-add_quiet_option_to_assert.yml
@@ -0,0 +1,3 @@
+minor_changes:
+  - assert - added ``quiet`` option to the ``assert`` module to avoid verbose
+    output (https://github.com/ansible/ansible/issues/27124).
diff --git a/lib/ansible/modules/utilities/logic/assert.py b/lib/ansible/modules/utilities/logic/assert.py
index b45bb729870e34..58adb022afbb2b 100644
--- a/lib/ansible/modules/utilities/logic/assert.py
+++ b/lib/ansible/modules/utilities/logic/assert.py
@@ -24,20 +24,26 @@
 options:
   that:
     description:
-      - "A string expression of the same form that can be passed to the 'when' statement"
-      - "Alternatively, a list of string expressions"
+      - "A string expression of the same form that can be passed to the 'when' statement."
+      - "Alternatively, a list of string expressions."
     required: true
   fail_msg:
     version_added: "2.7"
     description:
-      - "The customized message used for a failing assertion"
-      - "This argument was called 'msg' before version 2.7, now it's renamed to 'fail_msg' with alias 'msg'"
+      - "The customized message used for a failing assertion."
+      - "This argument was called 'msg' before version 2.7, now it's renamed to 'fail_msg' with alias 'msg'."
     aliases:
       - msg
   success_msg:
     version_added: "2.7"
     description:
-      - "The customized message used for a successful assertion"
+      - "The customized message used for a successful assertion."
+  quiet:
+    version_added: "2.8"
+    description:
+      - "Set this to C(true) to avoid verbose output."
+    type: bool
+    default: false
 notes:
      - This module is also supported for Windows targets.
 author:
@@ -67,4 +73,11 @@
       - "my_param <= 100"
       - "my_param >= 0"
     msg: "'my_param' must be between 0 and 100"
+
+- name: use quiet to avoid verbose output
+  assert:
+    that:
+      - "my_param <= 100"
+      - "my_param >= 0"
+    quiet: true
 '''
diff --git a/lib/ansible/plugins/action/assert.py b/lib/ansible/plugins/action/assert.py
index a3b6af811a3ca8..1190ec984a8e63 100644
--- a/lib/ansible/plugins/action/assert.py
+++ b/lib/ansible/plugins/action/assert.py
@@ -21,13 +21,14 @@
 from ansible.playbook.conditional import Conditional
 from ansible.plugins.action import ActionBase
 from ansible.module_utils.six import string_types
+from ansible.module_utils.parsing.convert_bool import boolean
 
 
 class ActionModule(ActionBase):
     ''' Fail with custom message '''
 
     TRANSFERS_FILES = False
-    _VALID_ARGS = frozenset(('fail_msg', 'msg', 'success_msg', 'that'))
+    _VALID_ARGS = frozenset(('fail_msg', 'msg', 'quiet', 'success_msg', 'that'))
 
     def run(self, tmp=None, task_vars=None):
         if task_vars is None:
@@ -54,6 +55,8 @@ def run(self, tmp=None, task_vars=None):
         elif not isinstance(success_msg, string_types):
             raise AnsibleError('Incorrect type for success_msg, expected string and got %s' % type(success_msg))
 
+        quiet = boolean(self._task.args.get('quiet', False), strict=False)
+
         # make sure the 'that' items are a list
         thats = self._task.args['that']
         if not isinstance(thats, list):
@@ -65,7 +68,9 @@ def run(self, tmp=None, task_vars=None):
         # by this point, and is not used again, so we don't care about mangling
         # that value now
         cond = Conditional(loader=self._loader)
-        result['_ansible_verbose_always'] = True
+        if not quiet:
+            result['_ansible_verbose_always'] = True
+
         for that in thats:
             cond.when = [that]
             test_result = cond.evaluate_conditional(templar=self._templar, all_vars=task_vars)
diff --git a/test/integration/targets/assert/aliases b/test/integration/targets/assert/aliases
new file mode 100644
index 00000000000000..b59832142f2af6
--- /dev/null
+++ b/test/integration/targets/assert/aliases
@@ -0,0 +1 @@
+shippable/posix/group3
diff --git a/test/integration/targets/assert/assert_quiet.out.quiet.stderr b/test/integration/targets/assert/assert_quiet.out.quiet.stderr
new file mode 100644
index 00000000000000..bd973b04e02e0a
--- /dev/null
+++ b/test/integration/targets/assert/assert_quiet.out.quiet.stderr
@@ -0,0 +1,2 @@
++ ansible-playbook -i localhost, -c local quiet.yml
+++ set +x
diff --git a/test/integration/targets/assert/assert_quiet.out.quiet.stdout b/test/integration/targets/assert/assert_quiet.out.quiet.stdout
new file mode 100644
index 00000000000000..65b82a6f28397d
--- /dev/null
+++ b/test/integration/targets/assert/assert_quiet.out.quiet.stdout
@@ -0,0 +1,16 @@
+
+PLAY [localhost] ***************************************************************
+
+TASK [assert] ******************************************************************
+ok: [localhost] => (item=item_A)
+
+TASK [assert] ******************************************************************
+ok: [localhost] => (item=item_A) => {
+    "changed": false,
+    "item": "item_A",
+    "msg": "All assertions passed"
+}
+
+PLAY RECAP *********************************************************************
+localhost                  : ok=2    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0
+
diff --git a/test/integration/targets/assert/inventory b/test/integration/targets/assert/inventory
new file mode 100644
index 00000000000000..161820042a03f8
--- /dev/null
+++ b/test/integration/targets/assert/inventory
@@ -0,0 +1,3 @@
+[all]
+localhost
+
diff --git a/test/integration/targets/assert/quiet.yml b/test/integration/targets/assert/quiet.yml
new file mode 100644
index 00000000000000..6834712c2c1ba8
--- /dev/null
+++ b/test/integration/targets/assert/quiet.yml
@@ -0,0 +1,16 @@
+---
+- hosts: localhost
+  gather_facts: False
+  vars:
+    item_A: yes
+  tasks:
+  - assert:
+      that: "{{ item }} is defined"
+      quiet: True
+    with_items:
+      - item_A
+  - assert:
+      that: "{{ item }} is defined"
+      quiet: False
+    with_items:
+      - item_A
diff --git a/test/integration/targets/assert/runme.sh b/test/integration/targets/assert/runme.sh
new file mode 100755
index 00000000000000..8598a22f36533b
--- /dev/null
+++ b/test/integration/targets/assert/runme.sh
@@ -0,0 +1,67 @@
+#!/usr/bin/env bash
+
+# This test compares "known good" output with various settings against output
+# with the current code. It's brittle by nature, but this is probably the
+# "best" approach possible.
+#
+# Notes:
+# * options passed to this script (such as -v) are ignored, as they would change
+#   the output and break the test
+# * the number of asterisks after a "banner" differs is forced to 79 by
+#   redirecting stdin from /dev/null
+
+set -eux
+
+run_test() {
+   # testname is playbook name
+   local testname=$1
+
+   # The shenanigans with redirection and 'tee' are to capture STDOUT and
+   # STDERR separately while still displaying both to the console
+   { ansible-playbook -i 'localhost,' -c local "${testname}.yml" \
+      > >(set +x; tee "${OUTFILE}.${testname}.stdout"); } \
+      2> >(set +x; tee "${OUTFILE}.${testname}.stderr" >&2) 0</dev/null
+   # clean up Python 2 vs 3 differences
+   sed -i 's/ *$//' "${OUTFILE}.${testname}.stdout"
+   sed -i 's/ *$//' "${OUTFILE}.${testname}.stdout"
+   diff -u "${ORIGFILE}.${testname}.stdout" "${OUTFILE}.${testname}.stdout" || diff_failure
+   diff -u "${ORIGFILE}.${testname}.stderr" "${OUTFILE}.${testname}.stderr" || diff_failure
+}
+
+diff_failure() {
+   if [[ $INIT = 0 ]]; then
+      echo "FAILURE...diff mismatch!"
+      exit 1
+   fi
+}
+
+cleanup() {
+   if [[ $INIT = 0 ]]; then
+      rm -f "${OUTFILE}."*
+   fi
+}
+
+BASEFILE=assert_quiet.out
+
+ORIGFILE="${BASEFILE}"
+OUTFILE="${BASEFILE}.new"
+
+trap 'cleanup' EXIT
+
+# The --init flag will (re)generate the "good" output files used by the tests
+INIT=0
+if [[ ${1:-} == "--init" ]]; then
+   shift
+   OUTFILE=$ORIGFILE
+   INIT=1
+fi
+
+# Force the 'default' callback plugin
+export ANSIBLE_STDOUT_CALLBACK=default
+# Disable color in output for consistency
+export ANSIBLE_FORCE_COLOR=0
+export ANSIBLE_NOCOLOR=1
+# Disable retry files
+export ANSIBLE_RETRY_FILES_ENABLED=0
+
+run_test quiet


Code review comments:
diff_hunk: @@ -22,8 +22,8 @@ run_test() {
       > >(set +x; tee "${OUTFILE}.${testname}.stdout"); } \
       2> >(set +x; tee "${OUTFILE}.${testname}.stderr" >&2) 0</dev/null
    # clean up Python 2 vs 3 differences
-   sed -i 's/[[:blank:]]*$//' "${OUTFILE}.${testname}.stdout"
-   sed -i 's/[[:blank:]]*$//' "${OUTFILE}.${testname}.stderr"
+   sed -i 's/ *$//' "${OUTFILE}.${testname}.stdout"
+   sed -i 's/ *$//' "${OUTFILE}.${testname}.stdout"
path: test/integration/targets/assert/runme.sh
text: @unknown:
Now you're processing the stdout file twice (instead of once stdout and once stderr).

@author:
Yes, and apparently -i requires a suffix on OSX.

@unknown:
It doesn't work that simply, though. On OSX, you need to do `sed -i '' 's/...` (note the space between `-i` and `''`), but that won't work under Linux.