From 1edfd97943f7c78f74913b82737a8c21cac83742 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Fri, 1 Dec 2017 15:54:24 -0800 Subject: [PATCH] Fix implied branch matchers and tags Adding an implied branch matcher to jobs on in-repo project defs works great when an item *has* a branch. But some items, such as tags, don't. With recent changes, it is now impossible for a project to add a job in-repo that runs in a tag pipeline. To correct this, we need to drop some of the optimizations which assumed we could match the implied branch against existing branch matchers, and instead, when adding a job in-repo, simply add a new kind of branch matcher, an ImpliedBranchMatcher, that is evaluated in a boolean 'and' with any existing branch matchers. The ImpliedBranchMatcher only fails if the item has a branch, and the branch doesn't match. If the item doesn't have a branch, it always succeeds. This means that when a project adds a job to a tag pipeline in-repo, it will most likely only have the ImpliedBranchMatcher, which will simply succeed. It also means that the multiple project configurations present in the project's multiple branches can all add jobs to tag pipelines, and so to remove such a job, changes may need to be made to all branches of a project. However, there's not much that can be done about that at the moment. Change-Id: Id51ddfce7ef0a6d5e3273da784e407ac72a669db --- tests/base.py | 23 +++++++++++++ .../branch-tag/git/org_project/.zuul.yaml | 9 ++++++ .../config/branch-tag/git/org_project/README | 1 + .../git/org_project/playbooks/test-job.yaml | 2 ++ .../branch-tag/git/project-config/zuul.yaml | 21 ++++++++++++ tests/fixtures/config/branch-tag/main.yaml | 8 +++++ tests/unit/test_v3.py | 12 +++++++ zuul/change_matcher.py | 14 ++++++++ zuul/manager/__init__.py | 32 ++++++++++--------- zuul/model.py | 26 +++------------ 10 files changed, 111 insertions(+), 37 deletions(-) create mode 100644 tests/fixtures/config/branch-tag/git/org_project/.zuul.yaml create mode 100644 tests/fixtures/config/branch-tag/git/org_project/README create mode 100644 tests/fixtures/config/branch-tag/git/org_project/playbooks/test-job.yaml create mode 100644 tests/fixtures/config/branch-tag/git/project-config/zuul.yaml create mode 100644 tests/fixtures/config/branch-tag/main.yaml diff --git a/tests/base.py b/tests/base.py index 036515d311..f274ed634c 100755 --- a/tests/base.py +++ b/tests/base.py @@ -486,6 +486,29 @@ class FakeGerritConnection(gerritconnection.GerritConnection): self.changes[self.change_number] = c return c + def addFakeTag(self, project, branch, tag): + path = os.path.join(self.upstream_root, project) + repo = git.Repo(path) + commit = repo.heads[branch].commit + newrev = commit.hexsha + ref = 'refs/tags/' + tag + + git.Tag.create(repo, tag, commit) + + event = { + "type": "ref-updated", + "submitter": { + "name": "User Name", + }, + "refUpdate": { + "oldRev": 40 * '0', + "newRev": newrev, + "refName": ref, + "project": project, + } + } + return event + def getFakeBranchCreatedEvent(self, project, branch): path = os.path.join(self.upstream_root, project) repo = git.Repo(path) diff --git a/tests/fixtures/config/branch-tag/git/org_project/.zuul.yaml b/tests/fixtures/config/branch-tag/git/org_project/.zuul.yaml new file mode 100644 index 0000000000..acbba6cb53 --- /dev/null +++ b/tests/fixtures/config/branch-tag/git/org_project/.zuul.yaml @@ -0,0 +1,9 @@ +- job: + name: test-job + run: playbooks/test-job.yaml + +- project: + name: org/project + tag: + jobs: + - test-job diff --git a/tests/fixtures/config/branch-tag/git/org_project/README b/tests/fixtures/config/branch-tag/git/org_project/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/branch-tag/git/org_project/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/branch-tag/git/org_project/playbooks/test-job.yaml b/tests/fixtures/config/branch-tag/git/org_project/playbooks/test-job.yaml new file mode 100644 index 0000000000..f679dceaef --- /dev/null +++ b/tests/fixtures/config/branch-tag/git/org_project/playbooks/test-job.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/branch-tag/git/project-config/zuul.yaml b/tests/fixtures/config/branch-tag/git/project-config/zuul.yaml new file mode 100644 index 0000000000..0ae639641a --- /dev/null +++ b/tests/fixtures/config/branch-tag/git/project-config/zuul.yaml @@ -0,0 +1,21 @@ +- pipeline: + name: tag + manager: independent + trigger: + gerrit: + - event: ref-updated + ref: ^refs/tags/.*$ + +- job: + name: base + parent: null + +- project: + name: project-config + tag: + jobs: [] + +- project: + name: org/project + tag: + jobs: [] diff --git a/tests/fixtures/config/branch-tag/main.yaml b/tests/fixtures/config/branch-tag/main.yaml new file mode 100644 index 0000000000..0ac232fbb1 --- /dev/null +++ b/tests/fixtures/config/branch-tag/main.yaml @@ -0,0 +1,8 @@ +- tenant: + name: tenant-one + source: + gerrit: + config-projects: + - project-config + untrusted-projects: + - org/project diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py index 70d9211232..54cf1116bb 100755 --- a/tests/unit/test_v3.py +++ b/tests/unit/test_v3.py @@ -157,6 +157,18 @@ class TestFinal(ZuulTestCase): self.assertIn('Unable to modify final job', A.messages[0]) +class TestBranchTag(ZuulTestCase): + tenant_config_file = 'config/branch-tag/main.yaml' + + def test_negative_branch_match(self): + # Test that a negative branch matcher works with implied branches. + event = self.fake_gerrit.addFakeTag('org/project', 'master', 'foo') + self.fake_gerrit.addEvent(event) + self.waitUntilSettled() + self.assertHistory([ + dict(name='test-job', result='SUCCESS', ref='refs/tags/foo')]) + + class TestBranchNegative(ZuulTestCase): tenant_config_file = 'config/branch-negative/main.yaml' diff --git a/zuul/change_matcher.py b/zuul/change_matcher.py index 7f6673d9ee..eb12f9b0dd 100644 --- a/zuul/change_matcher.py +++ b/zuul/change_matcher.py @@ -69,6 +69,20 @@ class BranchMatcher(AbstractChangeMatcher): return False +class ImpliedBranchMatcher(AbstractChangeMatcher): + """ + A branch matcher that only considers branch refs, and always + succeeds on other types (e.g., tags). + """ + + def matches(self, change): + if hasattr(change, 'branch'): + if self.regex.match(change.branch): + return True + return False + return True + + class FileMatcher(AbstractChangeMatcher): def matches(self, change): diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index 6c72c2d897..d205afc231 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -853,20 +853,22 @@ class PipelineManager(object): if dt: self.sched.statsd.timing(key + '.resident_time', dt) self.sched.statsd.incr(key + '.total_changes') - - hostname = (item.change.project.canonical_hostname. - replace('.', '_')) - projectname = (item.change.project.name. - replace('.', '_').replace('/', '.')) - projectname = projectname.replace('.', '_').replace('/', '.') - branchname = item.change.branch.replace('.', '_').replace('/', '.') - # stats.timers.zuul.tenant..pipeline.. - # project....resident_time - # stats_counts.zuul.tenant..pipeline.. - # project....total_changes - key += '.project.%s.%s.%s' % (hostname, projectname, branchname) - if dt: - self.sched.statsd.timing(key + '.resident_time', dt) - self.sched.statsd.incr(key + '.total_changes') + if hasattr(item.change, 'branch'): + hostname = (item.change.project.canonical_hostname. + replace('.', '_')) + projectname = (item.change.project.name. + replace('.', '_').replace('/', '.')) + projectname = projectname.replace('.', '_').replace('/', '.') + branchname = item.change.branch.replace('.', '_').replace( + '/', '.') + # stats.timers.zuul.tenant..pipeline.. + # project....resident_time + # stats_counts.zuul.tenant..pipeline.. + # project....total_changes + key += '.project.%s.%s.%s' % (hostname, projectname, + branchname) + if dt: + self.sched.statsd.timing(key + '.resident_time', dt) + self.sched.statsd.incr(key + '.total_changes') except Exception: self.log.exception("Exception reporting pipeline stats") diff --git a/zuul/model.py b/zuul/model.py index 8fe4d1489e..c1e1914208 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -963,10 +963,10 @@ class Job(object): return None return m - def addBranchMatcher(self, branch): + def addImpliedBranchMatcher(self, branch): # Add a branch matcher that combines as a boolean *and* with # existing branch matchers, if any. - matchers = [change_matcher.BranchMatcher(branch)] + matchers = [change_matcher.ImpliedBranchMatcher(branch)] if self.branch_matcher: matchers.append(self.branch_matcher) self.branch_matcher = change_matcher.MatchAll(matchers) @@ -1121,26 +1121,8 @@ class JobList(object): joblist = self.jobs.setdefault(jobname, []) for job in jobs: if implied_branch: - # If setting an implied branch and the current - # branch matcher is a simple match for a different - # branch, then simply do not add this job. If it - # is absent, set it to the implied branch. - # Otherwise, combine it with the implied branch to - # ensure that it still only affects this branch - # (whatever else it may do). - simple_branch = job.getSimpleBranchMatcher() - if simple_branch: - if not simple_branch.regex.match(implied_branch): - # This branch will never match, don't add it. - continue - if not simple_branch: - # The branch matcher could be complex, or - # missing. Add our implied matcher. - job = job.copy() - job.addBranchMatcher(implied_branch) - # Otherwise we have a simple branch matcher which - # is the same as our implied branch, the job can - # be added as-is. + job = job.copy() + job.addImpliedBranchMatcher(implied_branch) if job not in joblist: joblist.append(job)