From 9eef4e532aa31b19672ea2a601268924ee72ab17 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Thu, 6 Feb 2020 14:17:22 -0800 Subject: [PATCH] Don't run jobs if only their file matchers are updated Normally, if a change is made to a job configuration, we want to ignore the file matchers and run the job so that we can see the update in practice. However, we don't want to do so if the *only* change to the job configuration is the file matchers. The result is no different than before, and running the job tells us nothing about the value of the change. This behavior was already the case, but due to a bug. This change both fixes the bug and intentionally restores the behavior. The bug that is corrected is that when variants were applied, the actual file matchers attached to the frozen job were updated, but not their string/list representation (stored in the '_files' and '_irrelevant_files' attributes). This change moves those attributes to the list of context attributes (which are automatically copied on inheritance) -- the same as the actual matcher attributes. We also do the same for the '_branches' attribute (the text version of the branch matcher). These changes make the serialized form of the frozen job (which is returned by the web api) more accurate. And in the case of the files matchers, causes Zuul to correctly detect that the job has changed when they are updated. To restore the current (accidental but desired) behavior, we now pop the files and irrelevant-files keys from the serialized representation of the job before comparison when deciding whether to run the job during a config update. This means that it will only run if something other than a files matcher is changed. This change also removes the _implied_branch job attribute because it is no longer used anywhere (implied branches now arrive via projects and project templates). Change-Id: Id1d595873fe87ca980b2f33832f55542f9e5d496 --- tests/unit/test_scheduler.py | 27 +++++++++++++++++++++++++++ tests/unit/test_web.py | 8 -------- web/src/containers/job/JobVariant.jsx | 2 +- zuul/model.py | 18 +++++++++++------- 4 files changed, 39 insertions(+), 16 deletions(-) diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py index 3a306087e9..bb4c949e58 100644 --- a/tests/unit/test_scheduler.py +++ b/tests/unit/test_scheduler.py @@ -6256,6 +6256,33 @@ class TestJobUpdateFileMatcher(ZuulTestCase): dict(name='existing-irr', result='SUCCESS', changes='1,1'), ], ordered=False) + def test_job_update_files(self): + "Test that changes to file matchers themselves don't run jobs" + # Normally we want to ignore file matchers and run jobs if the + # job config changes, but if the only thing about the job + # config that changes *is* the file matchers, then we don't + # want to run it. + in_repo_conf = textwrap.dedent( + """ + - job: + name: existing-files + files: 'doesnotexist' + - job: + name: existing-irr + irrelevant-files: + - README + - ^zuul.d/.*$ + - newthing + """) + + 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([]) + def test_new_job(self): "Test matchers are overridden when creating a new job" in_repo_conf = textwrap.dedent( diff --git a/tests/unit/test_web.py b/tests/unit/test_web.py index 52caba989d..6d1a5a1333 100644 --- a/tests/unit/test_web.py +++ b/tests/unit/test_web.py @@ -336,7 +336,6 @@ class TestWeb(BaseTestWeb): 'irrelevant_files': [], 'match_on_config_updates': True, 'final': False, - 'implied_branch': None, 'nodeset': { 'groups': [], 'name': '', @@ -378,7 +377,6 @@ class TestWeb(BaseTestWeb): 'irrelevant_files': [], 'match_on_config_updates': True, 'final': False, - 'implied_branch': None, 'nodeset': { 'groups': [], 'name': '', @@ -422,7 +420,6 @@ class TestWeb(BaseTestWeb): 'description': None, 'files': [], 'final': False, - 'implied_branch': None, 'irrelevant_files': [], 'match_on_config_updates': True, 'name': 'test-job', @@ -541,7 +538,6 @@ class TestWeb(BaseTestWeb): 'description': None, 'files': [], 'final': False, - 'implied_branch': None, 'irrelevant_files': [], 'match_on_config_updates': True, 'name': 'project-merge', @@ -576,7 +572,6 @@ class TestWeb(BaseTestWeb): 'description': None, 'files': [], 'final': False, - 'implied_branch': None, 'irrelevant_files': [], 'match_on_config_updates': True, 'name': 'project-test1', @@ -611,7 +606,6 @@ class TestWeb(BaseTestWeb): 'description': None, 'files': [], 'final': False, - 'implied_branch': None, 'irrelevant_files': [], 'match_on_config_updates': True, 'name': 'project-test2', @@ -646,7 +640,6 @@ class TestWeb(BaseTestWeb): 'description': None, 'files': [], 'final': False, - 'implied_branch': None, 'irrelevant_files': [], 'match_on_config_updates': True, 'name': 'project1-project2-integration', @@ -701,7 +694,6 @@ class TestWeb(BaseTestWeb): 'description': None, 'files': [], 'final': False, - 'implied_branch': None, 'irrelevant_files': [], 'match_on_config_updates': True, 'name': 'project-post', diff --git a/web/src/containers/job/JobVariant.jsx b/web/src/containers/job/JobVariant.jsx index b0c137344f..515852a58f 100644 --- a/web/src/containers/job/JobVariant.jsx +++ b/web/src/containers/job/JobVariant.jsx @@ -84,7 +84,7 @@ class JobVariant extends React.Component { const jobInfos = [ 'description', 'context', 'builds', 'status', - 'parent', 'attempts', 'timeout', 'semaphore', 'implied_branch', + 'parent', 'attempts', 'timeout', 'semaphore', 'nodeset', 'variables', 'override_checkout', ] jobInfos.forEach(key => { diff --git a/zuul/model.py b/zuul/model.py index c0dc7518a5..876d38217d 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -1160,8 +1160,11 @@ class Job(ConfigObject): failure_url=None, success_url=None, branch_matcher=None, + _branches=(), file_matcher=None, + _files=(), irrelevant_file_matcher=None, # skip-if + _irrelevant_files=(), match_on_config_updates=True, tags=frozenset(), provides=frozenset(), @@ -1215,10 +1218,6 @@ class Job(ConfigObject): description=None, variant_description=None, protected_origin=None, - _branches=(), - _implied_branch=None, - _files=(), - _irrelevant_files=(), secrets=(), # secrets aren't inheritable queued=False, ) @@ -1243,7 +1242,6 @@ class Job(ConfigObject): d['files'] = self._files d['irrelevant_files'] = self._irrelevant_files d['variant_description'] = self.variant_description - d['implied_branch'] = self._implied_branch if self.source_context: d['source_context'] = self.source_context.toDict() else: @@ -3002,8 +3000,14 @@ class QueueItem(object): 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)): + old_job_dict = old_job.toDict(self.pipeline.tenant) + new_job_dict = job.toDict(self.pipeline.tenant) + # Ignore changes to file matchers since they don't affect + # the content of the job. + for attr in ['files', 'irrelevant_files']: + old_job_dict.pop(attr, None) + new_job_dict.pop(attr, None) + if (new_job_dict != old_job_dict): log.debug("Found an updated job") return True # This job's configuration has changed return False