From 9efe95ff3ce7f3ab9007cfbdd3420bbfb00a2fa6 Mon Sep 17 00:00:00 2001
From: "James E. Blair" <jeblair@redhat.com>
Date: Wed, 24 Oct 2018 13:46:35 -0700
Subject: [PATCH] Filter file comments for existing files

Remove file comments for files which do not appear in the change
and leave a warning.

Change-Id: Ib5f3d5580a263c37812a797eae96b8470ce16d7b
---
 .../git/org_project/playbooks/file-comments.yaml  |  3 +++
 tests/unit/test_gerrit.py                         |  8 +++++++-
 zuul/driver/gerrit/gerritreporter.py              |  3 ++-
 zuul/reporter/__init__.py                         | 15 +++++++++++++++
 4 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/tests/fixtures/config/gerrit-file-comments/git/org_project/playbooks/file-comments.yaml b/tests/fixtures/config/gerrit-file-comments/git/org_project/playbooks/file-comments.yaml
index 9fd145dea6..b281af2332 100644
--- a/tests/fixtures/config/gerrit-file-comments/git/org_project/playbooks/file-comments.yaml
+++ b/tests/fixtures/config/gerrit-file-comments/git/org_project/playbooks/file-comments.yaml
@@ -15,3 +15,6 @@
                     This is a much longer message.
 
                     With multiple paragraphs.
+              missingfile.txt:
+                - line: 89
+                  message: does not exist
diff --git a/tests/unit/test_gerrit.py b/tests/unit/test_gerrit.py
index bf08ecb00a..c3a9298b32 100644
--- a/tests/unit/test_gerrit.py
+++ b/tests/unit/test_gerrit.py
@@ -192,7 +192,11 @@ class TestFileComments(AnsibleZuulTestCase):
     tenant_config_file = 'config/gerrit-file-comments/main.yaml'
 
     def test_file_comments(self):
-        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+        A = self.fake_gerrit.addFakeChange(
+            'org/project', 'master', 'A',
+            files={'path/to/file.py': 'test1',
+                   'otherfile.txt': 'test2',
+                   })
         A.addApproval('Code-Review', 2)
         self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
         self.waitUntilSettled()
@@ -229,3 +233,5 @@ class TestFileComments(AnsibleZuulTestCase):
         )
         self.assertIn('expected a dictionary', A.messages[0],
                       "A should have a validation error reported")
+        self.assertIn('invalid file missingfile.txt', A.messages[0],
+                      "A should have file error reported")
diff --git a/zuul/driver/gerrit/gerritreporter.py b/zuul/driver/gerrit/gerritreporter.py
index bfcc4b4893..86fdc72e7a 100644
--- a/zuul/driver/gerrit/gerritreporter.py
+++ b/zuul/driver/gerrit/gerritreporter.py
@@ -50,8 +50,9 @@ class GerritReporter(BaseReporter):
                 self.connection.canonical_hostname:
             return
 
-        message = self._formatItemReport(item)
         comments = self._getFileComments(item)
+        self.filterComments(item, comments)
+        message = self._formatItemReport(item)
 
         self.log.debug("Report change %s, params %s,"
                        " message: %s, comments: %s" %
diff --git a/zuul/reporter/__init__.py b/zuul/reporter/__init__.py
index e41ea278f8..3e9520c5b2 100644
--- a/zuul/reporter/__init__.py
+++ b/zuul/reporter/__init__.py
@@ -82,6 +82,21 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
                                               end_line=mark.end_line,
                                               end_character=mark.end_column)))
 
+    def filterComments(self, item, comments):
+        """Filter comments for files in change
+
+        Remove any comments for files which do not appear in the
+        item's change.  Leave warning messages if this happens.
+
+        :arg QueueItem item: The queue item
+        :arg dict comments: a file comments dictionary (modified in place)
+        """
+
+        for fn in list(comments.keys()):
+            if fn not in item.change.files:
+                del comments[fn]
+                item.warning("Comments left for invalid file %s" % (fn,))
+
     def _getFormatter(self):
         format_methods = {
             'start': self._formatItemReportStart,