Merge "Support submitWholeTopic in Gerrit"

8 files changed, 178 insertions, 18 deletions

diff --git a/doc/source/config/queue.rst b/doc/source/config/queue.rst
index bc24bb4fb..c18bf8939 100644
--- a/doc/source/config/queue.rst
+++ b/doc/source/config/queue.rst
@@ -46,30 +46,31 @@ Here is an example ``queue`` configuration.
    .. attr:: allow-circular-dependencies
       :default: false
 
-      Define if Zuul is allowed to process circular dependencies between
-      changes for this queue. All projects that are part of a dependency cycle
-      must share the same change queue.
+      Determines whether Zuul is allowed to process circular
+      dependencies between changes for this queue. All projects that
+      are part of a dependency cycle must share the same change queue.
 
-      In case Zuul detects a dependency cycle it will make sure that every
-      change also includes all other changes that are part of the cycle.
-      However each change will still be a normal item in the queue with its own
-      jobs.
+      If Zuul detects a dependency cycle it will ensure that every
+      change also includes all other changes that are part of the
+      cycle.  However each change will still be a normal item in the
+      queue with its own jobs.
 
       Reporting of success will be postponed until all items in the cycle
-      succeeded. In case of a failure in any of those items the whole cycle
+      succeed. In the case of a failure in any of those items the whole cycle
       will be dequeued.
 
-      An error message will be posted to all items of the cycle in case some
+      An error message will be posted to all items of the cycle if some
       items fail to report (e.g. merge failure when some items were already
       merged). In this case the target branch(es) might be in a broken state.
 
       In general, circular dependencies are considered to be an
       antipattern since they add extra constraints to continuous
       deployment systems.  Additionally, due to the lack of atomicity
-      in merge operations in code review systems, it may be possible
-      for only part of a cycle to be merged.  In that case, manual
-      interventions (such as reverting a commit, or bypassing gating to
-      force-merge the remaining commits) may be required.
+      in merge operations in code review systems (this includes
+      Gerrit, even with submitWholeTopic set), it may be possible for
+      only part of a cycle to be merged.  In that case, manual
+      interventions (such as reverting a commit, or bypassing gating
+      to force-merge the remaining commits) may be required.
 
       .. warning:: If the remote system is able to merge the first but
                    unable to merge the second or later change in a
diff --git a/doc/source/drivers/gerrit.rst b/doc/source/drivers/gerrit.rst
index 48908b74b..5cb1f5d11 100644
--- a/doc/source/drivers/gerrit.rst
+++ b/doc/source/drivers/gerrit.rst
@@ -25,6 +25,13 @@ want Zuul to report on.  For instance, you may want to grant
 or values may be added to Gerrit.  Zuul is very flexible and can take
 advantage of those.
 
+If ``change.submitWholeTopic`` is configured in Gerrit, Zuul will
+honor this by enqueing changes with the same topic as circular
+dependencies.  However, it is still necessary to enable circular
+dependency support in any pipeline queues where such changes may
+appear.  See :attr:`queue.allow-circular-dependencies` for information
+on how to configure this.
+
 Connection Configuration
 ------------------------
 
