Merge "Fix merging with submitWholeTopic"

8 files changed, 126 insertions, 110 deletions

diff --git a/zuul/driver/elasticsearch/reporter.py b/zuul/driver/elasticsearch/reporter.py
index e35bbcd48..7802cb609 100644
--- a/zuul/driver/elasticsearch/reporter.py
+++ b/zuul/driver/elasticsearch/reporter.py
@@ -30,8 +30,10 @@ class ElasticsearchReporter(BaseReporter):
         self.index_vars = self.config.get('index-vars')
         self.index_returned_vars = self.config.get('index-returned-vars')
 
-    def report(self, item):
+    def report(self, item, phase1=True, phase2=True):
         """Create an entry into a database."""
+        if not phase1:
+            return
         docs = []
         index = '%s.%s-%s' % (self.index, item.pipeline.tenant.name,
                               time.strftime("%Y.%m.%d"))
diff --git a/zuul/driver/gerrit/gerritconnection.py b/zuul/driver/gerrit/gerritconnection.py
index e3adfefb5..885b428aa 100644
--- a/zuul/driver/gerrit/gerritconnection.py
+++ b/zuul/driver/gerrit/gerritconnection.py
@@ -1205,16 +1205,17 @@ class GerritConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
         self.event_queue.put(event)
 
     def review(self, item, message, submit, labels, checks_api,
-               file_comments, zuul_event_id=None):
+               file_comments, phase1, phase2, zuul_event_id=None):
         if self.session:
             meth = self.review_http
         else:
             meth = self.review_ssh
         return meth(item, message, submit, labels, checks_api,
-                    file_comments, zuul_event_id=zuul_event_id)
+                    file_comments, phase1, phase2,
+                    zuul_event_id=zuul_event_id)
 
     def review_ssh(self, item, message, submit, labels, checks_api,
-                   file_comments, zuul_event_id=None):
+                   file_comments, phase1, phase2, zuul_event_id=None):
         log = get_annotated_logger(self.log, zuul_event_id)
         if checks_api:
             log.error("Zuul is configured to report to the checks API, "
@@ -1223,23 +1224,24 @@ class GerritConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
         change = item.change
         project = change.project.name
         cmd = 'gerrit review --project %s' % project
-        if message:
-            b_len = len(message.encode('utf-8'))
-            if b_len >= GERRIT_HUMAN_MESSAGE_LIMIT:
-                log.info("Message truncated %d > %d" %
-                         (b_len, GERRIT_HUMAN_MESSAGE_LIMIT))
-                message = ("%s... (truncated)" %
-                           message[:GERRIT_HUMAN_MESSAGE_LIMIT - 20])
-            cmd += ' --message %s' % shlex.quote(message)
-        if submit:
+        if phase1:
+            if message:
+                b_len = len(message.encode('utf-8'))
+                if b_len >= GERRIT_HUMAN_MESSAGE_LIMIT:
+                    log.info("Message truncated %d > %d" %
+                             (b_len, GERRIT_HUMAN_MESSAGE_LIMIT))
+                    message = ("%s... (truncated)" %
+                               message[:GERRIT_HUMAN_MESSAGE_LIMIT - 20])
+                cmd += ' --message %s' % shlex.quote(message)
+            for key, val in labels.items():
+                if val is True:
+                    cmd += ' --%s' % key
+                else:
+                    cmd += ' --label %s=%s' % (key, val)
+            if self.version >= (2, 13, 0):
+                cmd += ' --tag autogenerated:zuul:%s' % (item.pipeline.name)
+        if phase2 and submit:
             cmd += ' --submit'
-        for key, val in labels.items():
-            if val is True:
-                cmd += ' --%s' % key
-            else:
-                cmd += ' --label %s=%s' % (key, val)
-        if self.version >= (2, 13, 0):
-            cmd += ' --tag autogenerated:zuul:%s' % (item.pipeline.name)
         changeid = '%s,%s' % (change.number, change.patchset)
         cmd += ' %s' % changeid
         out, err = self._ssh(cmd, zuul_event_id=zuul_event_id)
@@ -1292,8 +1294,13 @@ class GerritConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
                     time.sleep(x * 10)
 
     def review_http(self, item, message, submit, labels,
-                    checks_api, file_comments, zuul_event_id=None):
+                    checks_api, file_comments, phase1, phase2,
+                    zuul_event_id=None):
         change = item.change
