From e74f571085ce6910a23dc7c616cdc9adca5f1f2f Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Fri, 29 Sep 2017 15:14:31 -0700 Subject: [PATCH] Do not add implied branch matchers in project-templates We parse the project-pipeline definition of a job at the location of the project-pipeline. This includes both 'project' stanzas and 'project-templates' which are parsed in exactly the same way. This normally gives us the behavior we expect in that the job variants defined by the project or project-template appear to be defined in the location of the project or project-template. However, in one case, we want a 'late-binding' rather than 'early-binding' behavior. When it comes to calculating implied branch matchers, we want to use the value that would be derived if there were no project-template, and instead the job were simply defined on the project stanza itself. What is intended to happen is that project-pipeline job variants in a config project should never have implied branch matchers (since config projects don't have more than one branch). However, project- pipeline job variants on in-repo project stazas should get an implied branch matcher for the branch it's defined on. This is how we end up with behavior where the project definition in a project's master branch controls behavior only on the master branch (unless branches are explicitly specified), and the definition in a stable branch controls only the stable branch. That behavior should happen regardless of where a project-template is defined. Currently we are setting an implied branch matcher for job variants in a project template at the location of definition. Instead, set them when the job is actually used in a project. Change-Id: I5c8fbb3e0a2ecfac8bd95795be002e8cd15e61db --- doc/source/user/config.rst | 16 +++++- .../git/untrusted-config/zuul.d/project.yaml | 4 ++ .../zuul.d/templates.yaml | 0 .../config/templated-project/main.yaml | 1 + tests/print_layout.py | 8 +++ tests/unit/test_scheduler.py | 36 +++++++++++++ zuul/configloader.py | 52 ++++++++++++------- zuul/model.py | 24 ++++++--- 8 files changed, 112 insertions(+), 29 deletions(-) create mode 100644 tests/fixtures/config/templated-project/git/untrusted-config/zuul.d/project.yaml rename tests/fixtures/config/templated-project/git/{common-config => untrusted-config}/zuul.d/templates.yaml (100%) diff --git a/doc/source/user/config.rst b/doc/source/user/config.rst index 025ea71913..f55fb4fc4b 100644 --- a/doc/source/user/config.rst +++ b/doc/source/user/config.rst @@ -653,10 +653,22 @@ Here is an example of two job definitions: configuration, Zuul reads the ``master`` branch of a given project first, then other branches in alphabetical order. + * In the case of a job variant defined within a :ref:`project`, + if the project definition is in a :term:`config-project`, no + implied branch specifier is used. If it appears in an + :term:`untrusted-project`, with no branch specifier, the + branch containing the project definition is used as an implied + branch specifier. + + * In the case of a job variant defined within a + :ref:`project-template`, if no branch specifier appears, the + implied branch specifier for the :ref:`project` definition which + uses the project-template will be used. + * Any further job variants other than the reference definition in an untrusted-project will, if they do not have a branch - specifier, will have an implied branch specifier for the - current branch applied. + specifier, have an implied branch specifier for the current + branch applied. This allows for the very simple and expected workflow where if a project defines a job on the ``master`` branch with no branch diff --git a/tests/fixtures/config/templated-project/git/untrusted-config/zuul.d/project.yaml b/tests/fixtures/config/templated-project/git/untrusted-config/zuul.d/project.yaml new file mode 100644 index 0000000000..6d1892cac9 --- /dev/null +++ b/tests/fixtures/config/templated-project/git/untrusted-config/zuul.d/project.yaml @@ -0,0 +1,4 @@ +- project: + name: untrusted-config + templates: + - test-one-and-two diff --git a/tests/fixtures/config/templated-project/git/common-config/zuul.d/templates.yaml b/tests/fixtures/config/templated-project/git/untrusted-config/zuul.d/templates.yaml similarity index 100% rename from tests/fixtures/config/templated-project/git/common-config/zuul.d/templates.yaml rename to tests/fixtures/config/templated-project/git/untrusted-config/zuul.d/templates.yaml diff --git a/tests/fixtures/config/templated-project/main.yaml b/tests/fixtures/config/templated-project/main.yaml index e59b3964be..bb59838786 100644 --- a/tests/fixtures/config/templated-project/main.yaml +++ b/tests/fixtures/config/templated-project/main.yaml @@ -5,5 +5,6 @@ config-projects: - common-config untrusted-projects: + - untrusted-config - org/templated-project - org/layered-project diff --git a/tests/print_layout.py b/tests/print_layout.py index a295886d72..055270f512 100644 --- a/tests/print_layout.py +++ b/tests/print_layout.py @@ -59,6 +59,14 @@ def main(): if fn in ['zuul.yaml', '.zuul.yaml']: print_file('File: ' + os.path.join(gitrepo, fn), os.path.join(reporoot, fn)) + for subdir in ['.zuul.d', 'zuul.d']: + zuuld = os.path.join(reporoot, subdir) + if not os.path.exists(zuuld): + continue + filenames = os.listdir(zuuld) + for fn in filenames: + print_file('File: ' + os.path.join(gitrepo, subdir, fn), + os.path.join(zuuld, fn)) if __name__ == '__main__': diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py index f9a83ab8ed..f572a7b847 100755 --- a/tests/unit/test_scheduler.py +++ b/tests/unit/test_scheduler.py @@ -4841,6 +4841,42 @@ class TestSchedulerTemplatedProject(ZuulTestCase): self.assertEqual(self.getJobFromHistory('project-test6').result, 'SUCCESS') + def test_unimplied_branch_matchers(self): + # This tests that there are no implied branch matchers added + # by project templates. + self.create_branch('org/layered-project', 'stable') + + A = self.fake_gerrit.addFakeChange( + 'org/layered-project', 'stable', 'A') + + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(self.getJobFromHistory('project-test1').result, + 'SUCCESS') + print(self.getJobFromHistory('project-test1'). + parameters['zuul']['_inheritance_path']) + + def test_implied_branch_matchers(self): + # This tests that there is an implied branch matcher when a + # template is used on an in-repo project pipeline definition. + self.create_branch('untrusted-config', 'stable') + self.fake_gerrit.addEvent( + self.fake_gerrit.getFakeBranchCreatedEvent( + 'untrusted-config', 'stable')) + self.waitUntilSettled() + + A = self.fake_gerrit.addFakeChange( + 'untrusted-config', 'stable', 'A') + + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(self.getJobFromHistory('project-test1').result, + 'SUCCESS') + print(self.getJobFromHistory('project-test1'). + parameters['zuul']['_inheritance_path']) + class TestSchedulerSuccessURL(ZuulTestCase): tenant_config_file = 'config/success-url/main.yaml' diff --git a/zuul/configloader.py b/zuul/configloader.py index 97dfd21220..ca7e9fea49 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -456,19 +456,22 @@ class JobParser(object): # the reference definition of this job, and this is a project # repo, add an implicit branch matcher for this branch # (assuming there are no explicit branch matchers). But only - # for top-level job definitions and variants. - # Project-pipeline job variants should more closely attach to - # their branch if they appear in a project-repo. + # for top-level job definitions and variants. Never for + # project-templates. They, and in-project project-pipeline + # job variants, should more closely attach to their branch if + # they appear in a project-repo. That's handled in the + # ProjectParser. if (reference and reference.source_context and reference.source_context.branch != job.source_context.branch): - same_context = False + same_branch = False else: - same_context = True + same_branch = True if (job.source_context and (not job.source_context.trusted) and - ((not same_context) or project_pipeline)): + (not project_pipeline) and + (not same_branch)): return [job.source_context.branch] return None @@ -669,10 +672,7 @@ class JobParser(object): if (not branches) and ('branches' in conf): branches = as_list(conf['branches']) if branches: - matchers = [] - for branch in branches: - matchers.append(change_matcher.BranchMatcher(branch)) - job.branch_matcher = change_matcher.MatchAny(matchers) + job.setBranchMatcher(branches) if 'files' in conf: matchers = [] for fn in as_list(conf['files']): @@ -730,8 +730,12 @@ class ProjectTemplateParser(object): return vs.Schema(project_template) @staticmethod - def fromYaml(tenant, layout, conf): - with configuration_exceptions('project or project-template', conf): + def fromYaml(tenant, layout, conf, template): + if template: + project_or_template = 'project-template' + else: + project_or_template = 'project' + with configuration_exceptions(project_or_template, conf): ProjectTemplateParser.getSchema(layout)(conf) # Make a copy since we modify this later via pop conf = copy.deepcopy(conf) @@ -747,12 +751,13 @@ class ProjectTemplateParser(object): project_pipeline.queue_name = conf_pipeline.get('queue') ProjectTemplateParser._parseJobList( tenant, layout, conf_pipeline.get('jobs', []), - source_context, start_mark, project_pipeline.job_list) + source_context, start_mark, project_pipeline.job_list, + template) return project_template @staticmethod def _parseJobList(tenant, layout, conf, source_context, - start_mark, job_list): + start_mark, job_list, template): for conf_job in conf: if isinstance(conf_job, str): attrs = dict(name=conf_job) @@ -815,6 +820,7 @@ class ProjectParser(object): configs = [] for conf in conf_list: + implied_branch = None with configuration_exceptions('project', conf): if not conf['_source_context'].trusted: if project != conf['_source_context'].project: @@ -828,13 +834,18 @@ class ProjectParser(object): # all of the templates, including the newly parsed # one, in order. project_template = ProjectTemplateParser.fromYaml( - tenant, layout, conf) + tenant, layout, conf, template=False) + # If this project definition is in a place where it + # should get implied branch matchers, set it. + if (not conf['_source_context'].trusted): + implied_branch = conf['_source_context'].branch for name in conf_templates: if name not in layout.project_templates: raise TemplateNotFoundError(name) - configs.extend([layout.project_templates[name] + configs.extend([(layout.project_templates[name], + implied_branch) for name in conf_templates]) - configs.append(project_template) + configs.append((project_template, implied_branch)) # Set the following values to the first one that we # find and ignore subsequent settings. mode = conf.get('merge-mode') @@ -855,12 +866,13 @@ class ProjectParser(object): # For every template, iterate over the job tree and replace or # create the jobs in the final definition as needed. pipeline_defined = False - for template in configs: + for (template, implied_branch) in configs: if pipeline.name in template.pipelines: pipeline_defined = True template_pipeline = template.pipelines[pipeline.name] project_pipeline.job_list.inheritFrom( - template_pipeline.job_list) + template_pipeline.job_list, + implied_branch) if template_pipeline.queue_name: queue_name = template_pipeline.queue_name if queue_name: @@ -1520,7 +1532,7 @@ class TenantParser(object): if 'project-template' not in classes: continue layout.addProjectTemplate(ProjectTemplateParser.fromYaml( - tenant, layout, config_template)) + tenant, layout, config_template, template=True)) for config_projects in data.projects.values(): # Unlike other config classes, we expect multiple project diff --git a/zuul/model.py b/zuul/model.py index 2fa3d6ad1e..ccb0460eb8 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -23,6 +23,8 @@ from uuid import uuid4 import urllib.parse import textwrap +from zuul import change_matcher + MERGER_MERGE = 1 # "git merge" MERGER_MERGE_RESOLVE = 2 # "git merge -s resolve" MERGER_CHERRY_PICK = 3 # "git cherry-pick" @@ -901,6 +903,13 @@ class Job(object): if changed: self.roles = tuple(newroles) + def setBranchMatcher(self, branches): + # Set the branch matcher to match any of the supplied branches + matchers = [] + for branch in branches: + matchers.append(change_matcher.BranchMatcher(branch)) + self.branch_matcher = change_matcher.MatchAny(matchers) + def updateVariables(self, other_vars): v = self.variables Job._deepUpdate(v, other_vars) @@ -1028,14 +1037,14 @@ class JobList(object): else: self.jobs[job.name] = [job] - def inheritFrom(self, other): + def inheritFrom(self, other, implied_branch): for jobname, jobs in other.jobs.items(): - if jobname in self.jobs: - self.jobs[jobname].extend(jobs) - else: - # Be sure to make a copy here since this list may be - # modified. - self.jobs[jobname] = jobs[:] + joblist = self.jobs.setdefault(jobname, []) + for job in jobs: + if not job.branch_matcher and implied_branch: + job = job.copy() + job.setBranchMatcher([implied_branch]) + joblist.append(job) class JobGraph(object): @@ -2103,6 +2112,7 @@ class ProjectConfig(object): def __init__(self, name): self.name = name self.merge_mode = None + # The default branch for the project (usually master). self.default_branch = None self.pipelines = {} self.private_key_file = None