diff --git a/releasenotes/notes/gerrit-submit-whole-topic-5fef75db6ca450e8.yaml b/releasenotes/notes/gerrit-submit-whole-topic-5fef75db6ca450e8.yaml
new file mode 100644
index 000000000..c78d1bef8
--- /dev/null
+++ b/releasenotes/notes/gerrit-submit-whole-topic-5fef75db6ca450e8.yaml
@@ -0,0 +1,20 @@
+---
+features:
+  - |
+    Zuul now matches Gerrit's behavior when
+    ``change.submitWholeTopic`` is set in Gerrit.
+
+    When this setting is enabled in Gerrit and a change is submitted
+    (merged), all changes with the same topic are submitted
+    simultaneously.  Zuul will now query for changes which are set to
+    be submitted together by Gerrit when enqueing them and treat them
+    as if they are a set of circular dependencies.
+
+    If the projects are not part of pipeline queues which are
+    configured to allow circular dependencies, then Zuul will report
+    failure on enqueue.  Be sure that the submitWholeTopic setting in
+    Gerrit and the allow-circular-dependencies setting in Zuul match.
+
+    This functionality requires an HTTP connection to Gerrit.  If only
+    an SSH connection is available, then changes submitted together
+    will be ignored.
diff --git a/tests/base.py b/tests/base.py
index d0db346c9..51149adbd 100644
--- a/tests/base.py
+++ b/tests/base.py
@@ -383,7 +383,8 @@ class FakeGerritChange(object):
 
     def __init__(self, gerrit, number, project, branch, subject,
                  status='NEW', upstream_root=None, files={},
-                 parent=None, merge_parents=None, merge_files=None):
+                 parent=None, merge_parents=None, merge_files=None,
+                 topic=None):
         self.gerrit = gerrit
         self.source = gerrit
         self.reported = 0
@@ -421,6 +422,8 @@ class FakeGerritChange(object):
             'submitRecords': [],
             'url': '%s/%s' % (self.gerrit.baseurl.rstrip('/'), number)}
 
+        if topic:
+            self.data['topic'] = topic
         self.upstream_root = upstream_root
         if merge_parents:
             self.addMergePatchset(parents=merge_parents,
@@ -957,6 +960,7 @@ class GerritWebServer(object):
         class Server(http.server.SimpleHTTPRequestHandler):
             log = logging.getLogger("zuul.test.FakeGerritConnection")
             review_re = re.compile('/a/changes/(.*?)/revisions/(.*?)/review')
+            together_re = re.compile('/a/changes/(.*?)/submitted_together')
             submit_re = re.compile('/a/changes/(.*?)/submit')
             pending_checks_re = re.compile(
                 r'/a/plugins/checks/checks\.pending/\?'
@@ -1005,6 +1009,9 @@ class GerritWebServer(object):
                 m = self.files_re.match(path)
                 if m:
                     return self.get_files(m.group(1), m.group(2))
+                m = self.together_re.match(path)
+                if m:
+                    return self.get_submitted_together(m.group(1))
                 m = self.change_search_re.match(path)
                 if m:
                     return self.get_changes(m.group(1))
@@ -1132,6 +1139,21 @@ class GerritWebServer(object):
                 self.send_data(data)
                 self.end_headers()
 
+            def get_submitted_together(self, number):
+                change = fake_gerrit.changes.get(int(number))
+                if not change:
+                    return self._404()
+                topic = change.data.get('topic')
+                if not fake_gerrit._fake_submit_whole_topic:
+                    topic = None
+                if topic:
+                    results = fake_gerrit._simpleQuery(
+                        f'topic:{topic}', http=True)
+                else:
+                    results = []
+                self.send_data(results)
+                self.end_headers()
+
             def get_changes(self, query):
                 self.log.debug("simpleQueryHTTP: %s", query)
                 query = urllib.parse.unquote(query)
@@ -1262,6 +1284,7 @@ class FakeGerritConnection(gerritconnection.GerritConnection):
         self.fake_checkers = []
         self._poller_event = poller_event
         self._ref_watcher_event = ref_watcher_event
+        self._fake_submit_whole_topic = False
 
     def onStop(self):
         super().onStop()
@@ -1273,14 +1296,15 @@ class FakeGerritConnection(gerritconnection.GerritConnection):
 
     def addFakeChange(self, project, branch, subject, status='NEW',
                       files=None, parent=None, merge_parents=None,
-                      merge_files=None):
+                      merge_files=None, topic=None):
         """Add a change to the fake Gerrit."""
         self.change_number += 1
         c = FakeGerritChange(self, self.change_number, project, branch,
                              subject, upstream_root=self.upstream_root,
                              status=status, files=files, parent=parent,
                              merge_parents=merge_parents,
-                             merge_files=merge_files)
+                             merge_files=merge_files,
+                             topic=topic)
         self.changes[self.change_number] = c
         return c
 