+        changeid = "%s~%s~%s" % (
+            urllib.parse.quote(str(change.project), safe=''),
+            urllib.parse.quote(str(change.branch), safe=''),
+            change.id)
         log = get_annotated_logger(self.log, zuul_event_id)
         b_len = len(message.encode('utf-8'))
         if b_len >= GERRIT_HUMAN_MESSAGE_LIMIT:
@@ -1301,53 +1308,51 @@ class GerritConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
                      (b_len, GERRIT_HUMAN_MESSAGE_LIMIT))
             message = ("%s... (truncated)" %
                        message[:GERRIT_HUMAN_MESSAGE_LIMIT - 20])
-        data = dict(message=message,
-                    strict_labels=False)
-        if change.is_current_patchset:
-            if labels:
-                data['labels'] = labels
-            if file_comments:
-                if self.version >= (2, 15, 0):
-                    file_comments = copy.deepcopy(file_comments)
-                    url = item.formatStatusUrl()
-                    for comments in itertools.chain(file_comments.values()):
-                        for comment in comments:
-                            comment['robot_id'] = 'zuul'
-                            comment['robot_run_id'] = \
-                                item.current_build_set.uuid
-                            if url:
-                                comment['url'] = url
-                    data['robot_comments'] = file_comments
-                else:
-                    data['comments'] = file_comments
-        if self.version >= (2, 13, 0):
-            data['tag'] = 'autogenerated:zuul:%s' % (item.pipeline.name)
-        changeid = "%s~%s~%s" % (
-            urllib.parse.quote(str(change.project), safe=''),
-            urllib.parse.quote(str(change.branch), safe=''),
-            change.id)
-        if checks_api:
-            self.report_checks(log, item, changeid, checks_api)
-        if (message or data.get('labels') or data.get('comments')
-            or data.get('robot_comments')):
-            for x in range(1, 4):
-                try:
-                    self.post('changes/%s/revisions/%s/review' %
-                              (changeid, change.commit),
-                              data)
-                    break
-                except HTTPConflictException:
-                    log.exception("Conflict submitting data to gerrit.")
-                    break
-                except HTTPBadRequestException:
-                    log.exception(
-                        "Bad request submitting check data to gerrit.")
-                    break
-                except Exception:
-                    log.exception(
-                        "Error submitting data to gerrit, attempt %s", x)
-                    time.sleep(x * 10)
-        if change.is_current_patchset and submit:
+        data = dict(strict_labels=False)
+        if phase1:
+            data['message'] = message
+            if change.is_current_patchset:
+                if labels:
+                    data['labels'] = labels
+                if file_comments:
+                    if self.version >= (2, 15, 0):
+                        file_comments = copy.deepcopy(file_comments)
+                        url = item.formatStatusUrl()
+                        for comments in itertools.chain(
+                                file_comments.values()):
+                            for comment in comments:
+                                comment['robot_id'] = 'zuul'
+                                comment['robot_run_id'] = \
+                                    item.current_build_set.uuid
+                                if url:
+                                    comment['url'] = url
+                        data['robot_comments'] = file_comments
+                    else:
+                        data['comments'] = file_comments
+            if self.version >= (2, 13, 0):
+                data['tag'] = 'autogenerated:zuul:%s' % (item.pipeline.name)
+            if checks_api:
+                self.report_checks(log, item, changeid, checks_api)
+            if (message or data.get('labels') or data.get('comments')
+                or data.get('robot_comments')):
+                for x in range(1, 4):
+                    try:
+                        self.post('changes/%s/revisions/%s/review' %
+                                  (changeid, change.commit),
+                                  data)
+                        break
+                    except HTTPConflictException:
+                        log.exception("Conflict submitting data to gerrit.")
+                        break
+                    except HTTPBadRequestException:
+                        log.exception(
+                            "Bad request submitting check data to gerrit.")
+                        break
+                    except Exception:
+                        log.exception(
+                            "Error submitting data to gerrit, attempt %s", x)
+                        time.sleep(x * 10)
+        if phase2 and change.is_current_patchset and submit:
             for x in range(1, 4):
                 try:
                     self.post('changes/%s/submit' % (changeid,), {})
