zuul
/
zuul
Code Issues Proposed changes

Case sensitive label matching 

After upgrading Gerrit to 2.13 our gate stopped working. The reason
for this is that after a successful gate run zuul does something like
'gerrit review --label verified=2 --submit'. The verified label in
Gerrit by default is configured as 'Verified'. The newer version of
gerrit behaves different now. It accepts the +2 vote on verified but
doesn't submit the patch anymore if the casing is not correct. This
forces us to specify the label in the same casing as gerrit
expects. In that case the tolower() in canMerge prevents the patch
from entering the gate.

In order to avoid confusion and be consistent, avoid any case
conversions and use the labels exactly as defined in Gerrit.

Note that this patch requires changes to the pipelines such that the
labels are spelled exactly as defined in Gerrit.

This is a backport of I9713a075e07b268e4f2620c0862c128158283c7c to
master.

Change-Id: I55e6f12969c1c920a5017382523e71e12bc7ac3c
This commit is contained in:
Tobias Henkel 2017-07-12 10:51:43 +02:00 committed by Clark Boylan
parent c2def90f1e
commit 01740bd147
16 changed files with 105 additions and 84 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
NEWS.rst
View File

@ -12,6 +12,11 @@ Since 2.0.0:
the Zuul server in smaller deployments. Several configuration the Zuul server in smaller deployments. Several configuration
options have moved from the ``zuul`` section to ``merger``. options have moved from the ``zuul`` section to ``merger``.
* Gerrit label names must now be listed in your layout.yaml exactly as
they appear in Gerrit. This means case and special characters must
match. This change was made to accomodate Gerrit 2.13 which needs the
strings to match for changes to be successfully submitted.
Since 1.3.0: Since 1.3.0:
* The Jenkins launcher is replaced with Gearman launcher. An internal * The Jenkins launcher is replaced with Gearman launcher. An internal

26
tests/base.py
View File

