Support line comments in Gerrit

This documents the process for leaving line comments via a reporter
and implements it in the Gerrit reporter.

See https://review.openstack.org/579033 for a method of mapping to
the correct line numbers in an environment with speculative merging.

Change-Id: Ice396eeb51cea9c1c998c0fb6cc09572f945b31a

doc/source/user/jobs.rst

@@ -678,6 +678,27 @@ zuul.child_jobs is empty, all jobs will be marked as SKIPPED. Invalid child jobs
 are stripped and ignored, if only invalid jobs are listed it is the same as
 providing an empty list to zuul.child_jobs.
 
+Leaving file comments
+~~~~~~~~~~~~~~~~~~~~~
+
+To instruct the reporters to leave line comments on files in the
+change, set the **zuul.file_comments** value.  For example:
+
+.. code-block:: yaml
+
+  tasks:
+    - zuul_return:
+        data:
+          zuul:
+            file_comments:
+              path/to/file.py:
+                - line: 42
+                  message: "Line too long"
+                - line: 82
+                  message: "Line too short"
+
+Not all reporters currently support line comments; in these cases,
+reporters will simply ignore this data.
 
 .. _build_status:
 

releasenotes/notes/gerrit-line-comments-db364516af7030b1.yaml

@@ -0,0 +1,5 @@
+---
+features:
+  - |
+    Zuul now supports leaving file comments, though currently only when using
+    the Gerrit driver.  See the documentation for ``zuul_return`` for details.

tests/base.py

@@ -177,9 +177,10 @@ class FakeGerritChange(object):
         self.needed_by_changes = []
         self.fail_merge = False
         self.messages = []
+        self.comments = []
         self.data = {
             'branch': branch,
-            'comments': [],
+            'comments': self.comments,
             'commitMessage': subject,
             'createdOn': time.time(),
             'id': 'I' + random_sha1(),
@@ -272,6 +273,19 @@ class FakeGerritChange(object):
         self.patchsets.append(d)
         self.data['submitRecords'] = self.getSubmitRecords()
 
+    def addComment(self, filename, line, message, name, email, username):
+        comment = {
+            'file': filename,
+            'line': int(line),
+            'reviewer': {
+                'name': name,
+                'email': email,
+                'username': username,
+            },
+            'message': message,
+        }
+        self.comments.append(comment)
+
     def getPatchsetCreatedEvent(self, patchset):
         event = {"type": "patchset-created",
                  "change": {"project": self.project,
@@ -525,8 +539,9 @@ class GerritWebServer(object):
 
                 message = data['message']
                 action = data['labels']
+                comments = data.get('comments', {})
                 fake_gerrit._test_handle_review(
-                    int(change.data['number']), message, action)
+                    int(change.data['number']), message, action, comments)
                 self.send_response(200)
                 self.end_headers()
 
@@ -655,13 +670,14 @@ class FakeGerritConnection(gerritconnection.GerritConnection):
         }
         return event
 
-    def review(self, change, message, action):
+    def review(self, change, message, action, file_comments):
         if self.web_server:
             return super(FakeGerritConnection, self).review(
-                change, message, action)
+                change, message, action, file_comments)
         self._test_handle_review(int(change.number), message, action)
 
-    def _test_handle_review(self, change_number, message, action):
+    def _test_handle_review(self, change_number, message, action,
+                            file_comments=None):
         # Handle a review action from a test
         change = self.changes[change_number]
 
@@ -682,6 +698,12 @@ class FakeGerritConnection(gerritconnection.GerritConnection):
         if message:
             change.messages.append(message)
 
+        if file_comments:
+            for filename, commentlist in file_comments.items():
+                for comment in commentlist:
+                    change.addComment(filename, comment['line'],
+                                      comment['message'], 'Zuul',
+                                      'zuul@example.com', self.user)
         if 'submit' in action:
             change.setMerged()
         if message:

tests/fixtures/config/gerrit-file-comments/git/common-config/zuul.yaml

@@ -0,0 +1,17 @@
+- pipeline:
+    name: check
+    manager: independent
+    post-review: true
+    trigger:
+      gerrit:
+        - event: patchset-created
+    success:
+      gerrit:
+        Verified: 1
+    failure:
+      gerrit:
+        Verified: -1
+
+- job:
+    name: base
+    parent: null

tests/fixtures/config/gerrit-file-comments/git/org_project/README

@@ -0,0 +1 @@
+test

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

@@ -0,0 +1,17 @@
+- hosts: all
+  tasks:
+    - zuul_return:
+        data:
+          zuul:
+            file_comments:
+              path/to/file.py:
+                - line: 42
+                  message: line too long
+                - line: 82
+                  message: line too short
+              otherfile.txt:
+                - line: 21
+                  message: |
+                    This is a much longer message.
+
+                    With multiple paragraphs.

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

