Switch to late-binding inheritance
Change-Id: Ia44c8df825098b93efb4f9c9bfd43275b6d74378
diff --git a/doc/source/user/config.rst b/doc/source/user/config.rst
index 6ac3bb1..65bffcf 100644
--- a/doc/source/user/config.rst
+++ b/doc/source/user/config.rst
@@ -448,9 +448,7 @@
a base job. Each tenant has a default parent job which will be used
if no explicit parent is specified.
-Jobs also support a concept called variance. The first time a job
-definition appears is called the reference definition of the job.
-Subsequent job definitions with the same name are called variants.
+Multiple job definitions with the same name are called variants.
These may have different selection criteria which indicate to Zuul
that, for instance, the job should behave differently on a different
git branch. Unlike inheritance, all job variants must be defined in
diff --git a/tests/fixtures/config/branch-variants/git/project-config/zuul.yaml b/tests/fixtures/config/branch-variants/git/project-config/zuul.yaml
index 89d98a9..161e5a1 100644
--- a/tests/fixtures/config/branch-variants/git/project-config/zuul.yaml
+++ b/tests/fixtures/config/branch-variants/git/project-config/zuul.yaml
@@ -11,6 +11,26 @@
gerrit:
Verified: -1
+- pipeline:
+ name: gate
+ manager: dependent
+ trigger:
+ gerrit:
+ - event: comment-added
+ approval:
+ - Approved: 1
+ success:
+ gerrit:
+ Verified: 2
+ submit: true
+ failure:
+ gerrit:
+ Verified: -2
+ start:
+ gerrit:
+ Verified: 0
+ precedence: high
+
- job:
name: base
parent: null
@@ -23,3 +43,14 @@
name: project-config
check:
jobs: []
+ gate:
+ jobs:
+ - noop
+
+- project:
+ name: puppet-integration
+ check:
+ jobs: []
+ gate:
+ jobs:
+ - noop
diff --git a/tests/fixtures/config/branch-variants/git/puppet-integration/.zuul.yaml b/tests/fixtures/config/branch-variants/git/puppet-integration/.zuul.yaml
index 2545208..322927f 100644
--- a/tests/fixtures/config/branch-variants/git/puppet-integration/.zuul.yaml
+++ b/tests/fixtures/config/branch-variants/git/puppet-integration/.zuul.yaml
@@ -11,6 +11,8 @@
name: puppet-lint
parent: puppet-module-base
run: playbooks/run-lint
+ tags:
+ - master
- project-template:
name: puppet-check-jobs
diff --git a/tests/fixtures/config/branch-variants/git/puppet-integration/stable.zuul.yaml b/tests/fixtures/config/branch-variants/git/puppet-integration/stable.zuul.yaml
new file mode 100644
index 0000000..4701b80
--- /dev/null
+++ b/tests/fixtures/config/branch-variants/git/puppet-integration/stable.zuul.yaml
@@ -0,0 +1,26 @@
+- job:
+ name: puppet-base
+ pre-run: playbooks/prepare-node-common
+
+- job:
+ name: puppet-module-base
+ parent: puppet-base
+ pre-run: playbooks/prepare-node-unit
+
+- job:
+ name: puppet-lint
+ parent: puppet-module-base
+ run: playbooks/run-lint
+ tags:
+ - stable
+
+- project-template:
+ name: puppet-check-jobs
+ check:
+ jobs:
+ - puppet-lint
+
+- project:
+ name: puppet-integration
+ templates:
+ - puppet-check-jobs
diff --git a/tests/fixtures/layouts/parent-matchers.yaml b/tests/fixtures/layouts/parent-matchers.yaml
new file mode 100644
index 0000000..d53741e
--- /dev/null
+++ b/tests/fixtures/layouts/parent-matchers.yaml
@@ -0,0 +1,34 @@
+- pipeline:
+ name: check
+ manager: independent
+ trigger:
+ gerrit:
+ - event: patchset-created
+ success:
+ gerrit:
+ Verified: 1
+ failure:
+ gerrit:
+ Verified: -1
+
+- job:
+ name: base
+ parent: null
+
+- job:
+ name: parent-job
+ files: foo.txt
+
+- job:
+ name: parent-job
+ files: bar.txt
+
+- job:
+ name: child-job
+ parent: parent-job
+
+- project:
+ name: org/project
+ check:
+ jobs:
+ - child-job
diff --git a/tests/unit/test_model.py b/tests/unit/test_model.py
index ae24f06..27a1d98 100644
--- a/tests/unit/test_model.py
+++ b/tests/unit/test_model.py
@@ -15,6 +15,7 @@
import os
import random
+from unittest import skip
import fixtures
import testtools
@@ -146,6 +147,7 @@
"Unable to modify final job"):
job.applyVariant(bad_final)
+ @skip("This test relied on early-binding inheritance")
def test_job_auth_inheritance(self):
tenant = self.tenant
layout = self.layout
diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py
index 22d1139..1b5f9dd 100755
--- a/tests/unit/test_scheduler.py
+++ b/tests/unit/test_scheduler.py
@@ -4968,6 +4968,39 @@
self.gearman_server.release()
self.waitUntilSettled()
+ @simple_layout('layouts/parent-matchers.yaml')
+ def test_parent_matchers(self):
+ "Test that if a job's parent does not match, the job does not run"
+ A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+ self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+ self.waitUntilSettled()
+
+ self.assertHistory([])
+
+ files = {'foo.txt': ''}
+ B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B',
+ files=files)
+ self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+ self.waitUntilSettled()
+
+ files = {'bar.txt': ''}
+ C = self.fake_gerrit.addFakeChange('org/project', 'master', 'C',
+ files=files)
+ self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
+ self.waitUntilSettled()
+
+ files = {'foo.txt': '', 'bar.txt': ''}
+ D = self.fake_gerrit.addFakeChange('org/project', 'master', 'D',
+ files=files)
+ self.fake_gerrit.addEvent(D.getPatchsetCreatedEvent(1))
+ self.waitUntilSettled()
+
+ self.assertHistory([
+ dict(name='child-job', result='SUCCESS', changes='2,1'),
+ dict(name='child-job', result='SUCCESS', changes='3,1'),
+ dict(name='child-job', result='SUCCESS', changes='4,1'),
+ ])
+
class TestExecutor(ZuulTestCase):
tenant_config_file = 'config/single-tenant/main.yaml'
diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py
index 77c73dd..2a2a446 100755
--- a/tests/unit/test_v3.py
+++ b/tests/unit/test_v3.py
@@ -153,13 +153,12 @@
# Thus it should fail.
self.assertEqual(A.reported, 1)
self.assertEqual(A.patchsets[-1]['approvals'][0]['value'], '-1')
- self.assertIn('Unable to inherit from final job', A.messages[0])
+ self.assertIn('Unable to modify final job', A.messages[0])
class TestBranchVariants(ZuulTestCase):
tenant_config_file = 'config/branch-variants/main.yaml'
- @skip("This is broken until the next change")
def test_branch_variants(self):
# Test branch variants of jobs with inheritance
self.executor_server.hold_jobs_in_build = True
@@ -209,6 +208,47 @@
self.executor_server.release()
self.waitUntilSettled()
+ def test_branch_variants_divergent(self):
+ # Test branches can diverge and become independent
+ self.executor_server.hold_jobs_in_build = True
+ # This creates a new branch with a copy of the config in master
+ self.create_branch('puppet-integration', 'stable')
+ self.fake_gerrit.addEvent(
+ self.fake_gerrit.getFakeBranchCreatedEvent(
+ 'puppet-integration', 'stable'))
+ self.waitUntilSettled()
+
+ with open(os.path.join(FIXTURE_DIR,
+ 'config/branch-variants/git/',
+ 'puppet-integration/stable.zuul.yaml')) as f:
+ config = f.read()
+
+ file_dict = {'.zuul.yaml': config}
+ C = self.fake_gerrit.addFakeChange('puppet-integration', 'stable', 'C',
+ files=file_dict)
+ C.addApproval('Code-Review', 2)
+ self.fake_gerrit.addEvent(C.addApproval('Approved', 1))
+ self.waitUntilSettled()
+ self.fake_gerrit.addEvent(C.getChangeMergedEvent())
+ self.waitUntilSettled()
+
+ A = self.fake_gerrit.addFakeChange('puppet-integration', 'master', 'A')
+ self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+ self.waitUntilSettled()
+ B = self.fake_gerrit.addFakeChange('puppet-integration', 'stable', 'B')
+ self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
+ self.waitUntilSettled()
+
+ self.assertEqual(self.builds[0].parameters['zuul']['jobtags'],
+ ['master'])
+
+ self.assertEqual(self.builds[1].parameters['zuul']['jobtags'],
+ ['stable'])
+
+ self.executor_server.hold_jobs_in_build = False
+ self.executor_server.release()
+ self.waitUntilSettled()
+
class TestInRepoConfig(ZuulTestCase):
# A temporary class to hold new tests while others are disabled
diff --git a/zuul/configloader.py b/zuul/configloader.py
index ec5b09f..6ff7dad 100644
--- a/zuul/configloader.py
+++ b/zuul/configloader.py
@@ -491,28 +491,18 @@
job.source_context = conf.get('_source_context')
job.source_line = conf.get('_start_mark').line + 1
- is_variant = layout.hasJob(name)
- if not is_variant:
- if 'parent' in conf:
- if conf['parent'] is not None:
- # Parent job is explicitly specified, so inherit from it.
- parent = layout.getJob(conf['parent'])
- job.inheritFrom(parent)
- else:
- # Parent is explicitly set as None, so user intends
- # this to be a base job. That's only okay if we're in
- # a config project.
- if not conf['_source_context'].trusted:
- raise Exception(
- "Base jobs must be defined in config projects")
+ if 'parent' in conf:
+ if conf['parent'] is not None:
+ # Parent job is explicitly specified, so inherit from it.
+ job.parent = conf['parent']
else:
- parent = layout.getJob(tenant.default_base_job)
- job.inheritFrom(parent)
- else:
- if 'parent' in conf:
- # TODO(jeblair): warn the user that we're ignoring the
- # parent setting on this variant job definition.
- pass
+ # Parent is explicitly set as None, so user intends
+ # this to be a base job. That's only okay if we're in
+ # a config project.
+ if not conf['_source_context'].trusted:
+ raise Exception(
+ "Base jobs must be defined in config projects")
+ job.parent = job.BASE_JOB_MARKER
# Secrets are part of the playbook context so we must establish
# them earlier than playbooks.
@@ -596,7 +586,7 @@
run_name = os.path.join('playbooks', job.name)
run = model.PlaybookContext(job.source_context, run_name,
job.roles, secrets)
- job.implied_run = (run,) + job.implied_run
+ job.implied_run = (run,)
for k in JobParser.simple_attributes:
a = k.replace('-', '_')
@@ -632,14 +622,11 @@
job_project = model.JobProject(project_name,
project_override_branch)
new_projects[project_name] = job_project
- job.updateProjects(new_projects)
+ job.required_projects = new_projects
tags = conf.get('tags')
if tags:
- # Tags are merged via a union rather than a
- # destructive copy because they are intended to
- # accumulate onto any previously applied tags.
- job.tags = job.tags.union(set(tags))
+ job.tags = set(tags)
job.dependencies = frozenset(as_list(conf.get('dependencies')))
@@ -647,7 +634,7 @@
if variables:
if 'zuul' in variables:
raise Exception("Variables named 'zuul' are not allowed.")
- job.updateVariables(variables)
+ job.variables = variables
allowed_projects = conf.get('allowed-projects', None)
if allowed_projects:
@@ -1511,6 +1498,16 @@
"Skipped adding job %s which shadows an existing job" %
(job,))
+ # Now that all the jobs are loaded, verify their parents exist
+ for config_job in data.jobs:
+ classes = TenantParser._getLoadClasses(tenant, config_job)
+ if 'job' not in classes:
+ continue
+ with configuration_exceptions('job', config_job):
+ parent = config_job.get('parent')
+ if parent:
+ layout.getJob(parent)
+
if not skip_semaphores:
for config_semaphore in data.semaphores:
classes = TenantParser._getLoadClasses(
diff --git a/zuul/model.py b/zuul/model.py
index 7b0f4d6..ee2ea26 100644
--- a/zuul/model.py
+++ b/zuul/model.py
@@ -79,6 +79,12 @@
STATE_DELETING])
+class NoMatchingParentError(Exception):
+ """A job referenced a parent, but that parent had no variants which
+ matched the current change."""
+ pass
+
+
class Attributes(object):
"""A class to hold attributes for string formatting."""
@@ -697,6 +703,13 @@
self.roles == other.roles and
self.secrets == other.secrets)
+ def copy(self):
+ r = PlaybookContext(self.source_context,
+ self.path,
+ self.roles,
+ self.secrets)
+ return r
+
def toDict(self):
# Render to a dict to use in passing json to the executor
secrets = {}
@@ -785,6 +798,8 @@
(e.g., "job.run = ..." rather than "job.run.append(...)").
"""
+ BASE_JOB_MARKER = object()
+
def __init__(self, name):
# These attributes may override even the final form of a job
# in the context of a project-pipeline. They can not affect
@@ -811,6 +826,7 @@
# declared "final", these may not be overriden in a
# project-pipeline.
self.execution_attributes = dict(
+ parent=None,
timeout=None,
variables={},
nodeset=NodeSet(),
@@ -818,7 +834,6 @@
pre_run=(),
post_run=(),
run=(),
- implied_run=(),
semaphore=None,
attempts=3,
final=False,
@@ -837,6 +852,7 @@
source_line=None,
inheritance_path=(),
parent_data=None,
+ implied_run=(),
)
self.inheritable_attributes = {}
@@ -888,9 +904,13 @@
def getSafeAttributes(self):
return Attributes(name=self.name)
+ def isBase(self):
+ return self.parent is self.BASE_JOB_MARKER
+
+ def setBase(self):
+ self.inheritance_path = self.inheritance_path + (repr(self),)
+
def setRun(self):
- msg = 'self %s' % (repr(self),)
- self.inheritance_path = self.inheritance_path + (msg,)
if not self.run:
self.run = self.implied_run
@@ -964,24 +984,6 @@
else:
a[k] = bv
- def inheritFrom(self, other):
- """Copy the inheritable attributes which have been set on the other
- job to this job."""
- if not isinstance(other, Job):
- raise Exception("Job unable to inherit from %s" % (other,))
-
- if other.final:
- raise Exception("Unable to inherit from final job %s" %
- (repr(other),))
-
- # copy all attributes
- for k in self.inheritable_attributes:
- if (other._get(k) is not None):
- setattr(self, k, getattr(other, k))
-
- msg = 'inherit from %s' % (repr(other),)
- self.inheritance_path = other.inheritance_path + (msg,)
-
def copy(self):
job = Job(self.name)
for k in self.attributes:
@@ -989,10 +991,22 @@
setattr(job, k, copy.deepcopy(self._get(k)))
return job
+ def freezePlaybooks(self, pblist):
+ """Take a list of playbooks, and return a copy of it updated with this
+ job's roles.
+
+ """
+
+ ret = []
+ for old_pb in pblist:
+ pb = old_pb.copy()
+ pb.roles = self.roles
+ ret.append(pb)
+ return tuple(ret)
+
def applyVariant(self, other):
"""Copy the attributes which have been set on the other job to this
job."""
-
if not isinstance(other, Job):
raise Exception("Job unable to inherit from %s" % (other,))
@@ -1004,8 +1018,9 @@
"%s=%s with variant %s" % (
repr(self), k, other._get(k),
repr(other)))
- if k not in set(['pre_run', 'post_run', 'roles', 'variables',
- 'required_projects']):
+ if k not in set(['pre_run', 'run', 'post_run', 'roles',
+ 'variables', 'required_projects']):
+ # TODO(jeblair): determine if deepcopy is required
setattr(self, k, copy.deepcopy(other._get(k)))
# Don't set final above so that we don't trip an error halfway
@@ -1013,12 +1028,24 @@
if other.final != self.attributes['final']:
self.final = other.final
- if other._get('pre_run') is not None:
- self.pre_run = self.pre_run + other.pre_run
- if other._get('post_run') is not None:
- self.post_run = other.post_run + self.post_run
+ # We must update roles before any playbook contexts
if other._get('roles') is not None:
self.addRoles(other.roles)
+
+ # We only want to update implied run for inheritance, not
+ # variance.
+ if self.name != other.name:
+ other_implied_run = self.freezePlaybooks(other.implied_run)
+ self.implied_run = other_implied_run + self.implied_run
+ if other._get('run') is not None:
+ other_run = self.freezePlaybooks(other.run)
+ self.run = other_run
+ if other._get('pre_run') is not None:
+ other_pre_run = self.freezePlaybooks(other.pre_run)
+ self.pre_run = self.pre_run + other_pre_run
+ if other._get('post_run') is not None:
+ other_post_run = self.freezePlaybooks(other.post_run)
+ self.post_run = other_post_run + self.post_run
if other._get('variables') is not None:
self.updateVariables(other.variables)
if other._get('required_projects') is not None:
@@ -1032,8 +1059,7 @@
if other._get('tags') is not None:
self.tags = self.tags.union(other.tags)
- msg = 'apply variant %s' % (repr(other),)
- self.inheritance_path = self.inheritance_path + (msg,)
+ self.inheritance_path = self.inheritance_path + (repr(other),)
def changeMatches(self, change):
if self.branch_matcher and not self.branch_matcher.matches(change):
@@ -2386,7 +2412,9 @@
# elements are aspects of that job with different matchers
# that override some attribute of the job. These aspects all
# inherit from the reference definition.
- self.jobs = {'noop': [Job('noop')]}
+ noop = Job('noop')
+ noop.parent = noop.BASE_JOB_MARKER
+ self.jobs = {'noop': [noop]}
self.nodesets = {}
self.secrets = {}
self.semaphores = {}
@@ -2464,27 +2492,61 @@
def addProjectConfig(self, project_config):
self.project_configs[project_config.name] = project_config
+ def collectJobs(self, jobname, change, path=None, jobs=None, stack=None):
+ if stack is None:
+ stack = []
+ if jobs is None:
+ jobs = []
+ if path is None:
+ path = []
+ path.append(jobname)
+ matched = False
+ for variant in self.getJobs(jobname):
+ if not variant.changeMatches(change):
+ continue
+ if not variant.isBase():
+ parent = variant.parent
+ if not jobs and parent is None:
+ parent = self.tenant.default_base_job
+ else:
+ parent = None
+ if parent and parent not in path:
+ if parent in stack:
+ raise Exception("Dependency cycle in jobs: %s" % stack)
+ self.collectJobs(parent, change, path, jobs, stack + [jobname])
+ matched = True
+ jobs.append(variant)
+ if not matched:
+ raise NoMatchingParentError()
+ return jobs
+
def _createJobGraph(self, item, job_list, job_graph):
change = item.change
pipeline = item.pipeline
for jobname in job_list.jobs:
# This is the final job we are constructing
frozen_job = None
- # Whether the change matches any globally defined variant
- matched = False
- for variant in self.getJobs(jobname):
- if variant.changeMatches(change):
- if frozen_job is None:
- frozen_job = variant.copy()
- frozen_job.setRun()
- else:
- frozen_job.applyVariant(variant)
- matched = True
- if not matched:
+ try:
+ variants = self.collectJobs(jobname, change)
+ except NoMatchingParentError:
+ variants = None
+ if not variants:
# A change must match at least one defined job variant
# (that is to say that it must match more than just
# the job that is defined in the tree).
continue
+ for variant in variants:
+ if frozen_job is None:
+ frozen_job = variant.copy()
+ frozen_job.setBase()
+ else:
+ frozen_job.applyVariant(variant)
+ frozen_job.name = variant.name
+ # Set the implied run based on the last top-level job
+ # definition, before we start applying project-pipeline
+ # variants.
+ frozen_job.name = jobname
+ frozen_job.setRun()
# Whether the change matches any of the project pipeline
# variants
matched = False