Set fact fixes (#73684)

* fixes to set_fact

  correctly give error messages that were previously ignored
  corrected and expanded docs

4 files changed, 50 insertions, 31 deletions

diff --git a/changelogs/fragments/fix_set_fact.yml b/changelogs/fragments/fix_set_fact.yml
new file mode 100644
index 0000000000..0b739c8209
--- /dev/null
+++ b/changelogs/fragments/fix_set_fact.yml
@@ -0,0 +1,2 @@
+bugfixes:
+  - '[set_fact] Corrected and expanded documentation as well as now raise errors that were previously ignored.'
diff --git a/lib/ansible/modules/set_fact.py b/lib/ansible/modules/set_fact.py
index 0f5300d070..fe8eadf1a6 100644
--- a/lib/ansible/modules/set_fact.py
+++ b/lib/ansible/modules/set_fact.py
@@ -11,22 +11,20 @@ __metaclass__ = type
 DOCUMENTATION = r'''
 ---
 module: set_fact
-short_description: Set host facts from a task
+short_description: Set host variable(s) and fact(s).
 version_added: "1.2"
 description:
-    - This module allows setting new variables.
-    - Variables are set on a host-by-host basis just like facts discovered by the setup module.
-    - These variables will be available to subsequent plays during an ansible-playbook run.
-    - Set C(cacheable) to C(yes) to save variables across executions
-      using a fact cache. Variables created with set_fact have different precedence depending on whether they are or are not cached.
-    - Per the standard Ansible variable precedence rules, many other types of variables have a higher priority, so this value may be overridden.
-    - This module is also supported for Windows targets.
+    - This action allows setting variables associated to the current host.
+    - These variables will be available to subsequent plays during an ansible-playbook run via the host they were set on.
+    - Set C(cacheable) to C(yes) to save variables across executions using a fact cache.
+      Variables will keep the set_fact precedence for the current run, but will used 'cached fact' precedence for subsequent ones.
+    - Per the standard Ansible variable precedence rules, other types of variables have a higher priority, so this value may be overridden.
 options:
   key_value:
     description:
-      - The C(set_fact) module takes key=value pairs as variables to set
-        in the playbook scope. Or alternatively, accepts complex arguments
-        using the C(args:) statement.
+      - "The C(set_fact) module takes ``key=value`` pairs or ``key: value``(YAML notation) as variables to set in the playbook scope.
+        The 'key' is the resulting variable name and the value is, of course, the value of said variable."
+      - You can create multiple variables at once, by supplying multiple pairs, but do NOT mix notations.
     required: true
   cacheable:
     description:
@@ -41,10 +39,15 @@ options:
     default: no
     version_added: "2.4"
 notes:
-    - "The C(var=value) notation can only create strings or booleans.
-      If you want to create lists/arrays or dictionary/hashes use C(var: [val1, val2])."
-    - Since 'cacheable' is now a module param, 'cacheable' is no longer a valid fact name as of Ansible 2.4.
-    - This module is also supported for Windows targets.
+    - Because of the nature of tasks, set_fact will produce 'static' values for a variable.
+      Unlike normal 'lazy' variables, the value gets evaluated and templated on assignment.
+    - Some boolean values (yes, no, true, false) will always be converted to boolean type,
+      unless C(DEFAULT_JINJA2_NATIVE) is enabled.  This is done so the C(var=value) booleans,
+      otherwise it would only be able to create strings, but it also prevents using those values to create YAML strings.
+      Using the setting will restrict k=v to strings, but will allow you to specify string or boolean in YAML.
+    - "To create lists/arrays or dictionary/hashes use YAML notation C(var: [val1, val2])."
+    - Since 'cacheable' is now a module param, 'cacheable' is no longer a valid fact name.
+    - This action does not use a connection and always executes on the controller.
 seealso:
 - module: ansible.builtin.include_vars
 - ref: ansible_variable_precedence
@@ -54,7 +57,7 @@ author:
 '''
 
 EXAMPLES = r'''
-- name: Setting host facts using key=value pairs, note that this always creates strings or booleans
+- name: Setting host facts using key=value pairs, this format can only create strings or booleans
   set_fact: one_fact="something" other_fact="{{ local_var }}"
 
 - name: Setting host facts using complex arguments
@@ -69,12 +72,18 @@ EXAMPLES = r'''
     other_fact: "{{ local_var * 2 }}"
     cacheable: yes
 
-# As of Ansible 1.8, Ansible will convert boolean strings ('true', 'false', 'yes', 'no')
-# to proper boolean values when using the key=value syntax, however it is still
-# recommended that booleans be set using the complex argument style:
-- name:  Setting booleans using complex argument style
+- name: Creating list and dictionary variables
   set_fact:
-    one_fact: yes
-    other_fact: no
+    one_dict:
+        something: here
+        other: there
+    one_list:
+        - a
+        - b
+        - c
 
+- name: Creating list and dictionary variables using 'shorthand' YAML
+  set_fact:
+    two_dict: {'something': here2, 'other': somewhere}
+    two_list: [1,2,3]
 '''
diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py
index f492c08f05..9f5103be67 100644
--- a/lib/ansible/plugins/action/__init__.py
+++ b/lib/ansible/plugins/action/__init__.py
@@ -89,6 +89,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
         * Module parameters.  These are stored in self._task.args
         """
 
+        # does not default to {'changed': False, 'failed': False}, as it breaks async
         result = {}
 
         if tmp is not None:
diff --git a/lib/ansible/plugins/action/set_fact.py b/lib/ansible/plugins/action/set_fact.py
index d7fe573c1a..df1c0a43da 100644
--- a/lib/ansible/plugins/action/set_fact.py
+++ b/lib/ansible/plugins/action/set_fact.py
@@ -18,6 +18,7 @@
 from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
+from ansible.errors import AnsibleActionFail
 from ansible.module_utils.six import iteritems, string_types
 from ansible.module_utils.parsing.convert_bool import boolean
 from ansible.plugins.action import ActionBase
@@ -37,8 +38,7 @@ class ActionModule(ActionBase):
         result = super(ActionModule, self).run(tmp, task_vars)
         del tmp  # tmp no longer has any effect
 
-        facts = dict()
-
+        facts = {}
         cacheable = boolean(self._task.args.pop('cacheable', False))
 
         if self._task.args:
@@ -46,16 +46,23 @@ class ActionModule(ActionBase):
                 k = self._templar.template(k)
 
                 if not isidentifier(k):
-                    result['failed'] = True
-                    result['msg'] = ("The variable name '%s' is not valid. Variables must start with a letter or underscore character, and contain only "
-                                     "letters, numbers and underscores." % k)
-                    return result
+                    raise AnsibleActionFail("The variable name '%s' is not valid. Variables must start with a letter or underscore character, "
+                                            "and contain only letters, numbers and underscores." % k)
 
+                # NOTE: this should really use BOOLEANS from convert_bool, but only in the k=v case,
+                # right now it converts matching explicit YAML strings also when 'jinja2_native' is disabled.
                 if not C.DEFAULT_JINJA2_NATIVE and isinstance(v, string_types) and v.lower() in ('true', 'false', 'yes', 'no'):
                     v = boolean(v, strict=False)
                 facts[k] = v
+        else:
+            raise AnsibleActionFail('No key/value pairs provided, at least one is required for this action to succeed')
+
+        if facts:
+            # just as _facts actions, we don't set changed=true as we are not modifying the actual host
+            result['ansible_facts'] = facts
+            result['_ansible_facts_cacheable'] = cacheable
+        else:
+            # this should not happen, but JIC we get here
+            raise AnsibleActionFail('Unable to create any variables with provided arguments')
 
-        result['changed'] = False
-        result['ansible_facts'] = facts
-        result['_ansible_facts_cacheable'] = cacheable
         return result