Merge "Support submitWholeTopic in Gerrit"
This commit is contained in:
commit
30e6acb0fb
|
@ -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
|
||||
|
|
|
@ -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
|
||||
------------------------
|
||||
|
||||
|
|
|
@ -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.
|
|
@ -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()]
|
||||
|
|
|
@ -20,6 +20,7 @@ driver=gerrit
|
|||
server=review.example.com
|
||||
user=jenkins
|
||||
sshkey=fake_id_rsa_path
|
||||
password=badpassword
|
||||
|
||||
[connection github]
|
||||
driver=github
|
||||
|
|
|
@ -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"
|
||||
|
|
|
@ -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,
|
||||
|
|
|
@ -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(
|
||||
|
|
Loading…
Reference in New Issue