From 7fba932e5dfbabe80b9a1faadf0b0927f7ad01ec Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Mon, 8 Jul 2019 13:46:29 -0700 Subject: [PATCH] Run jobs when their own config changes This causes file matchers to automatically match when the configuration of the job itself changes. This can be used instead of matching "^.zuul.yaml$" which may cause too many jobs to run in larger repos. Change-Id: Ieddaead91b597282c5674ba99b0c0f387843c722 --- doc/source/user/config.rst | 9 ++ ...ch-on-config-updates-1c6621885bd3e1c9.yaml | 10 ++ .../git/common-config/playbooks/run.yaml | 1 + .../job-update/git/common-config/zuul.yaml | 17 ++++ .../config/job-update/git/org_project/README | 1 + .../git/org_project/zuul.d/existing.yaml | 19 ++++ tests/fixtures/config/job-update/main.yaml | 8 ++ tests/unit/test_scheduler.py | 92 +++++++++++++++++++ tests/unit/test_web.py | 8 ++ zuul/configloader.py | 5 +- zuul/model.py | 77 +++++++++++++--- 11 files changed, 231 insertions(+), 16 deletions(-) create mode 100644 releasenotes/notes/match-on-config-updates-1c6621885bd3e1c9.yaml create mode 100644 tests/fixtures/config/job-update/git/common-config/playbooks/run.yaml create mode 100644 tests/fixtures/config/job-update/git/common-config/zuul.yaml create mode 100644 tests/fixtures/config/job-update/git/org_project/README create mode 100644 tests/fixtures/config/job-update/git/org_project/zuul.d/existing.yaml create mode 100644 tests/fixtures/config/job-update/main.yaml diff --git a/doc/source/user/config.rst b/doc/source/user/config.rst index 47563f8923..c11183a18c 100644 --- a/doc/source/user/config.rst +++ b/doc/source/user/config.rst @@ -1252,6 +1252,15 @@ Here is an example of two job definitions: are in the docs directory. A regular expression or list of regular expressions. + .. attr:: match-on-config-updates + :default: true + + If this is set to ``true`` (the default), then the job's file + matchers are ignored if a change alters the job's configuration. + This means that changes to jobs with file matchers will be + self-testing without requiring that the file matchers include + the Zuul configuration file defining the job. + .. _project: Project diff --git a/releasenotes/notes/match-on-config-updates-1c6621885bd3e1c9.yaml b/releasenotes/notes/match-on-config-updates-1c6621885bd3e1c9.yaml new file mode 100644 index 0000000000..72fe2c9c54 --- /dev/null +++ b/releasenotes/notes/match-on-config-updates-1c6621885bd3e1c9.yaml @@ -0,0 +1,10 @@ +--- +upgrade: + - | + Jobs with file matchers will now automatically match if the configuration + of the job is changed. This means that the Zuul configuration file no + longer needs to be included in the list of files to match in order for + changes to job configuration to be self-testing. + + To keep the old behavior, set :attr:`job.match-on-config-updates` + to ``False``. diff --git a/tests/fixtures/config/job-update/git/common-config/playbooks/run.yaml b/tests/fixtures/config/job-update/git/common-config/playbooks/run.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/tests/fixtures/config/job-update/git/common-config/playbooks/run.yaml @@ -0,0 +1 @@ +--- diff --git a/tests/fixtures/config/job-update/git/common-config/zuul.yaml b/tests/fixtures/config/job-update/git/common-config/zuul.yaml new file mode 100644 index 0000000000..949fd2fa92 --- /dev/null +++ b/tests/fixtures/config/job-update/git/common-config/zuul.yaml @@ -0,0 +1,17 @@ +- 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/run.yaml diff --git a/tests/fixtures/config/job-update/git/org_project/README b/tests/fixtures/config/job-update/git/org_project/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/job-update/git/org_project/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/job-update/git/org_project/zuul.d/existing.yaml b/tests/fixtures/config/job-update/git/org_project/zuul.d/existing.yaml new file mode 100644 index 0000000000..b7a32939ca --- /dev/null +++ b/tests/fixtures/config/job-update/git/org_project/zuul.d/existing.yaml @@ -0,0 +1,19 @@ +# Every fake change in the unit tests modifies "README" + +- job: + name: existing-files + files: + - README.txt + +- job: + name: existing-irr + irrelevant-files: + - README + - ^zuul.d/.*$ + +- project: + name: org/project + check: + jobs: + - existing-files + - existing-irr diff --git a/tests/fixtures/config/job-update/main.yaml b/tests/fixtures/config/job-update/main.yaml new file mode 100644 index 0000000000..208e274b13 --- /dev/null +++ b/tests/fixtures/config/job-update/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_scheduler.py b/tests/unit/test_scheduler.py index 309ea35726..bdcc966691 100644 --- a/tests/unit/test_scheduler.py +++ b/tests/unit/test_scheduler.py @@ -5712,6 +5712,98 @@ For CI problems and help debugging, contact ci@example.org""" ], ordered=False) +class TestJobUpdateFileMatcher(ZuulTestCase): + tenant_config_file = 'config/job-update/main.yaml' + + def test_matchers(self): + "Test matchers work as expected with no change" + file_dict = {'README.txt': ''} + A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A', + files=file_dict) + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + file_dict = {'something_else': ''} + B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B', + files=file_dict) + self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertHistory([ + dict(name='existing-files', result='SUCCESS', changes='1,1'), + dict(name='existing-irr', result='SUCCESS', changes='2,1'), + ]) + + def test_job_update(self): + "Test matchers are overridden with a config update" + in_repo_conf = textwrap.dedent( + """ + - job: + name: existing-files + tags: foo + - job: + name: existing-irr + tags: foo + """) + + file_dict = {'zuul.d/new.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.assertHistory([ + dict(name='existing-files', result='SUCCESS', changes='1,1'), + dict(name='existing-irr', result='SUCCESS', changes='1,1'), + ], ordered=False) + + def test_new_job(self): + "Test matchers are overridden when creating a new job" + in_repo_conf = textwrap.dedent( + """ + - job: + name: new-files + parent: existing-files + + - project: + check: + jobs: + - new-files + """) + + file_dict = {'zuul.d/new.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.assertHistory([ + dict(name='new-files', result='SUCCESS', changes='1,1'), + ]) + + def test_disable_match(self): + "Test matchers are not overridden if we say so" + in_repo_conf = textwrap.dedent( + """ + - job: + name: new-files + parent: existing-files + match-on-config-updates: false + + - project: + check: + jobs: + - new-files + """) + + file_dict = {'zuul.d/new.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.assertHistory([]) + + class TestAmbiguousProjectNames(ZuulTestCase): config_file = 'zuul-connections-multiple-gerrits.conf' tenant_config_file = 'config/ambiguous-names/main.yaml' diff --git a/tests/unit/test_web.py b/tests/unit/test_web.py index f243231846..1c5767fe76 100644 --- a/tests/unit/test_web.py +++ b/tests/unit/test_web.py @@ -324,6 +324,7 @@ class TestWeb(BaseTestWeb): 'description': None, 'files': [], 'irrelevant_files': [], + 'match_on_config_updates': True, 'final': False, 'implied_branch': None, 'nodeset': { @@ -364,6 +365,7 @@ class TestWeb(BaseTestWeb): 'description': None, 'files': [], 'irrelevant_files': [], + 'match_on_config_updates': True, 'final': False, 'implied_branch': None, 'nodeset': { @@ -410,6 +412,7 @@ class TestWeb(BaseTestWeb): 'final': False, 'implied_branch': None, 'irrelevant_files': [], + 'match_on_config_updates': True, 'name': 'test-job', 'parent': 'base', 'post_review': None, @@ -527,6 +530,7 @@ class TestWeb(BaseTestWeb): 'final': False, 'implied_branch': None, 'irrelevant_files': [], + 'match_on_config_updates': True, 'name': 'project-merge', 'parent': 'base', 'post_review': None, @@ -560,6 +564,7 @@ class TestWeb(BaseTestWeb): 'final': False, 'implied_branch': None, 'irrelevant_files': [], + 'match_on_config_updates': True, 'name': 'project-test1', 'parent': 'base', 'post_review': None, @@ -593,6 +598,7 @@ class TestWeb(BaseTestWeb): 'final': False, 'implied_branch': None, 'irrelevant_files': [], + 'match_on_config_updates': True, 'name': 'project-test2', 'parent': 'base', 'post_review': None, @@ -626,6 +632,7 @@ class TestWeb(BaseTestWeb): 'final': False, 'implied_branch': None, 'irrelevant_files': [], + 'match_on_config_updates': True, 'name': 'project1-project2-integration', 'parent': 'base', 'post_review': None, @@ -679,6 +686,7 @@ class TestWeb(BaseTestWeb): 'final': False, 'implied_branch': None, 'irrelevant_files': [], + 'match_on_config_updates': True, 'name': 'project-post', 'parent': 'base', 'post_review': None, diff --git a/zuul/configloader.py b/zuul/configloader.py index cf586b46f4..e56d13ac5a 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -621,7 +621,9 @@ class JobParser(object): 'override-checkout': str, 'description': str, 'variant-description': str, - 'post-review': bool} + 'post-review': bool, + 'match-on-config-updates': bool, + } job_name = {vs.Required('name'): str} @@ -645,6 +647,7 @@ class JobParser(object): 'success-url', 'override-branch', 'override-checkout', + 'match-on-config-updates', ] def __init__(self, pcontext): diff --git a/zuul/model.py b/zuul/model.py index c01135aea0..d6ec52c9f4 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -1141,6 +1141,7 @@ class Job(ConfigObject): branch_matcher=None, file_matcher=None, irrelevant_file_matcher=None, # skip-if + match_on_config_updates=True, tags=frozenset(), provides=frozenset(), requires=frozenset(), @@ -1253,6 +1254,7 @@ class Job(ConfigObject): d['cleanup_run'] = list(map(lambda x: x.toSchemaDict(), self.cleanup_run)) d['post_review'] = self.post_review + d['match_on_config_updates'] = self.match_on_config_updates if self.isBase(): d['parent'] = None elif self.parent: @@ -2111,6 +2113,7 @@ class QueueItem(object): self.layout = None self.project_pipeline_config = None self.job_graph = None + self._old_job_graph = None # Cached job graph of previous layout self._cached_sql_results = {} self.event = event # The trigger event that lead to this queue item @@ -2131,6 +2134,7 @@ class QueueItem(object): self.layout = None self.project_pipeline_config = None self.job_graph = None + self._old_job_graph = None def addBuild(self, build): self.current_build_set.addBuild(build) @@ -2176,6 +2180,7 @@ class QueueItem(object): except Exception: self.project_pipeline_config = None self.job_graph = None + self._old_job_graph = None raise def hasJobGraph(self): @@ -2864,6 +2869,31 @@ class QueueItem(object): newrev=newrev, ) + def updatesJobConfig(self, job): + log = self.annotateLogger(self.log) + layout_ahead = self.pipeline.tenant.layout + if self.item_ahead and self.item_ahead.layout: + layout_ahead = self.item_ahead.layout + if layout_ahead and self.layout and self.layout is not layout_ahead: + # This change updates the layout. Calculate the job as it + # would be if the layout had not changed. + if self._old_job_graph is None: + ppc = layout_ahead.getProjectPipelineConfig(self) + log.debug("Creating job graph for config change detecction") + self._old_job_graph = layout_ahead.createJobGraph( + self, ppc, skip_file_matcher=True) + log.debug("Done creating job graph for " + "config change detecction") + old_job = self._old_job_graph.jobs.get(job.name) + if old_job is None: + log.debug("Found a newly created job") + return True # A newly created job + if (job.toDict(self.pipeline.tenant) != + old_job.toDict(self.pipeline.tenant)): + log.debug("Found an updated job") + return True # This job's configuration has changed + return False + class Ref(object): """An existing state of a Project.""" @@ -3942,6 +3972,17 @@ class Layout(object): frozen_job.applyVariant(variant, item.layout) frozen_job.name = variant.name frozen_job.name = jobname + + # Now merge variables set from this parent ppc + # (i.e. project+templates) directly into the job vars + frozen_job.updateProjectVariables(ppc.variables) + + # If the job does not specify an ansible version default to the + # tenant default. + if not frozen_job.ansible_version: + frozen_job.ansible_version = \ + item.layout.tenant.default_ansible_version + log.debug("Froze job %s for %s", jobname, change) # Whether the change matches any of the project pipeline # variants @@ -3965,13 +4006,29 @@ class Layout(object): item.debug("No matching pipeline variants for {jobname}". format(jobname=jobname), indent=2) continue + updates_job_config = False if not skip_file_matcher and \ not frozen_job.changeMatchesFiles(change): - log.debug("Job %s did not match files in %s", - repr(frozen_job), change) - item.debug("Job {jobname} did not match files". - format(jobname=jobname), indent=2) - continue + matched_files = False + if frozen_job.match_on_config_updates: + updates_job_config = item.updatesJobConfig(frozen_job) + else: + matched_files = True + if not matched_files: + if updates_job_config: + # Log the reason we're ignoring the file matcher + log.debug("The configuration of job %s is " + "changed by %s; ignoring file matcher", + repr(frozen_job), change) + item.debug("The configuration of job {jobname} is " + "changed; ignoring file matcher". + format(jobname=jobname), indent=2) + else: + log.debug("Job %s did not match files in %s", + repr(frozen_job), change) + item.debug("Job {jobname} did not match files". + format(jobname=jobname), indent=2) + continue if frozen_job.abstract: raise Exception("Job %s is abstract and may not be " "directly run" % @@ -3989,16 +4046,6 @@ class Layout(object): raise Exception("Job %s does not specify a run playbook" % ( frozen_job.name,)) - # Now merge variables set from this parent ppc - # (i.e. project+templates) directly into the job vars - frozen_job.updateProjectVariables(ppc.variables) - - # If the job does not specify an ansible version default to the - # tenant default. - if not frozen_job.ansible_version: - frozen_job.ansible_version = \ - item.layout.tenant.default_ansible_version - job_graph.addJob(frozen_job) def createJobGraph(self, item, ppc, skip_file_matcher=False):