From 0029267403c627a45fe9b357f645d4893bc3d4af Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Thu, 26 Oct 2017 15:29:39 -0700 Subject: [PATCH] Validate that a job has a run playbook on freeze We now know, before attempting to run a job, whether the inheritance hierarchy has produced a main playbook for the job. If there is none, error early. Also, in the executor, assume that any specified playbooks are required to exist, and use the more specific version of the error message if they don't. Change-Id: Id7dc5934c665cf939820b12b5ded53adeb60c0a8 --- tests/fixtures/layouts/no-run.yaml | 22 ++++++++++++++++++++++ tests/unit/test_model.py | 3 ++- tests/unit/test_scheduler.py | 9 +++++++++ zuul/executor/server.py | 20 +++++++------------- zuul/model.py | 4 ++++ 5 files changed, 44 insertions(+), 14 deletions(-) create mode 100644 tests/fixtures/layouts/no-run.yaml diff --git a/tests/fixtures/layouts/no-run.yaml b/tests/fixtures/layouts/no-run.yaml new file mode 100644 index 0000000000..bccee9c753 --- /dev/null +++ b/tests/fixtures/layouts/no-run.yaml @@ -0,0 +1,22 @@ +- pipeline: + name: check + manager: independent + trigger: + gerrit: + - event: patchset-created + success: + gerrit: + Verified: 1 + failure: + gerrit: + Verified: -1 + +- job: + name: base + parent: null + +- project: + name: org/project + check: + jobs: + - base diff --git a/tests/unit/test_model.py b/tests/unit/test_model.py index 11f4eebccf..784fcb3cf4 100644 --- a/tests/unit/test_model.py +++ b/tests/unit/test_model.py @@ -194,7 +194,8 @@ class TestJob(BaseTestCase): 'name': 'project', 'gate': { 'jobs': [ - {'python27': {'timeout': 70}} + {'python27': {'timeout': 70, + 'run': 'playbooks/python27.yaml'}} ] } }]) diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py index e30a43db9c..b0df2b2df2 100755 --- a/tests/unit/test_scheduler.py +++ b/tests/unit/test_scheduler.py @@ -2486,6 +2486,15 @@ class TestScheduler(ZuulTestCase): self.assertEqual([x['path'] for x in p['playbooks']], ['playbooks/python27.yaml']) + @simple_layout("layouts/no-run.yaml") + def test_job_without_run(self): + "Test that a job without a run playbook errors" + A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A') + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + self.assertIn('Job base does not specify a run playbook', + A.messages[-1]) + def test_queue_names(self): "Test shared change queue names" tenant = self.sched.abide.tenants.get('tenant-one') diff --git a/zuul/executor/server.py b/zuul/executor/server.py index 82e7bf4b09..d72d1eec39 100644 --- a/zuul/executor/server.py +++ b/zuul/executor/server.py @@ -935,7 +935,7 @@ class AnsibleJob(object): "Ansible plugin dir %s found adjacent to playbook %s in " "non-trusted repo." % (entry, path)) - def findPlaybook(self, path, required=False, trusted=False): + def findPlaybook(self, path, trusted=False): for ext in ['', '.yaml', '.yml']: fn = path + ext if os.path.exists(fn): @@ -943,35 +943,30 @@ class AnsibleJob(object): playbook_dir = os.path.dirname(os.path.abspath(fn)) self._blockPluginDirs(playbook_dir) return fn - if required: - raise ExecutorError("Unable to find playbook %s" % path) - return None + raise ExecutorError("Unable to find playbook %s" % path) def preparePlaybooks(self, args): self.writeAnsibleConfig(self.jobdir.setup_playbook) for playbook in args['pre_playbooks']: jobdir_playbook = self.jobdir.addPrePlaybook() - self.preparePlaybook(jobdir_playbook, playbook, - args, required=True) + self.preparePlaybook(jobdir_playbook, playbook, args) for playbook in args['playbooks']: jobdir_playbook = self.jobdir.addPlaybook() - self.preparePlaybook(jobdir_playbook, playbook, - args, required=False) + self.preparePlaybook(jobdir_playbook, playbook, args) if jobdir_playbook.path is not None: self.jobdir.playbook = jobdir_playbook break if self.jobdir.playbook is None: - raise ExecutorError("No valid playbook found") + raise ExecutorError("No playbook specified") for playbook in args['post_playbooks']: jobdir_playbook = self.jobdir.addPostPlaybook() - self.preparePlaybook(jobdir_playbook, playbook, - args, required=True) + self.preparePlaybook(jobdir_playbook, playbook, args) - def preparePlaybook(self, jobdir_playbook, playbook, args, required): + def preparePlaybook(self, jobdir_playbook, playbook, args): self.log.debug("Prepare playbook repo for %s" % (playbook['project'],)) # Check out the playbook repo if needed and set the path to @@ -1006,7 +1001,6 @@ class AnsibleJob(object): jobdir_playbook.path = self.findPlaybook( path, - required=required, trusted=playbook['trusted']) # If this playbook doesn't exist, don't bother preparing diff --git a/zuul/model.py b/zuul/model.py index e3da56f4ea..3f85087b93 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -2410,6 +2410,7 @@ class Layout(object): # inherit from the reference definition. noop = Job('noop') noop.parent = noop.BASE_JOB_MARKER + noop.run = 'noop.yaml' self.jobs = {'noop': [noop]} self.nodesets = {} self.secrets = {} @@ -2558,6 +2559,9 @@ class Layout(object): raise Exception("Pre-review pipeline %s does not allow " "post-review job %s" % ( pipeline.name, frozen_job.name)) + if not frozen_job.run: + raise Exception("Job %s does not specify a run playbook" % ( + frozen_job.name,)) job_graph.addJob(frozen_job) def createJobGraph(self, item):