@@ -1431,6 +1455,10 @@ class FakeGerritConnection(gerritconnection.GerritConnection):
                 queryMethod(change) for change in self.changes.values()
                 if change.data["lastUpdated"] >= cut_off_time
             ]
+        elif query.startswith('topic:'):
+            topic = query[len('topic:'):].strip()
+            l = [queryMethod(change) for change in self.changes.values()
+                 if topic in change.data.get('topic', '')]
         else:
             # Query all open changes
             l = [queryMethod(change) for change in self.changes.values()]
diff --git a/tests/fixtures/zuul-gerrit-github.conf b/tests/fixtures/zuul-gerrit-github.conf
index 1b2813dd4..0d03bacc2 100644
--- a/tests/fixtures/zuul-gerrit-github.conf
+++ b/tests/fixtures/zuul-gerrit-github.conf
@@ -20,6 +20,7 @@ driver=gerrit
 server=review.example.com
 user=jenkins
 sshkey=fake_id_rsa_path
+password=badpassword
 
 [connection github]
 driver=github
diff --git a/tests/unit/test_circular_dependencies.py b/tests/unit/test_circular_dependencies.py
index 0b48a66a9..713408ee9 100644
--- a/tests/unit/test_circular_dependencies.py
+++ b/tests/unit/test_circular_dependencies.py
@@ -1410,6 +1410,45 @@ class TestGerritCircularDependencies(ZuulTestCase):
         self.assertEqual(B.data['status'], 'NEW')
         self.assertEqual(C.data['status'], 'MERGED')
 
+    def test_submitted_together(self):
+        self.fake_gerrit._fake_submit_whole_topic = True
+        A = self.fake_gerrit.addFakeChange('org/project1', "master", "A",
+                                           topic='test-topic')
+        B = self.fake_gerrit.addFakeChange('org/project2', "master", "B",
+                                           topic='test-topic')
+
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+
+        self.assertEqual(len(A.patchsets[-1]["approvals"]), 1)
+        self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified")
+        self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "1")
+
+        self.assertEqual(len(B.patchsets[-1]["approvals"]), 1)
+        self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified")
+        self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "1")
+
+        # We're about to add approvals to changes without adding the
+        # triggering events to Zuul, so that we can be sure that it is
+        # enqueing the changes based on dependencies, not because of
+        # triggering events.  Since it will have the changes cached
+        # already (without approvals), we need to clear the cache
+        # first.
+        for connection in self.scheds.first.connections.connections.values():
+            connection.maintainCache([], max_age=0)
+
+        A.addApproval("Code-Review", 2)
+        B.addApproval("Code-Review", 2)
+        A.addApproval("Approved", 1)
+        self.fake_gerrit.addEvent(B.addApproval("Approved", 1))
+        self.waitUntilSettled()
+
+        self.assertEqual(A.reported, 3)
+        self.assertEqual(B.reported, 3)
+        self.assertEqual(A.data["status"], "MERGED")
+        self.assertEqual(B.data["status"], "MERGED")
+
 
 class TestGithubCircularDependencies(ZuulTestCase):
     config_file = "zuul-gerrit-github.conf"
diff --git a/tests/unit/test_web.py b/tests/unit/test_web.py
index 7ce82de51..98bbd5f77 100644
--- a/tests/unit/test_web.py
+++ b/tests/unit/test_web.py
@@ -2667,10 +2667,12 @@ class TestWebMulti(BaseTestWeb):
 
     def test_web_connections_list_multi(self):
         data = self.get_url('api/connections').json()
