Add better error when k=v syntax is used with YAML in tasks (#41754)

* Add error message for k=v and YAML in a single task

Find the correct line, column, and position for k=v errors since they are different than the position reported initially.

Document bug in quoting syntax check.

* Change tense or error message

Since the error still exists, switch to present tense rather than past tense.

* Remove double spaces after periods in error messages.

http://www.slate.com/articles/technology/technology/2011/01/space_invaders.html

* Add changelog fragment

* Add tests for new error message

* Fix tests

* Add clarifying comments to unit test

changelogs/fragments/better-kv-error-reporting.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - properly report errors when k=v syntax is mixed with YAML syntax in a task (https://github.com/ansible/ansible/issues/27210)

lib/ansible/errors/__init__.py

@@ -19,8 +19,9 @@
 from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
-import traceback
+import re
 import sys
+import traceback
 
 from ansible.errors.yaml_strings import (
     YAML_COMMON_DICT_ERROR,
@@ -30,6 +31,7 @@ from ansible.errors.yaml_strings import (
     YAML_COMMON_UNQUOTED_COLON_ERROR,
     YAML_COMMON_UNQUOTED_VARIABLE_ERROR,
     YAML_POSITION_DETAILS,
+    YAML_AND_SHORTHAND_ERROR,
 )
 from ansible.module_utils._text import to_native, to_text
 from ansible.module_utils.common._collections_compat import Sequence
@@ -120,9 +122,18 @@ class AnsibleError(Exception):
                 prev_line = to_text(prev_line)
                 if target_line:
                     stripped_line = target_line.replace(" ", "")
-                    arrow_line = (" " * (col_number - 1)) + "^ here"
-                    # header_line = ("=" * 73)
-                    error_message += "\nThe offending line appears to be:\n\n%s\n%s\n%s\n" % (prev_line.rstrip(), target_line.rstrip(), arrow_line)
+
+                    # Check for k=v syntax in addition to YAML syntax and set the appropriate error position,
+                    # arrow index
+                    if re.search(r'\w+(\s+)?=(\s+)?[\w/-]+', prev_line):
+                        error_position = prev_line.rstrip().find('=')
+                        arrow_line = (" " * error_position) + "^ here"
+                        error_message = YAML_POSITION_DETAILS % (src_file, line_number - 1, error_position + 1)
+                        error_message += "\nThe offending line appears to be:\n\n%s\n%s\n\n" % (prev_line.rstrip(), arrow_line)
+                        error_message += YAML_AND_SHORTHAND_ERROR
+                    else:
+                        arrow_line = (" " * (col_number - 1)) + "^ here"
+                        error_message += "\nThe offending line appears to be:\n\n%s\n%s\n%s\n" % (prev_line.rstrip(), target_line.rstrip(), arrow_line)
 
                     # TODO: There may be cases where there is a valid tab in a line that has other errors.
                     if '\t' in target_line:
@@ -143,6 +154,9 @@ class AnsibleError(Exception):
                         error_message += YAML_COMMON_UNQUOTED_COLON_ERROR
                     # otherwise, check for some common quoting mistakes
                     else:
+                        # FIXME: This needs to split on the first ':' to account for modules like lineinfile
+                        # that may have lines that contain legitimate colons, e.g., line: 'i ALL= (ALL) NOPASSWD: ALL'
+                        # and throw off the quote matching logic.
                         parts = target_line.split(":")
                         if len(parts) > 1:
                             middle = parts[1].strip()

lib/ansible/errors/yaml_strings.py

@@ -9,11 +9,11 @@
 #
 # Ansible is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
-# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
+# along with Ansible. If not, see <http://www.gnu.org/licenses/>.
 
 # Make coding more python3-ish
 from __future__ import (absolute_import, division, print_function)
@@ -34,13 +34,13 @@ Syntax Error while loading YAML.
   %s"""
 
 YAML_POSITION_DETAILS = """\
-The error appears to have been in '%s': line %s, column %s, but may
+The error appears to be in '%s': line %s, column %s, but may
 be elsewhere in the file depending on the exact syntax problem.
 """
 
 YAML_COMMON_DICT_ERROR = """\
-This one looks easy to fix.  YAML thought it was looking for the start of a
-hash/dictionary and was confused to see a second "{".  Most likely this was
+This one looks easy to fix. YAML thought it was looking for the start of a
+hash/dictionary and was confused to see a second "{". Most likely this was
 meant to be an ansible template evaluation instead, so we have to give the
 parser a small hint that we wanted a string instead. The solution here is to
 just quote the entire value.
@@ -56,7 +56,7 @@ It should be written as:
 
 YAML_COMMON_UNQUOTED_VARIABLE_ERROR = """\
 We could be wrong, but this one looks like it might be an issue with
-missing quotes.  Always quote template expression brackets when they
+missing quotes. Always quote template expression brackets when they
 start a value. For instance:
 
     with_items:
@@ -69,7 +69,7 @@ Should be written as:
 """
 
 YAML_COMMON_UNQUOTED_COLON_ERROR = """\
-This one looks easy to fix.  There seems to be an extra unquoted colon in the line
+This one looks easy to fix. There seems to be an extra unquoted colon in the line
 and this is confusing the parser. It was only expecting to find one free
 colon. The solution is just add some quotes around the colon, or quote the
 entire line after the first colon.
@@ -88,9 +88,9 @@ Or:
 """
 
 YAML_COMMON_PARTIALLY_QUOTED_LINE_ERROR = """\
-This one looks easy to fix.  It seems that there is a value started
+This one looks easy to fix. It seems that there is a value started
 with a quote, and the YAML parser is expecting to see the line ended
-with the same kind of quote.  For instance:
+with the same kind of quote. For instance:
 
     when: "ok" in result.stdout
 
@@ -105,8 +105,8 @@ Or equivalently:
 
 YAML_COMMON_UNBALANCED_QUOTES_ERROR = """\
 We could be wrong, but this one looks like it might be an issue with
-unbalanced quotes.  If starting a value with a quote, make sure the
-line ends with the same set of quotes.  For instance this arbitrary
+unbalanced quotes. If starting a value with a quote, make sure the
+line ends with the same set of quotes. For instance this arbitrary
 example:
 
     foo: "bad" "wolf"
@@ -133,3 +133,8 @@ Should be written as:
         version: 1.2.3
 # ^--- all spaces here.
 """
+
+YAML_AND_SHORTHAND_ERROR = """\
+There appears to be both 'k=v' shorthand syntax and YAML in this task. \
+Only one syntax may be used.
+"""

test/units/errors/test_errors.py

@@ -48,6 +48,31 @@ class TestErrors(unittest.TestCase):
         self.assertEqual(e.message, self.unicode_message)
         self.assertEqual(e.__repr__(), self.unicode_message)
 
+    @patch.object(AnsibleError, '_get_error_lines_from_file')
+    def test_error_with_kv(self, mock_method):
+        ''' This tests a task with both YAML and k=v syntax
+
+        - lineinfile: line=foo path=bar
+            line: foo
+
+        An accurate error message and position indicator are expected.
+
+        _get_error_lines_from_file() returns (target_line, prev_line)
+        '''
+
+        self.obj.ansible_pos = ('foo.yml', 2, 1)
+
+        mock_method.return_value = ['    line: foo\n', '- lineinfile: line=foo path=bar\n']
+
+        e = AnsibleError(self.message, self.obj)
+        self.assertEqual(
+            e.message,
+            ("This is the error message\n\nThe error appears to be in 'foo.yml': line 1, column 19, but may\nbe elsewhere in the "
+             "file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n- lineinfile: line=foo path=bar\n"
+             "                  ^ here\n\n"
+             "There appears to be both 'k=v' shorthand syntax and YAML in this task. Only one syntax may be used.\n")
+        )
+
     @patch.object(AnsibleError, '_get_error_lines_from_file')
     def test_error_with_object(self, mock_method):
         self.obj.ansible_pos = ('foo.yml', 1, 1)
@@ -57,7 +82,7 @@ class TestErrors(unittest.TestCase):
 
         self.assertEqual(
             e.message,
-            ("This is the error message\n\nThe error appears to have been in 'foo.yml': line 1, column 1, but may\nbe elsewhere in the file depending on the "
+            ("This is the error message\n\nThe error appears to be in 'foo.yml': line 1, column 1, but may\nbe elsewhere in the file depending on the "
              "exact syntax problem.\n\nThe offending line appears to be:\n\n\nthis is line 1\n^ here\n")
         )
 
@@ -71,7 +96,7 @@ class TestErrors(unittest.TestCase):
             e = AnsibleError(self.message, self.obj)
             self.assertEqual(
                 e.message,
-                ("This is the error message\n\nThe error appears to have been in 'foo.yml': line 1, column 1, but may\nbe elsewhere in the file depending on "
+                ("This is the error message\n\nThe error appears to be in 'foo.yml': line 1, column 1, but may\nbe elsewhere in the file depending on "
                  "the exact syntax problem.\n\nThe offending line appears to be:\n\n\nthis is line 1\n^ here\n")
             )
 
@@ -80,7 +105,7 @@ class TestErrors(unittest.TestCase):
             e = AnsibleError(self.message, self.obj)
             self.assertEqual(
                 e.message,
-                ("This is the error message\n\nThe error appears to have been in 'foo.yml': line 2, column 1, but may\nbe elsewhere in the file depending on "
+                ("This is the error message\n\nThe error appears to be in 'foo.yml': line 2, column 1, but may\nbe elsewhere in the file depending on "
                  "the exact syntax problem.\n\n(specified line no longer in file, maybe it changed?)")
             )
 
@@ -93,7 +118,7 @@ class TestErrors(unittest.TestCase):
             e = AnsibleError(self.unicode_message, self.obj)
             self.assertEqual(
                 e.message,
-                ("This is an error with \xf0\x9f\x98\xa8 in it\n\nThe error appears to have been in 'foo.yml': line 1, column 1, but may\nbe elsewhere in the "
+                ("This is an error with \xf0\x9f\x98\xa8 in it\n\nThe error appears to be in 'foo.yml': line 1, column 1, but may\nbe elsewhere in the "
                  "file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\nthis line has unicode \xf0\x9f\x98\xa8 in it!\n^ "
                  "here\n")
             )

test/units/playbook/test_task.py

@@ -84,11 +84,11 @@ class TestTask(unittest.TestCase):
         self.assertIsInstance(cm.exception, errors.AnsibleParserError)
         self.assertEqual(cm.exception._obj, ds)
         self.assertEqual(cm.exception._obj, kv_bad_args_ds)
-        self.assertIn("The error appears to have been in 'test_task_faux_playbook.yml", cm.exception.message)
+        self.assertIn("The error appears to be in 'test_task_faux_playbook.yml", cm.exception.message)
         self.assertIn(kv_bad_args_str, cm.exception.message)
         self.assertIn('apk', cm.exception.message)
         self.assertEquals(cm.exception.message.count('The offending line'), 1)
-        self.assertEquals(cm.exception.message.count('The error appears to have been in'), 1)
+        self.assertEquals(cm.exception.message.count('The error appears to be in'), 1)
 
     def test_task_auto_name(self):
         assert 'name' not in kv_command_task