diff --git a/zuul/driver/gerrit/gerritreporter.py b/zuul/driver/gerrit/gerritreporter.py
index fbee0e0ec..984b8742a 100644
--- a/zuul/driver/gerrit/gerritreporter.py
+++ b/zuul/driver/gerrit/gerritreporter.py
@@ -35,7 +35,7 @@ class GerritReporter(BaseReporter):
         self._checks_api = action.pop('checks-api', None)
         self._labels = action
 
-    def report(self, item):
+    def report(self, item, phase1=True, phase2=True):
         """Send a message to gerrit."""
         log = get_annotated_logger(self.log, item.event)
 
@@ -70,7 +70,8 @@ class GerritReporter(BaseReporter):
 
         return self.connection.review(item, message, self._submit,
                                       self._labels, self._checks_api,
-                                      comments, zuul_event_id=item.event)
+                                      comments, phase1, phase2,
+                                      zuul_event_id=item.event)
 
     def getSubmitAllowNeeds(self):
         """Get a list of code review labels that are allowed to be
diff --git a/zuul/driver/github/githubreporter.py b/zuul/driver/github/githubreporter.py
index c5297a23a..de62f2565 100644
--- a/zuul/driver/github/githubreporter.py
+++ b/zuul/driver/github/githubreporter.py
@@ -55,7 +55,7 @@ class GithubReporter(BaseReporter):
             self._unlabels = [self._unlabels]
         self.context = "{}/{}".format(pipeline.tenant.name, pipeline.name)
 
-    def report(self, item):
+    def report(self, item, phase1=True, phase2=True):
         """Report on an event."""
         # If the source is not GithubSource we cannot report anything here.
         if not isinstance(item.change.project.source, GithubSource):
@@ -69,7 +69,7 @@ class GithubReporter(BaseReporter):
 
         # order is important for github branch protection.
         # A status should be set before a merge attempt
-        if self._commit_status is not None:
+        if phase1 and self._commit_status is not None:
             if (hasattr(item.change, 'patchset') and
                     item.change.patchset is not None):
                 self.setCommitStatus(item)
@@ -80,22 +80,24 @@ class GithubReporter(BaseReporter):
         # If the change is not a pull request (e.g. a push) skip them.
         if hasattr(item.change, 'number'):
             errors_received = False
-            if self._labels or self._unlabels:
-                self.setLabels(item)
-            if self._review:
-                self.addReview(item)
-            if self._check:
-                check_errors = self.updateCheck(item)
-                # TODO (felix): We could use this mechanism to also report back
-                # errors from label and review actions
-                if check_errors:
-                    item.current_build_set.warning_messages.extend(
-                        check_errors
-                    )
-                    errors_received = True
-            if self._create_comment or errors_received:
-                self.addPullComment(item)
-            if (self._merge):
+            if phase1:
+                if self._labels or self._unlabels:
+                    self.setLabels(item)
+                if self._review:
+                    self.addReview(item)
+                if self._check:
+                    check_errors = self.updateCheck(item)
+                    # TODO (felix): We could use this mechanism to
+                    # also report back errors from label and review
+                    # actions
+                    if check_errors:
+                        item.current_build_set.warning_messages.extend(
+                            check_errors
+                        )
+                        errors_received = True
+                if self._create_comment or errors_received:
+                    self.addPullComment(item)
+            if phase2 and self._merge:
                 try:
                     self.mergePull(item)
                 except Exception as e:
diff --git a/zuul/driver/gitlab/gitlabreporter.py b/zuul/driver/gitlab/gitlabreporter.py
index f364521f5..819c89c47 100644
--- a/zuul/driver/gitlab/gitlabreporter.py
+++ b/zuul/driver/gitlab/gitlabreporter.py
@@ -50,7 +50,7 @@ class GitlabReporter(BaseReporter):
         if not isinstance(self._unlabels, list):
             self._unlabels = [self._unlabels]
 
-    def report(self, item):
+    def report(self, item, phase1=True, phase2=True):
         """Report on an event."""
         if not isinstance(item.change.project.source, GitlabSource):
             return
@@ -60,13 +60,14 @@ class GitlabReporter(BaseReporter):
             return
 
         if hasattr(item.change, 'number'):
-            if self._create_comment:
-                self.addMRComment(item)
-            if self._approval is not None:
-                self.setApproval(item)
-            if self._labels or self._unlabels:
-                self.setLabels(item)
-            if self._merge:
+            if phase1:
+                if self._create_comment:
+                    self.addMRComment(item)
+                if self._approval is not None:
+                    self.setApproval(item)
+                if self._labels or self._unlabels:
+                    self.setLabels(item)
+            if phase2 and self._merge:
                 self.mergeMR(item)
                 if not item.change.is_merged:
                     msg = self._formatItemReportMergeConflict(item)
diff --git a/zuul/driver/mqtt/mqttreporter.py b/zuul/driver/mqtt/mqttreporter.py
index a53389337..4090bb082 100644
--- a/zuul/driver/mqtt/mqttreporter.py
+++ b/zuul/driver/mqtt/mqttreporter.py
@@ -27,7 +27,9 @@ class MQTTReporter(BaseReporter):
     name = 'mqtt'
     log = logging.getLogger("zuul.MQTTReporter")
 
-    def report(self, item):
+    def report(self, item, phase1=True, phase2=True):
+        if not phase1:
+            return
         log = get_annotated_logger(self.log, item.event)
         log.debug("Report change %s, params %s", item.change, self.config)
         message = {
diff --git a/zuul/driver/pagure/pagurereporter.py b/zuul/driver/pagure/pagurereporter.py
index 918a31b6b..0bfdbc9b8 100644
--- a/zuul/driver/pagure/pagurereporter.py
+++ b/zuul/driver/pagure/pagurereporter.py
@@ -34,7 +34,7 @@ class PagureReporter(BaseReporter):
         self._merge = self.config.get('merge', False)
         self.context = "{}/{}".format(pipeline.tenant.name, pipeline.name)
 
-    def report(self, item):
+    def report(self, item, phase1=True, phase2=True):
         """Report on an event."""
 
         # If the source is not PagureSource we cannot report anything here.
@@ -47,17 +47,18 @@ class PagureReporter(BaseReporter):
                 self.connection.canonical_hostname:
             return
 
-        if self._commit_status is not None:
-            if (hasattr(item.change, 'patchset') and
-                    item.change.patchset is not None):
-                self.setCommitStatus(item)
-            elif (hasattr(item.change, 'newrev') and
-                    item.change.newrev is not None):
-                self.setCommitStatus(item)
-        if hasattr(item.change, 'number'):
-            if self._create_comment:
-                self.addPullComment(item)
-        if self._merge:
+        if phase1:
+            if self._commit_status is not None:
+                if (hasattr(item.change, 'patchset') and
+                        item.change.patchset is not None):
+                    self.setCommitStatus(item)
+                elif (hasattr(item.change, 'newrev') and
+                        item.change.newrev is not None):
+                    self.setCommitStatus(item)
+            if hasattr(item.change, 'number'):
+                if self._create_comment:
+                    self.addPullComment(item)
+        if phase2 and self._merge:
             self.mergePull(item)
             if not item.change.is_merged:
                 msg = self._formatItemReportMergeConflict(item)
diff --git a/zuul/driver/smtp/smtpreporter.py b/zuul/driver/smtp/smtpreporter.py
index 58bd25cef..4815026ab 100644
--- a/zuul/driver/smtp/smtpreporter.py
+++ b/zuul/driver/smtp/smtpreporter.py
@@ -25,8 +25,10 @@ class SMTPReporter(BaseReporter):
     name = 'smtp'
     log = logging.getLogger("zuul.SMTPReporter")
 
-    def report(self, item):
+    def report(self, item, phase1=True, phase2=True):
         """Send the compiled report message via smtp."""
+        if not phase1:
+            return
         log = get_annotated_logger(self.log, item.event)
         message = self._formatItemReport(item)