+        port = self.web.connections.connections['gerrit'].web_server.port
+        url = f'http://localhost:{port}'
         gerrit_connection = {
             'driver': 'gerrit',
             'name': 'gerrit',
-            'baseurl': 'https://review.example.com',
+            'baseurl': url,
             'canonical_hostname': 'review.example.com',
             'server': 'review.example.com',
             'port': 29418,
diff --git a/zuul/driver/gerrit/gerritconnection.py b/zuul/driver/gerrit/gerritconnection.py
index 704ca2d2b..66d807966 100644
--- a/zuul/driver/gerrit/gerritconnection.py
+++ b/zuul/driver/gerrit/gerritconnection.py
@@ -881,6 +881,32 @@ class GerritConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
                 records.append(result)
         return [(x.number, x.current_patchset) for x in records]
 
+    def _getSubmittedTogether(self, change, event):
+        if not self.session:
+            return []
+        # We could probably ask for everything in one query, but it
+        # might be extremely large, so just get the change ids here
+        # and then query the individual changes.
+        log = get_annotated_logger(self.log, event)
+        log.debug("Updating %s: Looking for changes submitted together",
+                  change)
+        ret = []
+        try:
+            data = self.get(f'changes/{change.number}/submitted_together')
+        except Exception:
+            log.error("Unable to find changes submitted together for %s",
+                      change)
+            return ret
+        for c in data:
+            dep_change = c['_number']
+            dep_ps = c['revisions'][c['current_revision']]['_number']
+            if str(dep_change) == str(change.number):
+                continue
+            log.debug("Updating %s: Found change %s,%s submitted together",
+                      change, dep_change, dep_ps)
+            ret.append((dep_change, dep_ps))
+        return ret
+
     def _updateChange(self, key, change, event, history):
         log = get_annotated_logger(self.log, event)
 
@@ -999,6 +1025,41 @@ class GerritConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
                 log.exception("Failed to get commit-needed change %s,%s",
                               dep_num, dep_ps)
 
+        for (dep_num, dep_ps) in self._getSubmittedTogether(change, event):
+            try:
+                log.debug("Updating %s: Getting submitted-together "
+                          "change %s,%s",
+                          change, dep_num, dep_ps)
+                # Because a submitted-together change may be a cross-repo
+                # dependency, cause that change to refresh so that it will
+                # reference the latest patchset of its Depends-On (this
+                # change). In case the dep is already in history we already
+                # refreshed this change so refresh is not needed in this case.
+                refresh = (dep_num, dep_ps) not in history
+                dep_key = ChangeKey(self.connection_name, None,
+                                    'GerritChange', str(dep_num), str(dep_ps))
+                dep = self._getChange(
+                    dep_key, refresh=refresh, history=history,
+                    event=event)
+                # Gerrit changes to be submitted together do not
+                # necessarily get posted with dependency cycles using
+                # git trees and depends-on. However, they are
+                # functionally equivalent to a stack of changes with
+                # cycles using those methods. Here we set
+                # needs_changes and needed_by_changes as if there were
+                # a cycle. This ensures Zuul's cycle handling manages
+                # the submitted together changes properly.
+                if dep.open and dep not in needs_changes:
+                    compat_needs_changes.append(dep.cache_key)
+                    needs_changes.add(dep.cache_key)
+                if (dep.open and dep.is_current_patchset
+                    and dep not in needed_by_changes):
+                    compat_needed_by_changes.append(dep.cache_key)
+                    needed_by_changes.add(dep.cache_key)
+            except Exception:
+                log.exception("Failed to get commit-needed change %s,%s",
+                              dep_num, dep_ps)
+
         self.updateChangeAttributes(
             change,
             git_needs_changes=git_needs_changes,
@@ -1292,7 +1353,8 @@ class GerritConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
                     self.post('changes/%s/submit' % (changeid,), {})
                     break
                 except HTTPConflictException:
-                    log.exception("Conflict submitting data to gerrit.")
+                    log.info("Conflict submitting data to gerrit, "
+                             "change may already be merged")
                     break
                 except HTTPBadRequestException:
                     log.exception(