Merge file comments from multiple tasks

When multiple tasks (or roles) return file_comments for the
same file via zuul_return only the last value is taken into account.

This uses a similar approach like for artifacts to merge the
file comments of multiple zuul_return calls. With that, all file
comments are merged additive into a final data structure. File comments
for the same file and/or the same line are kept distinct and don't
override each other.

Change-Id: Ib7300c243deaeb7922182a94cfa41e617fc4e6e6

doc/source/reference/jobs.rst

@@ -906,6 +906,9 @@ may be disabled by setting the
 **zuul.disable_file_comment_line_mapping** variable to ``true`` in
 *zuul_return*.
 
+If *zuul_return* is invoked multiple times (e.g., via multiple playbooks), then
+the elements of `zuul.file_comments` from each invocation will be appended.
+
 Pausing the job
 ~~~~~~~~~~~~~~~
 

tests/fixtures/config/gerrit-file-comments/git/org_project/playbooks/file-comments.yaml

@@ -12,12 +12,26 @@
                 - line: 2
                   message: levels are ignored by gerrit
                   level: warning
+              missingfile.txt:
+                - line: 89
+                  message: does not exist
+    - name: Another task using zuul_return
+      zuul_return:
+        data:
+          zuul:
+            file_comments:
               otherfile.txt:
                 - line: 21
                   message: |
                     This is a much longer message.
 
                     With multiple paragraphs.
-              missingfile.txt:
-                - line: 89
-                  message: does not exist
+              path/to/file.py:
+                - line: 21
+                  message: |
+                    A second zuul return value using the same file should not
+                    override the first result, but both should be merged.
+                - line: 42
+                  message: |
+                    A second comment applied to the same line in the same file
+                    should also be added to the result.

tests/unit/test_gerrit.py

@@ -246,7 +246,7 @@ class TestFileComments(AnsibleZuulTestCase):
                          'SUCCESS')
         self.assertEqual(self.getJobFromHistory('file-comments-error').result,
                          'SUCCESS')
-        self.assertEqual(len(A.comments), 4)
+        self.assertEqual(len(A.comments), 6)
         comments = sorted(A.comments, key=lambda x: x['line'])
         self.assertEqual(
             comments[0],
@@ -261,7 +261,25 @@ class TestFileComments(AnsibleZuulTestCase):
                 },
             },
         )
-        self.assertEqual(comments[1],
+
+        self.assertEqual(
+            comments[1],
+            {
+                "file": "path/to/file.py",
+                "line": 21,
+                "message": (
+                    "A second zuul return value using the same file should not"
+                    "\noverride the first result, but both should be merged.\n"
+                ),
+                "reviewer": {
+                    "email": "zuul@example.com",
+                    "name": "Zuul",
+                    "username": "jenkins",
+                },
+            },
+        )
+
+        self.assertEqual(comments[2],
                          {'file': 'otherfile.txt',
                           'line': 21,
                           'message': 'This is a much longer message.\n\n'
@@ -270,7 +288,8 @@ class TestFileComments(AnsibleZuulTestCase):
                                        'name': 'Zuul',
                                        'username': 'jenkins'}}
         )
-        self.assertEqual(comments[2],
+
+        self.assertEqual(comments[3],
                          {'file': 'path/to/file.py',
                           'line': 42,
                           'message': 'line too long',
@@ -278,7 +297,25 @@ class TestFileComments(AnsibleZuulTestCase):
                                        'name': 'Zuul',
                                        'username': 'jenkins'}}
         )
-        self.assertEqual(comments[3],
+
+        self.assertEqual(
+            comments[4],
+            {
+                "file": "path/to/file.py",
+                "line": 42,
+                "message": (
+                    "A second comment applied to the same line in the same "
+                    "file\nshould also be added to the result.\n"
+                ),
+                "reviewer": {
+                    "email": "zuul@example.com",
+                    "name": "Zuul",
+                    "username": "jenkins",
+                }
+            }
+        )
+
+        self.assertEqual(comments[5],
                          {'file': 'path/to/file.py',
                           'line': 82,
                           'message': 'line too short',

zuul/ansible/base/actiongeneral/zuul_return.py

@@ -16,6 +16,7 @@
 import os
 import json
 import tempfile
+from copy import deepcopy
 
 from ansible.plugins.action import ActionBase
 
@@ -42,19 +43,52 @@ def merge_data(dict_a, dict_b):
     """
     Merge dict_a into dict_b, handling any special cases for zuul variables
     """
-    artifacts_a = dict_a.get('zuul', {}).get('artifacts', [])
-    if not isinstance(artifacts_a, list):
-        artifacts_a = []
-    artifacts_b = dict_b.get('zuul', {}).get('artifacts', [])
-    if not isinstance(artifacts_b, list):
-        artifacts_b = []
-    artifacts = artifacts_a + artifacts_b
+    artifacts = merge_artifacts(dict_a, dict_b)
+    file_comments = merge_file_comments(dict_a, dict_b)
     merge_dict(dict_a, dict_b)
     if artifacts:
         dict_b.setdefault('zuul', {})['artifacts'] = artifacts
+    if file_comments:
+        dict_b.setdefault("zuul", {})["file_comments"] = file_comments
     return dict_b
 
 
+def merge_artifacts(dict_a, dict_b):
+    """Merge artifacts from both dictionary
+
+    The artifacts from both dictionaries will be merged (additive) into a new
+    list.
+    """
+    artifacts_a = dict_a.get('zuul', {}).get("artifacts", [])
+    if not isinstance(artifacts_a, list):
+        artifacts_a = []
+    artifacts_b = dict_b.get('zuul', {}).get("artifacts", [])
+    if not isinstance(artifacts_b, list):
+        artifacts_b = []
+    artifacts = artifacts_a + artifacts_b
+    return artifacts
+
+
+def merge_file_comments(dict_a, dict_b):
+    """Merge file_comments from both dictionary.
+
+    File comments applied to the same line and/or file will be added to the
+    existing ones.
+    """
+    file_comments_a = dict_a.get('zuul', {}).get("file_comments", {})
+    if not isinstance(file_comments_a, dict):
+        file_comments_a = {}
+    file_comments_b = dict_b.get('zuul', {}).get("file_comments", {})
+    if not isinstance(file_comments_b, dict):
+        file_comments_b = {}
+    # Merge all comments for each file from b into a. In case a contains
+    # comments for the same file, both are merged and not overriden.
+    file_comments = deepcopy(file_comments_b)
+    for key, value in file_comments_a.items():
+        file_comments.setdefault(key, []).extend(value)
+    return file_comments
+
+
 def set_value(path, new_data, new_file):
     workdir = os.path.dirname(path)
     data = None