From 4df7c8def963c561766a6d90cba7b66414fdd84d Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Wed, 17 Apr 2024 14:04:38 -0700 Subject: [PATCH] Support negated regexes in files/irrelevant-files These attributes are documented to support the new Zuul-standard regex format (str or {pattern: str, negate: bool}) but actually only accepted strings. Update them to include negation, and add tests. Change-Id: Ie8a4f816afc5277006b61406b7b0bd725947dd78 --- .../notes/negate-files-4edc64fc264f3fb0.yaml | 6 ++ tests/fixtures/layouts/files-negate.yaml | 30 +++++++++ .../layouts/irrelevant-files-negate.yaml | 30 +++++++++ tests/unit/test_change_matcher.py | 60 +++++++++++++++++ tests/unit/test_scheduler.py | 66 +++++++++++++++++++ zuul/configloader.py | 17 +++-- zuul/lib/re2util.py | 8 +++ zuul/model.py | 15 +++-- 8 files changed, 221 insertions(+), 11 deletions(-) create mode 100644 releasenotes/notes/negate-files-4edc64fc264f3fb0.yaml create mode 100644 tests/fixtures/layouts/files-negate.yaml create mode 100644 tests/fixtures/layouts/irrelevant-files-negate.yaml diff --git a/releasenotes/notes/negate-files-4edc64fc264f3fb0.yaml b/releasenotes/notes/negate-files-4edc64fc264f3fb0.yaml new file mode 100644 index 0000000000..0d52a7c06b --- /dev/null +++ b/releasenotes/notes/negate-files-4edc64fc264f3fb0.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + The :attr:`job.files` and :attr:`job.irrelevant-files` attributes + now fully support the new :ref:`regular expression ` + syntax, including negation. diff --git a/tests/fixtures/layouts/files-negate.yaml b/tests/fixtures/layouts/files-negate.yaml new file mode 100644 index 0000000000..0c6d809056 --- /dev/null +++ b/tests/fixtures/layouts/files-negate.yaml @@ -0,0 +1,30 @@ +- 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: project-test-files + run: playbooks/project-test-files.yaml + +- project: + name: org/project + check: + jobs: + - project-test-files: + files: + - regex: ^(dontrun|README|/COMMIT_MSG)$ + negate: true diff --git a/tests/fixtures/layouts/irrelevant-files-negate.yaml b/tests/fixtures/layouts/irrelevant-files-negate.yaml new file mode 100644 index 0000000000..6f2f039bb5 --- /dev/null +++ b/tests/fixtures/layouts/irrelevant-files-negate.yaml @@ -0,0 +1,30 @@ +- 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: project-test-irrelevant-files + run: playbooks/project-test-irrelevant-files.yaml + +- project: + name: org/project + check: + jobs: + - project-test-irrelevant-files: + irrelevant-files: + - regex: ^respectme$ + negate: true diff --git a/tests/unit/test_change_matcher.py b/tests/unit/test_change_matcher.py index 6367132ece..3133935769 100644 --- a/tests/unit/test_change_matcher.py +++ b/tests/unit/test_change_matcher.py @@ -121,6 +121,39 @@ class TestMatchAllFiles(BaseTestFilesMatcher): self._test_matches(True, files=['docs/foo']) +class TestMatchAllFilesNegate(BaseTestFilesMatcher): + + def setUp(self): + super().setUp() + self.matcher = cm.MatchAllFiles( + [cm.FileMatcher(ZuulRegex('^docs/.*$', negate=True))]) + + def test_matches_returns_false_when_files_attr_missing(self): + delattr(self.change, 'files') + self._test_matches(False) + + def test_matches_returns_false_when_no_files(self): + self._test_matches(False) + + def test_matches_returns_false_when_not_all_files_match(self): + self._test_matches(False, files=['/COMMIT_MSG', 'docs/foo', 'foo/bar']) + + def test_matches_returns_false_when_single_file_does_not_match(self): + self._test_matches(False, files=['docs/foo']) + + def test_matches_returns_false_when_commit_message_matches(self): + self._test_matches(False, files=['/COMMIT_MSG']) + + def test_matches_returns_false_when_all_files_match(self): + self._test_matches(False, files=['/COMMIT_MSG', 'docs/foo']) + + def test_matches_returns_false_when_single_file_matches(self): + self._test_matches(False, files=['docs/foo']) + + def test_matches_returns_true_when_no_files_match(self): + self._test_matches(True, files=['foo']) + + class TestMatchAnyFiles(BaseTestFilesMatcher): def setUp(self): @@ -148,6 +181,33 @@ class TestMatchAnyFiles(BaseTestFilesMatcher): self._test_matches(False, files=['/COMMIT_MSG', 'foo/bar']) +class TestMatchAnyFilesNegate(BaseTestFilesMatcher): + + def setUp(self): + super().setUp() + self.matcher = cm.MatchAnyFiles( + [cm.FileMatcher(ZuulRegex('^docs/.*$', negate=True))]) + + def test_matches_returns_true_when_files_attr_missing(self): + delattr(self.change, 'files') + self._test_matches(True) + + def test_matches_returns_true_when_no_files(self): + self._test_matches(True) + + def test_matches_returns_true_when_only_commit_message(self): + self._test_matches(True, files=['/COMMIT_MSG']) + + def test_matches_returns_true_when_some_files_match(self): + self._test_matches(True, files=['/COMMIT_MSG', 'docs/foo', 'foo/bar']) + + def test_matches_returns_false_when_single_file_matches(self): + self._test_matches(False, files=['docs/foo']) + + def test_matches_returns_true_when_no_matching_files(self): + self._test_matches(True, files=['/COMMIT_MSG', 'foo/bar']) + + class TestMatchAll(BaseTestMatcher): def test_matches_returns_true(self): diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py index a235aa3a30..bd7d33ea61 100644 --- a/tests/unit/test_scheduler.py +++ b/tests/unit/test_scheduler.py @@ -3467,6 +3467,36 @@ class TestScheduler(ZuulTestCase): self.assertEqual(B.data['status'], 'MERGED') self.assertEqual(B.reported, 2) + def _test_files_negated_jobs(self, should_skip): + "Test that jobs with negated files filter run only when appropriate" + if should_skip: + files = {'dontrun': 'me\n'} + else: + files = {'dorun': 'please!\n'} + + change = self.fake_gerrit.addFakeChange('org/project', + 'master', + 'test files', + files=files) + self.fake_gerrit.addEvent(change.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + tested_change_ids = [x.changes[0] for x in self.history + if x.name == 'project-test-files'] + + if should_skip: + self.assertEqual([], tested_change_ids) + else: + self.assertIn(change.data['number'], tested_change_ids) + + @simple_layout('layouts/files-negate.yaml') + def test_files_negated_no_match_skips_job(self): + self._test_files_negated_jobs(should_skip=True) + + @simple_layout('layouts/files-negate.yaml') + def test_files_negated_match_runs_job(self): + self._test_files_negated_jobs(should_skip=False) + def _test_irrelevant_files_jobs(self, should_skip): "Test that jobs with irrelevant-files filter run only when appropriate" if should_skip: @@ -3497,6 +3527,42 @@ class TestScheduler(ZuulTestCase): def test_irrelevant_files_no_match_runs_job(self): self._test_irrelevant_files_jobs(should_skip=False) + def _test_irrelevant_files_negated_jobs(self, should_skip): + "Test that jobs with irrelevant-files filter run only when appropriate" + if should_skip: + files = {'ignoreme': 'ignored\n'} + else: + files = {'respectme': 'please!\n'} + + change = self.fake_gerrit.addFakeChange('org/project', + 'master', + 'test irrelevant-files', + files=files) + self.fake_gerrit.addEvent(change.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + tested_change_ids = [x.changes[0] for x in self.history + if x.name == 'project-test-irrelevant-files'] + + if should_skip: + self.assertEqual([], tested_change_ids) + else: + self.assertIn(change.data['number'], tested_change_ids) + + @simple_layout('layouts/irrelevant-files-negate.yaml') + def test_irrelevant_files_negated_match_skips_job(self): + # Anything other than "respectme" is irrelevant. This adds + # "README" which is irrelevant, and "ignoreme" which is + # irrelevant, so the job should not run. + self._test_irrelevant_files_negated_jobs(should_skip=True) + + @simple_layout('layouts/irrelevant-files-negate.yaml') + def test_irrelevant_files_negated_no_match_runs_job(self): + # Anything other than "respectme" is irrelevant. This adds + # "README" which is irrelevant, and "respecme" which *is* + # relevant, so the job should run. + self._test_irrelevant_files_negated_jobs(should_skip=False) + @simple_layout('layouts/inheritance.yaml') def test_inherited_jobs_keep_matchers(self): files = {'ignoreme': 'ignored\n'} diff --git a/zuul/configloader.py b/zuul/configloader.py index 2242841405..24fb2b8da4 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -571,9 +571,9 @@ class JobParser(object): 'semaphores': to_list(vs.Any(semaphore, str)), 'tags': to_list(str), 'branches': to_list(vs.Any(ZUUL_REGEX, str)), - 'files': to_list(str), + 'files': to_list(vs.Any(ZUUL_REGEX, str)), 'secrets': to_list(vs.Any(secret, str)), - 'irrelevant-files': to_list(str), + 'irrelevant-files': to_list(vs.Any(ZUUL_REGEX, str)), # validation happens in NodeSetParser 'nodeset': vs.Any(dict, str), 'timeout': int, @@ -941,9 +941,18 @@ class JobParser(object): if branches: job.setBranchMatcher(branches) if 'files' in conf: - job.setFileMatcher(as_list(conf['files'])) + with self.pcontext.confAttr(conf, 'files') as conf_files: + job.setFileMatcher([ + make_regex(x, self.pcontext) + for x in as_list(conf_files) + ]) if 'irrelevant-files' in conf: - job.setIrrelevantFileMatcher(as_list(conf['irrelevant-files'])) + with self.pcontext.confAttr(conf, + 'irrelevant-files') as conf_ifiles: + job.setIrrelevantFileMatcher([ + make_regex(x, self.pcontext) + for x in as_list(conf_ifiles) + ]) if 'failure-output' in conf: failure_output = as_list(conf['failure-output']) # Test compilation to detect errors, but the zuul_stream diff --git a/zuul/lib/re2util.py b/zuul/lib/re2util.py index c9ae281a3e..38263ab5f9 100644 --- a/zuul/lib/re2util.py +++ b/zuul/lib/re2util.py @@ -93,6 +93,14 @@ class ZuulRegex: return not self.re.search(subject) return self.re.search(subject) + def toDict(self): + # This is used in user-facing serialization, like zuul-web, to + # match job syntax. + return { + "regex": self.pattern, + "negate": self.negate, + } + def serialize(self): return { "pattern": self.pattern, diff --git a/zuul/model.py b/zuul/model.py index d546ddbea0..8d746b6d0d 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -42,7 +42,6 @@ import jsonpath_rw from zuul import change_matcher from zuul.lib.config import get_default -from zuul.lib.re2util import ZuulRegex from zuul.lib.result_data import get_artifacts_from_result_data from zuul.lib.logutil import get_annotated_logger from zuul.lib.capabilities import capabilities_registry @@ -3169,18 +3168,20 @@ class Job(ConfigObject): def setFileMatcher(self, files): # Set the file matcher to match any of the change files - self._files = files + # Input is a list of ZuulRegex objects + self._files = [x.toDict() for x in files] matchers = [] - for fn in files: - matchers.append(change_matcher.FileMatcher(ZuulRegex(fn))) + for zuul_regex in files: + matchers.append(change_matcher.FileMatcher(zuul_regex)) self.file_matcher = change_matcher.MatchAnyFiles(matchers) def setIrrelevantFileMatcher(self, irrelevant_files): # Set the irrelevant file matcher to match any of the change files - self._irrelevant_files = irrelevant_files + # Input is a list of ZuulRegex objects + self._irrelevant_files = [x.toDict() for x in irrelevant_files] matchers = [] - for fn in irrelevant_files: - matchers.append(change_matcher.FileMatcher(ZuulRegex(fn))) + for zuul_regex in irrelevant_files: + matchers.append(change_matcher.FileMatcher(zuul_regex)) self.irrelevant_file_matcher = change_matcher.MatchAllFiles(matchers) def updateVariables(self, other_vars, other_extra_vars, other_host_vars,