@@ -0,0 +1,10 @@
+- job:
+    parent: base
+    name: file-comments
+    run: playbooks/file-comments.yaml
+
+- project:
+    name: org/project
+    check:
+      jobs:
+        - file-comments

tests/fixtures/config/gerrit-file-comments/main.yaml

@@ -0,0 +1,8 @@
+- tenant:
+    name: tenant-one
+    source:
+      gerrit:
+        config-projects:
+          - common-config
+        untrusted-projects:
+          - org/project

tests/unit/test_gerrit.py

@@ -16,7 +16,7 @@ import os
 from unittest import mock
 
 import tests.base
-from tests.base import BaseTestCase, ZuulTestCase
+from tests.base import BaseTestCase, ZuulTestCase, AnsibleZuulTestCase
 from zuul.driver.gerrit import GerritDriver
 from zuul.driver.gerrit.gerritconnection import GerritConnection
 
@@ -100,3 +100,44 @@ class TestGerritWeb(ZuulTestCase):
                          'label1')
         self.assertEqual(self.getJobFromHistory('project-test2').node,
                          'label1')
+
+
+class TestFileComments(AnsibleZuulTestCase):
+    # A temporary class to hold new tests while others are disabled
+    config_file = 'zuul-gerrit-web.conf'
+    tenant_config_file = 'config/gerrit-file-comments/main.yaml'
+
+    def test_multiple_tenants(self):
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+        A.addApproval('Code-Review', 2)
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertEqual(self.getJobFromHistory('file-comments').result,
+                         'SUCCESS')
+        self.assertEqual(len(A.comments), 3)
+        comments = sorted(A.comments, key=lambda x: x['line'])
+        self.assertEqual(comments[0],
+                         {'file': 'otherfile.txt',
+                          'line': 21,
+                          'message': 'This is a much longer message.\n\n'
+                          'With multiple paragraphs.\n',
+                          'reviewer': {'email': 'zuul@example.com',
+                                       'name': 'Zuul',
+                                       'username': 'jenkins'}}
+        )
+        self.assertEqual(comments[1],
+                         {'file': 'path/to/file.py',
+                          'line': 42,
+                          'message': 'line too long',
+                          'reviewer': {'email': 'zuul@example.com',
+                                       'name': 'Zuul',
+                                       'username': 'jenkins'}}
+        )
+        self.assertEqual(comments[2],
+                         {'file': 'path/to/file.py',
+                          'line': 82,
+                          'message': 'line too short',
+                          'reviewer': {'email': 'zuul@example.com',
+                                       'name': 'Zuul',
+                                       'username': 'jenkins'}}
+        )

zuul/driver/gerrit/gerritconnection.py

@@ -776,14 +776,16 @@ class GerritConnection(BaseConnection):
     def eventDone(self):
         self.event_queue.task_done()
 
-    def review(self, change, message, action={}):
+    def review(self, change, message, action={},
+               file_comments={}):
         if self.session:
             meth = self.review_http
         else:
             meth = self.review_ssh
-        return meth(change, message, action)
+        return meth(change, message, action, file_comments)
 
-    def review_ssh(self, change, message, action={}):
+    def review_ssh(self, change, message, action={},
+                   file_comments={}):
         project = change.project.name
         cmd = 'gerrit review --project %s' % project
         if message:

zuul/driver/gerrit/gerritreporter.py

@@ -25,6 +25,17 @@ class GerritReporter(BaseReporter):
     name = 'gerrit'
     log = logging.getLogger("zuul.GerritReporter")
 
+    def _getFileComments(self, item):
+        ret = {}
+        for build in item.current_build_set.getBuilds():
+            fc = build.result_data.get('zuul', {}).get('file_comments')
+            if not fc:
+                continue
+            for fn, comments in fc.items():
+                existing_comments = ret.setdefault(fn, [])
+                existing_comments += comments
+        return ret
+
     def report(self, item):
         """Send a message to gerrit."""
 
@@ -39,13 +50,16 @@ class GerritReporter(BaseReporter):
             return
 
         message = self._formatItemReport(item)
+        comments = self._getFileComments(item)
 
-        self.log.debug("Report change %s, params %s, message: %s" %
-                       (item.change, self.config, message))
+        self.log.debug("Report change %s, params %s,"
+                       " message: %s, comments: %s" %
+                       (item.change, self.config, message, comments))
         item.change._ref_sha = item.change.project.source.getRefSha(
             item.change.project, 'refs/heads/' + item.change.branch)
 
-        return self.connection.review(item.change, message, self.config)
+        return self.connection.review(item.change, message, self.config,
+                                      comments)
 
     def getSubmitAllowNeeds(self):
         """Get a list of code review labels that are allowed to be