From ebf65d3de8d50c22a3937ee1de8df381cdef3590 Mon Sep 17 00:00:00 2001 From: Ian Wienand Date: Thu, 23 Apr 2020 15:29:10 +1000 Subject: [PATCH] Add intermediate flag for jobs I recently had an issue using our nodepool container jobs from the dib repository. Nodepool jobs define a base container job and then two variants; one for installing with released tools and one for installing with siblings from git. In this case, you never really want to inherit from the base job; you want to choose one of the two variants. Although the base job was marked abstract, it didn't really stop me accidentally inheriting from it directly. This proposes an "intermediate" flag that would be applied to such a base job. In a case such as above, it would have raised an error and alerted me to use one of the two other jobs. Change-Id: Ifbb9ffa65f86a6b86b63a38e3234a12b564ba3c1 --- doc/source/reference/job_def.rst | 15 ++++ .../intermediate-jobs-101e04e7e1497af9.yaml | 5 ++ .../git/common-config/playbooks/base.yaml | 2 + .../intermediate/git/common-config/zuul.yaml | 50 +++++++++++++ .../intermediate/git/org_project/README | 1 + tests/fixtures/config/intermediate/main.yaml | 8 ++ tests/unit/test_v3.py | 75 +++++++++++++++++++ tests/unit/test_web.py | 8 ++ zuul/configloader.py | 5 ++ zuul/model.py | 18 ++++- 10 files changed, 186 insertions(+), 1 deletion(-) create mode 100644 releasenotes/notes/intermediate-jobs-101e04e7e1497af9.yaml create mode 100644 tests/fixtures/config/intermediate/git/common-config/playbooks/base.yaml create mode 100644 tests/fixtures/config/intermediate/git/common-config/zuul.yaml create mode 100644 tests/fixtures/config/intermediate/git/org_project/README create mode 100644 tests/fixtures/config/intermediate/main.yaml diff --git a/doc/source/reference/job_def.rst b/doc/source/reference/job_def.rst index b5fe636272..1562c5f699 100644 --- a/doc/source/reference/job_def.rst +++ b/doc/source/reference/job_def.rst @@ -153,6 +153,21 @@ Here is an example of two job definitions: limitation does not apply to jobs in a :term:`config-project`. + .. attr:: intermediate + :default: false + + An intermediate job must be inherited by an abstract job; it can + not be inherited by a final job. All ``intermediate`` jobs + *must* also be ``abstract``; a configuration error will be + raised if not. + + For example, you may define a base abstract job `foo` and create + two abstract jobs that inherit from `foo` called + `foo-production` and `foo-development`. If it would be an error + to accidentally inherit from the base job `foo` instead of + choosing one of the two variants, `foo` could be marked as + ``intermediate``. + .. attr:: success-message :default: SUCCESS diff --git a/releasenotes/notes/intermediate-jobs-101e04e7e1497af9.yaml b/releasenotes/notes/intermediate-jobs-101e04e7e1497af9.yaml new file mode 100644 index 0000000000..3a1b4c6f69 --- /dev/null +++ b/releasenotes/notes/intermediate-jobs-101e04e7e1497af9.yaml @@ -0,0 +1,5 @@ +--- +features: + - Jobs may specify the new ``intermediate`` flag to note they may only + be inherited by abstract jobs. This can be useful if building a job + hierarchy where wish to limit where a base job is instantiated. diff --git a/tests/fixtures/config/intermediate/git/common-config/playbooks/base.yaml b/tests/fixtures/config/intermediate/git/common-config/playbooks/base.yaml new file mode 100644 index 0000000000..f679dceaef --- /dev/null +++ b/tests/fixtures/config/intermediate/git/common-config/playbooks/base.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/intermediate/git/common-config/zuul.yaml b/tests/fixtures/config/intermediate/git/common-config/zuul.yaml new file mode 100644 index 0000000000..6c787c91db --- /dev/null +++ b/tests/fixtures/config/intermediate/git/common-config/zuul.yaml @@ -0,0 +1,50 @@ +- pipeline: + name: check + manager: independent + trigger: + gerrit: + - event: patchset-created + success: + gerrit: + Verified: 1 + failure: + gerrit: + Verified: -1 + +- job: + name: base + parent: null + run: playbooks/base.yaml + +- job: + name: job-abstract-intermediate + abstract: true + intermediate: true + +- job: + name: job-abstract + abstract: true + parent: job-abstract-intermediate + +# an intermediate, with an intermediate parent also +- job: + name: job-another-intermediate + parent: job-abstract-intermediate + abstract: true + intermediate: true + +- job: + name: job-another-abstract + parent: job-another-intermediate + abstract: true + +- job: + name: job-actual + parent: job-another-abstract + run: playbooks/base.yaml + +- project: + name: org/project + check: + jobs: [] + diff --git a/tests/fixtures/config/intermediate/git/org_project/README b/tests/fixtures/config/intermediate/git/org_project/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/intermediate/git/org_project/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/intermediate/main.yaml b/tests/fixtures/config/intermediate/main.yaml new file mode 100644 index 0000000000..208e274b13 --- /dev/null +++ b/tests/fixtures/config/intermediate/main.yaml @@ -0,0 +1,8 @@ +- tenant: + name: tenant-one + source: + gerrit: + config-projects: + - common-config + untrusted-projects: + - org/project diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py index 7753b96339..266bed5242 100644 --- a/tests/unit/test_v3.py +++ b/tests/unit/test_v3.py @@ -221,6 +221,81 @@ class TestAbstract(ZuulTestCase): self.assertEqual(A.patchsets[-1]['approvals'][0]['value'], '1') +class TestIntermediate(ZuulTestCase): + tenant_config_file = 'config/intermediate/main.yaml' + + def test_intermediate_fail(self): + # you can not instantiate from an intermediate job + in_repo_conf = textwrap.dedent( + """ + - job: + name: job-instantiate-intermediate + parent: job-abstract-intermediate + + - project: + check: + jobs: + - job-instantiate-intermediate + """) + + file_dict = {'zuul.yaml': in_repo_conf} + A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A', + files=file_dict) + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 1) + self.assertEqual(A.patchsets[-1]['approvals'][0]['value'], '-1') + self.assertIn('may only inherit to another abstract job', + A.messages[0]) + + def test_intermediate_config_fail(self): + # an intermediate job must also be abstract + in_repo_conf = textwrap.dedent( + """ + - job: + name: job-intermediate-but-not-abstract + intermediate: true + abstract: false + + - project: + check: + jobs: + - job-intermediate-but-not-abstract + """) + + file_dict = {'zuul.yaml': in_repo_conf} + A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A', + files=file_dict) + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 1) + self.assertEqual(A.patchsets[-1]['approvals'][0]['value'], '-1') + self.assertIn('An intermediate job must also be abstract', + A.messages[0]) + + def test_intermediate_several(self): + # test passing through several intermediate jobs + in_repo_conf = textwrap.dedent( + """ + - project: + name: org/project + check: + jobs: + - job-actual + """) + + file_dict = {'.zuul.yaml': in_repo_conf} + A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A', + files=file_dict) + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 1) + self.assertEqual(A.patchsets[-1]['approvals'][0]['value'], '1') + + class TestFinal(ZuulTestCase): tenant_config_file = 'config/final/main.yaml' diff --git a/tests/unit/test_web.py b/tests/unit/test_web.py index af968b1bc9..528ce2671f 100644 --- a/tests/unit/test_web.py +++ b/tests/unit/test_web.py @@ -335,6 +335,7 @@ class TestWeb(BaseTestWeb): 'dependencies': [], 'description': None, 'files': [], + 'intermediate': False, 'irrelevant_files': [], 'match_on_config_updates': True, 'final': False, @@ -376,6 +377,7 @@ class TestWeb(BaseTestWeb): 'dependencies': [], 'description': None, 'files': [], + 'intermediate': False, 'irrelevant_files': [], 'match_on_config_updates': True, 'final': False, @@ -422,6 +424,7 @@ class TestWeb(BaseTestWeb): 'description': None, 'files': [], 'final': False, + 'intermediate': False, 'irrelevant_files': [], 'match_on_config_updates': True, 'name': 'test-job', @@ -540,6 +543,7 @@ class TestWeb(BaseTestWeb): 'description': None, 'files': [], 'final': False, + 'intermediate': False, 'irrelevant_files': [], 'match_on_config_updates': True, 'name': 'project-merge', @@ -574,6 +578,7 @@ class TestWeb(BaseTestWeb): 'description': None, 'files': [], 'final': False, + 'intermediate': False, 'irrelevant_files': [], 'match_on_config_updates': True, 'name': 'project-test1', @@ -608,6 +613,7 @@ class TestWeb(BaseTestWeb): 'description': None, 'files': [], 'final': False, + 'intermediate': False, 'irrelevant_files': [], 'match_on_config_updates': True, 'name': 'project-test2', @@ -642,6 +648,7 @@ class TestWeb(BaseTestWeb): 'description': None, 'files': [], 'final': False, + 'intermediate': False, 'irrelevant_files': [], 'match_on_config_updates': True, 'name': 'project1-project2-integration', @@ -696,6 +703,7 @@ class TestWeb(BaseTestWeb): 'description': None, 'files': [], 'final': False, + 'intermediate': False, 'irrelevant_files': [], 'match_on_config_updates': True, 'name': 'project-post', diff --git a/zuul/configloader.py b/zuul/configloader.py index 629de58d51..d49a280815 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -598,6 +598,7 @@ class JobParser(object): 'final': bool, 'abstract': bool, 'protected': bool, + 'intermediate': bool, 'requires': to_list(str), 'provides': to_list(str), 'failure-message': str, @@ -650,6 +651,7 @@ class JobParser(object): 'final', 'abstract', 'protected', + 'intermediate', 'timeout', 'post-timeout', 'workspace', @@ -805,6 +807,9 @@ class JobParser(object): job.roles, secrets) job.run = job.run + (run,) + if conf.get('intermediate', False) and not conf.get('abstract', False): + raise Exception("An intermediate job must also be abstract") + for k in self.simple_attributes: a = k.replace('-', '_') if k in conf: diff --git a/zuul/model.py b/zuul/model.py index 04961dde1a..096fa16434 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -1198,6 +1198,7 @@ class Job(ConfigObject): attempts=3, final=False, abstract=False, + intermediate=False, protected=None, roles=(), required_projects={}, @@ -1260,6 +1261,7 @@ class Job(ConfigObject): d['variables'] = self.variables d['final'] = self.final d['abstract'] = self.abstract + d['intermediate'] = self.intermediate d['protected'] = self.protected d['voting'] = self.voting d['timeout'] = self.timeout @@ -1551,7 +1553,8 @@ class Job(ConfigObject): for k in self.execution_attributes: if (other._get(k) is not None and - k not in set(['final', 'abstract', 'protected'])): + k not in set(['final', 'abstract', 'protected', + 'intermediate'])): if self.final: raise Exception("Unable to modify final job %s attribute " "%s=%s with variant %s" % ( @@ -1584,6 +1587,19 @@ class Job(ConfigObject): elif other.abstract: self.abstract = True + # An intermediate job may only be inherited by an abstract + # job. Note intermediate jobs must be also be abstract, that + # has been enforced during config reading. Similar to + # abstract, it is cleared by inheriting. + if self.intermediate and not other.abstract: + raise Exception("Intermediate job %s may only inherit " + "to another abstract job" % + (repr(self))) + if other.name != self.name: + self.intermediate = other.intermediate + elif other.intermediate: + self.intermediate = True + # Protected may only be set to true if other.protected is not None: # don't allow to reset protected flag