@ -103,9 +103,22 @@ class ChangeReference(git.Reference):
class FakeChange(object): class FakeChange(object):
categories = {'APRV': ('Approved', -1, 1), categories = {'Approved': ('Approved', -1, 1),
'CRVW': ('Code-Review', -2, 2), 'Code-Review': ('Code-Review', -2, 2),
'VRFY': ('Verified', -2, 2)} 'Verified': ('Verified', -2, 2)}
# TODO(tobiash): This is used as a translation layer between the tests
# which use lower case labels. This can be removed if all
# tests are converted to use the correct casing.
categories_translation = {'approved': 'Approved',
'code-review': 'Code-Review',
'verified': 'Verified',
'Approved': 'Approved',
'Code-Review': 'Code-Review',
'Verified': 'Verified',
'CRVW': 'Code-Review',
'APRV': 'Approved',
'VRFY': 'Verified'}
def __init__(self, gerrit, number, project, branch, subject, def __init__(self, gerrit, number, project, branch, subject,
status='NEW', upstream_root=None): status='NEW', upstream_root=None):
@ -290,8 +303,8 @@ class FakeChange(object):
if not granted_on: if not granted_on:
granted_on = time.time() granted_on = time.time()
approval = { approval = {
'description': self.categories[category][0], 'description': self.categories_translation[category],
'type': category, 'type': self.categories_translation[category],
'value': str(value), 'value': str(value),
'by': { 'by': {
'username': username, 'username': username,
@ -300,7 +313,8 @@ class FakeChange(object):
'grantedOn': int(granted_on) 'grantedOn': int(granted_on)
} }
for i, x in enumerate(self.patchsets[-1]['approvals'][:]): for i, x in enumerate(self.patchsets[-1]['approvals'][:]):
if x['by']['username'] == username and x['type'] == category: if x['by']['username'] == username and \
x['type'] == self.categories_translation[category]:
del self.patchsets[-1]['approvals'][i] del self.patchsets[-1]['approvals'][i]
self.patchsets[-1]['approvals'].append(approval) self.patchsets[-1]['approvals'].append(approval)
event = {'approvals': [approval], event = {'approvals': [approval],

12
tests/fixtures/layout-cloner.yaml vendored
View File

@ -6,10 +6,10 @@ pipelines:
- event: patchset-created - event: patchset-created
success: success:
gerrit: gerrit:
verified: 1 Verified: 1
failure: failure:
gerrit: gerrit:
verified: -1 Verified: -1
- name: gate - name: gate
manager: DependentPipelineManager manager: DependentPipelineManager
@ -18,17 +18,17 @@ pipelines:
gerrit: gerrit:
- event: comment-added - event: comment-added
approval: approval:
- approved: 1 - Approved: 1
start: start:
gerrit: gerrit:
verified: 0 Verified: 0
success: success:
gerrit: gerrit:
verified: 2 Verified: 2
submit: true submit: true
failure: failure:
gerrit: gerrit:
verified: -2 Verified: -2
- name: post - name: post
manager: IndependentPipelineManager manager: IndependentPipelineManager

12
tests/fixtures/layout-delayed-repo-init.yaml vendored
View File

@ -6,10 +6,10 @@ pipelines:
- event: patchset-created - event: patchset-created
success: success:
gerrit: gerrit:
verified: 1 Verified: 1
failure: failure:
gerrit: gerrit:
verified: -1 Verified: -1
- name: post - name: post
manager: IndependentPipelineManager manager: IndependentPipelineManager
@ -25,17 +25,17 @@ pipelines:
gerrit: gerrit:
- event: comment-added - event: comment-added
approval: approval:
- approved: 1 - Approved: 1
success: success:
gerrit: gerrit:
verified: 2 Verified: 2
submit: true submit: true
failure: failure:
gerrit: gerrit:
verified: -2 Verified: -2
start: start:
gerrit: gerrit:
verified: 0 Verified: 0
precedence: high precedence: high
projects: projects:

8
tests/fixtures/layout-footer-message.yaml vendored
View File

@ -10,21 +10,21 @@ pipelines:
gerrit: gerrit:
- event: comment-added - event: comment-added
approval: approval:
- approved: 1 - Approved: 1
success: success:
gerrit: gerrit:
verified: 2 Verified: 2
submit: true submit: true
smtp: smtp:
to: success@example.org to: success@example.org
failure: failure:
gerrit: gerrit:
verified: -2 Verified: -2
smtp: smtp:
to: failure@example.org to: failure@example.org
start: start:
gerrit: gerrit:
verified: 0 Verified: 0
precedence: high precedence: high
projects: projects:

8
tests/fixtures/layout-live-reconfiguration-functions.yaml vendored
View File

@ -9,17 +9,17 @@ pipelines:
gerrit: gerrit:
- event: comment-added - event: comment-added
approval: approval:
- approved: 1 - Approved: 1
success: success:
gerrit: gerrit:
verified: 2 Verified: 2
submit: true submit: true
failure: failure:
gerrit: gerrit:
verified: -2 Verified: -2
start: start:
gerrit: gerrit:
verified: 0 Verified: 0
precedence: high precedence: high
jobs: jobs:

14
tests/fixtures/layout-merge-failure.yaml vendored
View File

@ -6,10 +6,10 @@ pipelines:
- event: patchset-created - event: patchset-created
success: success:
gerrit: gerrit:
verified: 1 Verified: 1
failure: failure:
gerrit: gerrit:
verified: -1 Verified: -1
- name: post - name: post
manager: IndependentPipelineManager manager: IndependentPipelineManager
@ -26,22 +26,22 @@ pipelines:
gerrit: gerrit:
- event: comment-added - event: comment-added
approval: approval:
- approved: 1 - Approved: 1
success: success:
gerrit: gerrit:
verified: 2 Verified: 2
submit: true submit: true
failure: failure:
gerrit: gerrit:
verified: -2 Verified: -2
merge-failure: merge-failure:
gerrit: gerrit:
verified: -1 Verified: -1
smtp: smtp:
to: you@example.com to: you@example.com
start: start:
gerrit: gerrit:
verified: 0 Verified: 0
precedence: high precedence: high
projects: projects:

6
tests/fixtures/layout-rate-limit.yaml vendored
View File

@ -6,13 +6,13 @@ pipelines:
gerrit: gerrit:
- event: comment-added - event: comment-added
approval: approval:
- approved: 1 - Approved: 1
start: start:
gerrit: gerrit:
verified: 0 Verified: 0
success: success:
gerrit: gerrit:
verified: 2 Verified: 2
submit: true submit: true
failure: failure:
gerrit: gerrit:

12
tests/fixtures/layout-repo-deleted.yaml vendored
View File

@ -6,10 +6,10 @@ pipelines:
- event: patchset-created - event: patchset-created
success: success:
gerrit: gerrit:
verified: 1 Verified: 1
failure: failure:
gerrit: gerrit:
verified: -1 Verified: -1
- name: post - name: post
manager: IndependentPipelineManager manager: IndependentPipelineManager
@ -25,17 +25,17 @@ pipelines:
gerrit: gerrit:
- event: comment-added - event: comment-added
approval: approval:
- approved: 1 - Approved: 1
success: success:
gerrit: gerrit:
verified: 2 Verified: 2
submit: true submit: true
failure: failure:
gerrit: gerrit:
verified: -2 Verified: -2
start: start:
gerrit: gerrit:
verified: 0 Verified: 0
precedence: high precedence: high
projects: projects:

12
tests/fixtures/layout-swift.yaml vendored
View File

@ -6,10 +6,10 @@ pipelines:
- event: patchset-created - event: patchset-created
success: success:
gerrit: gerrit:
verified: 1 Verified: 1
failure: failure:
gerrit: gerrit:
verified: -1 Verified: -1
- name: post - name: post
manager: IndependentPipelineManager manager: IndependentPipelineManager
@ -25,17 +25,17 @@ pipelines:
gerrit: gerrit:
- event: comment-added - event: comment-added
approval: approval:
- approved: 1 - Approved: 1
success: success:
gerrit: gerrit:
verified: 2 Verified: 2
submit: true submit: true
failure: failure:
gerrit: gerrit:
verified: -2 Verified: -2
start: start:
gerrit: gerrit:
verified: 0 Verified: 0
precedence: high precedence: high
jobs: jobs:

16
tests/fixtures/layout-zuultrigger-enqueued.yaml vendored
View File

@ -4,7 +4,7 @@ pipelines:
source: gerrit source: gerrit
require: require:
approval: approval:
- verified: -1 - Verified: -1
trigger: trigger:
gerrit: gerrit:
- event: patchset-created - event: patchset-created
@ -13,10 +13,10 @@ pipelines:
pipeline: gate pipeline: gate
success: success:
gerrit: gerrit:
verified: 1 Verified: 1
failure: failure:
gerrit: gerrit:
verified: -1 Verified: -1
- name: gate - name: gate
manager: DependentPipelineManager manager: DependentPipelineManager
@ -24,25 +24,25 @@ pipelines:
source: gerrit source: gerrit
require: require:
approval: approval:
- verified: 1 - Verified: 1
trigger: trigger:
gerrit: gerrit:
- event: comment-added - event: comment-added
approval: approval:
- approved: 1 - Approved: 1
zuul: zuul:
- event: parent-change-enqueued - event: parent-change-enqueued
pipeline: gate pipeline: gate
success: success:
gerrit: gerrit:
verified: 2 Verified: 2
submit: true submit: true
failure: failure:
gerrit: gerrit:
verified: -2 Verified: -2
start: start:
gerrit: gerrit:
verified: 0 Verified: 0
precedence: high precedence: high
projects: projects:

14
tests/fixtures/layout-zuultrigger-merged.yaml vendored
View File

@ -7,10 +7,10 @@ pipelines:
- event: patchset-created - event: patchset-created
success: success:
gerrit: gerrit:
verified: 1 Verified: 1
failure: failure:
gerrit: gerrit:
verified: -1 Verified: -1
- name: gate - name: gate
manager: DependentPipelineManager manager: DependentPipelineManager
@ -20,17 +20,17 @@ pipelines:
gerrit: gerrit:
- event: comment-added - event: comment-added
approval: approval:
- approved: 1 - Approved: 1
success: success:
gerrit: gerrit:
verified: 2 Verified: 2
submit: true submit: true
failure: failure:
gerrit: gerrit:
verified: -2 Verified: -2
start: start:
gerrit: gerrit:
verified: 0 Verified: 0
precedence: high precedence: high
- name: merge-check - name: merge-check
@ -42,7 +42,7 @@ pipelines:
- event: project-change-merged - event: project-change-merged
merge-failure: merge-failure:
gerrit: gerrit:
verified: -1 Verified: -1
projects: projects:
- name: org/project - name: org/project

30
tests/fixtures/layout.yaml vendored
View File

@ -9,10 +9,10 @@ pipelines:
- event: patchset-created - event: patchset-created
success: success:
gerrit: gerrit:
verified: 1 Verified: 1
failure: failure:
gerrit: gerrit:
verified: -1 Verified: -1
- name: post - name: post
manager: IndependentPipelineManager manager: IndependentPipelineManager
@ -28,17 +28,17 @@ pipelines:
gerrit: gerrit:
- event: comment-added - event: comment-added
approval: approval:
- approved: 1 - Approved: 1
success: success:
gerrit: gerrit:
verified: 2 Verified: 2
submit: true submit: true
failure: failure:
gerrit: gerrit:
verified: -2 Verified: -2
start: start:
gerrit: gerrit:
verified: 0 Verified: 0
precedence: high precedence: high
- name: unused - name: unused
@ -48,7 +48,7 @@ pipelines:
gerrit: gerrit:
- event: comment-added - event: comment-added
approval: approval:
- approved: 1 - Approved: 1
- name: dup1 - name: dup1
manager: IndependentPipelineManager manager: IndependentPipelineManager
@ -57,10 +57,10 @@ pipelines:
- event: change-restored - event: change-restored
success: success:
gerrit: gerrit:
verified: 1 Verified: 1
failure: failure:
gerrit: gerrit:
verified: -1 Verified: -1
- name: dup2 - name: dup2
manager: IndependentPipelineManager manager: IndependentPipelineManager
@ -69,10 +69,10 @@ pipelines:
- event: change-restored - event: change-restored
success: success:
gerrit: gerrit:
verified: 1 Verified: 1
failure: failure:
gerrit: gerrit:
verified: -1 Verified: -1
- name: conflict - name: conflict
manager: DependentPipelineManager manager: DependentPipelineManager
@ -81,17 +81,17 @@ pipelines:
gerrit: gerrit:
- event: comment-added - event: comment-added
approval: approval:
- approved: 1 - Approved: 1
success: success:
gerrit: gerrit:
verified: 2 Verified: 2
submit: true submit: true
failure: failure:
gerrit: gerrit:
verified: -2 Verified: -2
start: start:
gerrit: gerrit:
verified: 0 Verified: 0
- name: experimental - name: experimental
manager: IndependentPipelineManager manager: IndependentPipelineManager

4
tests/test_connection.py
View File

@ -60,7 +60,7 @@ class TestConnections(ZuulDBTestCase):
self.waitUntilSettled() self.waitUntilSettled()
self.assertEqual(len(A.patchsets[-1]['approvals']), 1) self.assertEqual(len(A.patchsets[-1]['approvals']), 1)
self.assertEqual(A.patchsets[-1]['approvals'][0]['type'], 'VRFY') self.assertEqual(A.patchsets[-1]['approvals'][0]['type'], 'Verified')
self.assertEqual(A.patchsets[-1]['approvals'][0]['value'], '1') self.assertEqual(A.patchsets[-1]['approvals'][0]['value'], '1')
self.assertEqual(A.patchsets[-1]['approvals'][0]['by']['username'], self.assertEqual(A.patchsets[-1]['approvals'][0]['by']['username'],
'jenkins') 'jenkins')
@ -72,7 +72,7 @@ class TestConnections(ZuulDBTestCase):
self.waitUntilSettled() self.waitUntilSettled()
self.assertEqual(len(B.patchsets[-1]['approvals']), 1) self.assertEqual(len(B.patchsets[-1]['approvals']), 1)
self.assertEqual(B.patchsets[-1]['approvals'][0]['type'], 'VRFY') self.assertEqual(B.patchsets[-1]['approvals'][0]['type'], 'Verified')
self.assertEqual(B.patchsets[-1]['approvals'][0]['value'], '-1') self.assertEqual(B.patchsets[-1]['approvals'][0]['value'], '-1')
self.assertEqual(B.patchsets[-1]['approvals'][0]['by']['username'], self.assertEqual(B.patchsets[-1]['approvals'][0]['by']['username'],
'civoter') 'civoter')

8
zuul/model.py
View File

@ -1123,7 +1123,8 @@ class BaseFilter(object):
else: else:
if not isinstance(v, list): if not isinstance(v, list):
v = [v] v = [v]
if (normalizeCategory(approval['description']) != k or if (normalizeCategory(approval['description']) !=
normalizeCategory(k) or
int(approval['value']) not in v): int(approval['value']) not in v):
return False return False
return True return True
@ -1303,8 +1304,9 @@ class EventFilter(BaseFilter):
for category, value in self.event_approvals.items(): for category, value in self.event_approvals.items():
matches_approval = False matches_approval = False
for eapproval in event.approvals: for eapproval in event.approvals:
if (normalizeCategory(eapproval['description']) == category and if (normalizeCategory(eapproval['description']) ==
int(eapproval['value']) == int(value)): normalizeCategory(category) and
int(eapproval['value']) == int(value)):
matches_approval = True matches_approval = True
if not matches_approval: if not matches_approval:
return False return False

2
zuul/source/gerrit.py
View File

@ -125,7 +125,7 @@ class GerritSource(BaseSource):
continue continue
elif label['status'] in ['NEED', 'REJECT']: elif label['status'] in ['NEED', 'REJECT']:
# It may be our own rejection, so we ignore # It may be our own rejection, so we ignore
if label['label'].lower() not in allow_needs: if label['label'] not in allow_needs:
return False return False
continue continue
else: else: