Fix implied branch matchers and override-checkout
When specifying job.override-checkout, we apply job variants from that match the specified branch. The mechanism we use to do that is to create a synthetic Ref object to pass to the branch matcher instead of the real branch of the Change (since the real branch is likely different -- that's why override-checkout was specified). However, branch matching behavior has gottes slightly more sophisticated and Ref objects don't match in the same way that Change objects do. In particular, implied branch matchers match Ref subclasses that have a branch attribute iff the match is exact. This means that if a user constructed two branches: * testbranch * testbranch2 and used override-checkout to favor a job definition from testbranch2, the implied branch matcher for the variant in testbranch would match since the matcher behaved as if it were matching a Ref not a Change or Branch. To correct this, we update the simulated change object used in the override-checkout variant matching routine to be a Branch (which unsurprisingly has a branch attribute) instead of a Ref. The test test_implied_branch_matcher_similar_override_checkout is added to cover this test case. Additionally, the test test_implied_branch_matcher_similar is added for good measure (it tests implied branch matchers in the same way but without specifying override-checkout), though its behavior is already correct. A release note is included since this may have an effect on job behavior. Change-Id: I1104eaf02f752e8a73e9b04939f03a4888763b27
This commit is contained in:
James E. Blair
parent
6a5a2d0f6f
commit
95097565e6
@ -0,0 +1,13 @@
|
||||
---
|
||||
fixes:
|
||||
- |
|
||||
The use of implied branch matchers in jobs where
|
||||
`override-checkout` was specified could cause some jobs to include
|
||||
unintended variants. Specifically: job variants with implied
|
||||
branch matchers on branches which are substring matches of a
|
||||
branch specified in the `override-checkout` job attribute may have
|
||||
been used when not intended.
|
||||
|
||||
This has been corrected so that the same job variant matching
|
||||
process happens whether the change's branch or the branch
|
||||
specified by override-checkout is used.
|
||||
@ -923,6 +923,159 @@ class TestBranchMismatch(ZuulTestCase):
|
||||
self.assertIn('nodeset "bar" was not found', A.messages[0],
|
||||
"A should have a syntax error reported")
|
||||
|
||||
def _updateConfig(self, config, branch):
|
||||
file_dict = {'zuul.yaml': config}
|
||||
C = self.fake_gerrit.addFakeChange('org/project1', branch, 'C',
|
||||
files=file_dict)
|
||||
C.setMerged()
|
||||
self.fake_gerrit.addEvent(C.getChangeMergedEvent())
|
||||
self.waitUntilSettled()
|
||||
|
||||
def test_implied_branch_matcher_similar(self):
|
||||
# Test that we perform a full-text match with implied branch
|
||||
# matchers.
|
||||
self.create_branch('org/project1', 'testbranch')
|
||||
self.create_branch('org/project1', 'testbranch2')
|
||||
self.fake_gerrit.addEvent(
|
||||
self.fake_gerrit.getFakeBranchCreatedEvent(
|
||||
'org/project1', 'testbranch'))
|
||||
self.fake_gerrit.addEvent(
|
||||
self.fake_gerrit.getFakeBranchCreatedEvent(
|
||||
'org/project1', 'testbranch2'))
|
||||
|
||||
config = textwrap.dedent(
|
||||
"""
|
||||
- job:
|
||||
name: testjob
|
||||
vars:
|
||||
this_branch: testbranch
|
||||
testbranch: true
|
||||
- project:
|
||||
check: {jobs: [testjob]}
|
||||
""")
|
||||
self._updateConfig(config, 'testbranch')
|
||||
config = textwrap.dedent(
|
||||
"""
|
||||
- job:
|
||||
name: testjob
|
||||
vars:
|
||||
this_branch: testbranch2
|
||||
testbranch2: true
|
||||
- project:
|
||||
check: {jobs: [testjob]}
|
||||
""")
|
||||
self._updateConfig(config, 'testbranch2')
|
||||
|
||||
self.executor_server.hold_jobs_in_build = True
|
||||
A = self.fake_gerrit.addFakeChange(
|
||||
'org/project1', 'testbranch', 'A')
|
||||
self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
|
||||
self.waitUntilSettled()
|
||||
|
||||
self.assertEqual(
|
||||
{'testbranch': True, 'this_branch': 'testbranch'},
|
||||
self.builds[0].job.combined_variables)
|
||||
|
||||
self.executor_server.release()
|
||||
self.waitUntilSettled()
|
||||
|
||||
B = self.fake_gerrit.addFakeChange(
|
||||
'org/project1', 'testbranch2', 'B')
|
||||
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
|
||||
self.waitUntilSettled()
|
||||
|
||||
# The two jobs should have distinct variables. Notably,
|
||||
# testbranch2 should not pick up vars from testbranch.
|
||||
self.assertEqual(
|
||||
{'testbranch2': True, 'this_branch': 'testbranch2'},
|
||||
self.builds[0].job.combined_variables)
|
||||
|
||||
self.executor_server.hold_jobs_in_build = False
|
||||
self.executor_server.release()
|
||||
self.waitUntilSettled()
|
||||
|
||||
self.assertHistory([
|
||||
dict(name='testjob', result='SUCCESS', changes='3,1'),
|
||||
dict(name='testjob', result='SUCCESS', changes='4,1'),
|
||||
], ordered=False)
|
||||
|
||||
def test_implied_branch_matcher_similar_override_checkout(self):
|
||||
# Overriding a checkout has some branch matching implications.
|
||||
# Make sure that we are performing a full-text match on
|
||||
# branches when we override a checkout.
|
||||
self.create_branch('org/project1', 'testbranch')
|
||||
self.create_branch('org/project1', 'testbranch2')
|
||||
self.fake_gerrit.addEvent(
|
||||
self.fake_gerrit.getFakeBranchCreatedEvent(
|
||||
'org/project1', 'testbranch'))
|
||||
self.fake_gerrit.addEvent(
|
||||
self.fake_gerrit.getFakeBranchCreatedEvent(
|
||||
'org/project1', 'testbranch2'))
|
||||
config = textwrap.dedent(
|
||||
"""
|
||||
- job:
|
||||
name: testjob
|
||||
vars:
|
||||
this_branch: testbranch
|
||||
testbranch: true
|
||||
- project:
|
||||
check: {jobs: [testjob]}
|
||||
""")
|
||||
self._updateConfig(config, 'testbranch')
|
||||
config = textwrap.dedent(
|
||||
"""
|
||||
- job:
|
||||
name: testjob
|
||||
vars:
|
||||
this_branch: testbranch2
|
||||
testbranch2: true
|
||||
- project:
|
||||
check: {jobs: [testjob]}
|
||||
""")
|
||||
self._updateConfig(config, 'testbranch2')
|
||||
|
||||
self.executor_server.hold_jobs_in_build = True
|
||||
config = textwrap.dedent(
|
||||
"""
|
||||
- job:
|
||||
name: project-test1
|
||||
- job:
|
||||
name: testjob-testbranch
|
||||
parent: testjob
|
||||
override-checkout: testbranch
|
||||
- job:
|
||||
name: testjob-testbranch2
|
||||
parent: testjob
|
||||
override-checkout: testbranch2
|
||||
- project:
|
||||
check: {jobs: [testjob-testbranch, testjob-testbranch2]}
|
||||
""")
|
||||
file_dict = {'zuul.yaml': config}
|
||||
A = self.fake_gerrit.addFakeChange(
|
||||
'org/project1', 'master', 'A', files=file_dict)
|
||||
self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
|
||||
self.waitUntilSettled()
|
||||
|
||||
self.assertEqual(
|
||||
{'testbranch': True, 'this_branch': 'testbranch'},
|
||||
self.builds[0].job.combined_variables)
|
||||
|
||||
# The two jobs should have distinct variables (notably, the
|
||||
# variant on testbranch2 should not pick up vars from
|
||||
# testbranch.
|
||||
self.assertEqual(
|
||||
{'testbranch2': True, 'this_branch': 'testbranch2'},
|
||||
self.builds[1].job.combined_variables)
|
||||
|
||||
self.executor_server.hold_jobs_in_build = False
|
||||
self.executor_server.release()
|
||||
self.waitUntilSettled()
|
||||
|
||||
self.assertHistory([
|
||||
dict(name='testjob-testbranch', result='SUCCESS', changes='3,1'),
|
||||
dict(name='testjob-testbranch2', result='SUCCESS', changes='3,1'),
|
||||
], ordered=False)
|
||||
|
||||
|
||||
class TestBranchRef(ZuulTestCase):
|
||||
tenant_config_file = 'config/branch-ref/main.yaml'
|
||||
|
||||
@ -3135,10 +3135,9 @@ class Job(ConfigObject):
|
||||
branch_change = change
|
||||
else:
|
||||
# If an override branch is supplied, create a very basic
|
||||
# change (a Ref) and set its branch to the override
|
||||
# branch.
|
||||
branch_change = Ref(change.project)
|
||||
branch_change.ref = override_branch
|
||||
# change and set its branch to the override branch.
|
||||
branch_change = Branch(change.project)
|
||||
branch_change.branch = override_branch
|
||||
|
||||
if self.branch_matcher and not self.branch_matcher.matches(
|
||||
branch_change):
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user