Update include-vars to use the ref checkout on ref jobs

Users might well expect that if they use include-vars in a job that
runs on a ref (branch or tag), that Zuul would use the vars file in
that ref.  That was not the case, and instead Zuul would use either
an override checkout or the project default branch.  That is because
include-vars follows the same pattern as zuul roles.  Except that Zuul
roles have a different quirk: they will pick the playbook branch if
the role repo is the same as the playbook repo.  That smooths a lot
of edge cases with tags.  But that doesn't make sense for include-vars
which should not change depending on what playbook is running.

This means that it is nearly impossible to do something sensible with
include-vars on a ref job.  The only way to cause it to load the actual
values in the ref would be to set an override checkout to the ref itself
which is not practical for tag jobs.  This means most of the time tag
jobs with include-vars would end up using the project default branch
(master), which may be very different than what was expected if the
tag was made from some other branch.

To address this, the default behavior for include-vars is to use the
ref checkout if the include-vars project is the project of the triggering
ref.  This should produce the most intuitive behavior for users.  In
case the previous behavior of using floating values from master is
desirable, a new option, 'use-ref' can be set to 'false'.

Change-Id: Ifeb3b333fe6dbb021b2189d75ad710ae03c52371

doc/source/config/job.rst

@@ -1019,6 +1019,18 @@ Here is an example of two job definitions:
          This option is mutually exclusive with
          :attr:`job.include-vars.project`.
 
+      .. attr:: use-ref
+         :default: true
+
+         When this is ``true`` (the default) if the job is triggered
+         by a ref, and that ref is for the include-vars project, then
+         Zuul will checkout the ref and use the file from that ref
+         checkout.  If the include-vars is for a different project
+         than the triggering ref, or the job is not triggered by a
+         ref, or this is set to ``false``, then Zuul will follow the
+         normal fallback procedure for branches to determine from
+         which branch to load the file.
+
       An example using job-vars:
 
       .. code-block:: yaml

releasenotes/notes/include-vars-use-ref-e4620d562750e30b.yaml

@@ -0,0 +1,12 @@
+---
+upgrade:
+  - |
+    Include-vars behavior has changed when running on tags.
+    Previously, the use of :attr:`job.include-vars` in a job that ran
+    on a ref (i.e., tag or branch rather than a change) would use
+    either the override-checkout or the project default branch to
+    source the variables file.  In the case that the job is running on
+    a ref of the include-vars project, it will now source the file
+    from the ref checkout itself; this is expected to be more useful
+    and intuitive for users.  To restore the previous behavior, the
+    :attr:`job.include-vars.use-ref` may be used.

tests/fixtures/config/include-vars/git/common-config/zuul.yaml

@@ -13,6 +13,14 @@
       gerrit:
         Verified: -1
 
+- pipeline:
+    name: tag
+    manager: independent
+    trigger:
+      gerrit:
+        - event: ref-updated
+          ref: ^refs/tags/.*$
+
 - job:
     name: base
     parent: null

tests/fixtures/config/include-vars/git/org_project1/zuul.yaml

@@ -8,6 +8,12 @@
     vars:
       job_var_precedence: job-vars
 
+- job:
+    name: same-project-no-ref
+    include-vars:
+      - name: project1-vars.yaml
+        use-ref: false
+
 - job:
     name: other-project
     include-vars:
@@ -24,3 +30,11 @@
             dependencies:
               - parent-job
         - other-project
+    tag:
+      jobs:
+        - parent-job
+        - same-project:
+            dependencies:
+              - parent-job
+        - other-project
+        - same-project-no-ref

tests/fixtures/config/include-vars/git/org_project2/project2-vars.yaml

@@ -1 +1,2 @@
 project2: true
+project2_var: original

tests/unit/test_v3.py

@@ -10834,6 +10834,7 @@ class TestIncludeVars(ZuulTestCase):
         inventory = self.getBuildInventory('other-project')
         ivars = inventory['all']['vars']
         self.assertTrue(ivars['project2'])
+        self.assertEqual('original', ivars['project2_var'])
 
         self.executor_server.hold_jobs_in_build = False
         self.executor_server.release()
@@ -10899,3 +10900,91 @@ class TestIncludeVars(ZuulTestCase):
                          "A should report failure")
         self.assertEqual(A.patchsets[0]['approvals'][0]['value'], "-1")
         self.assertIn('extra keys not allowed', A.messages[0])
+
+    def test_include_vars_tag(self):
+        # Test including vars in a tag job
+        self.executor_server.hold_jobs_in_build = True
+
+        # Create a tag with the original values
+        event = self.fake_gerrit.addFakeTag('org/project1', 'master',
+                                            'foo', 'test message')
+        # Create a tag on the "other" project too
+        self.fake_gerrit.addFakeTag('org/project2', 'master',
+                                    'foo', 'test message')
+
+        # Update master with new values
+        in_repo_vars = textwrap.dedent(
+            """
+            project1: true
+            job_var_precedence: include-vars-master
+            project_var_precedence: include-vars-master
+            parent_var_precedence: include-vars-master
+            """)
+        file_dict = {'project1-vars.yaml': in_repo_vars}
+        A = self.fake_gerrit.addFakeChange('org/project1', 'master', 'A',
+                                           files=file_dict)
+        A.setMerged()
+        self.fake_gerrit.addEvent(A.getChangeMergedEvent())
+        in_repo_vars = textwrap.dedent(
+            """
+            project2: true
+            project2_var: master
+            """)
+        file_dict = {'project2-vars.yaml': in_repo_vars}
+        B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B',
+                                           files=file_dict)
+        B.setMerged()
+        self.fake_gerrit.addEvent(B.getChangeMergedEvent())
+        self.waitUntilSettled()
+
+        # Send the tag
+        self.executor_server.returnData(
+            'parent-job', 'refs/tags/foo',
+            {
+                'parent_var_precedence': 'parent-vars',
+                'parent_vars': True,
+            }
+        )
+        self.fake_gerrit.addEvent(event)
+        self.waitUntilSettled("waiting for parent job to be running")
+
+        self.executor_server.release('parent-job')
+        self.waitUntilSettled("waiting for remaining jobs to start")
+
+        inventory = self.getBuildInventory('same-project')
+        ivars = inventory['all']['vars']
+        # We read a variable from the expected file
+        self.assertTrue(ivars['project1'])
+        # Job and project vars have higher precedence than file
+        self.assertEqual('job-vars', ivars['job_var_precedence'])
+        self.assertEqual('project-vars', ivars['project_var_precedence'])
+        # Files have higher precedence than returned parent data
+        self.assertEqual('include-vars', ivars['parent_var_precedence'])
+        # Make sure we did get returned parent data
+        self.assertTrue(ivars['parent_vars'])
+
+        inventory = self.getBuildInventory('other-project')
+        ivars = inventory['all']['vars']
+        self.assertTrue(ivars['project2'])
+        # We don't check out the tag on project2 (even though it has a
+        # matching tag) because this is not an event for that tag.
+        self.assertEqual('master', ivars['project2_var'])
+
+        inventory = self.getBuildInventory('same-project-no-ref')
+        ivars = inventory['all']['vars']
+        # We read a variable from the expected file
+        self.assertTrue(ivars['project1'])
+        # Files have higher precedence than returned parent data
+        self.assertEqual('include-vars-master', ivars['parent_var_precedence'])
+
+        self.executor_server.hold_jobs_in_build = False
+        self.executor_server.release()
+        self.waitUntilSettled()
+
+        self.assertHistory([
+            dict(name='parent-job', result='SUCCESS', ref='refs/tags/foo'),
+            dict(name='same-project', result='SUCCESS', ref='refs/tags/foo'),
+            dict(name='same-project-no-ref', result='SUCCESS',
+                 ref='refs/tags/foo'),
+            dict(name='other-project', result='SUCCESS', ref='refs/tags/foo'),
+        ], ordered=False)

zuul/configloader.py

@@ -749,6 +749,7 @@ class JobParser(object):
     complex_include_vars_zuul_project_def = {
         vs.Required('name'): str,
         'zuul-project': bool,
+        'use-ref': bool,
         'required': bool,
     }
 
@@ -1166,6 +1167,7 @@ class JobParser(object):
                         iv['name'],
                         project_cn,
                         iv.get('required', True),
+                        iv.get('use-ref', True),
                     )
                     include_vars.append(job_include_vars)
                 job.include_vars = tuple(include_vars)

zuul/executor/server.py

@@ -2481,7 +2481,8 @@ class AnsibleJob(object):
         # It is neither a bare role, nor a collection of roles
         raise RoleNotFoundError("Unable to find role in %s" % (path,))
 
-    def selectBranchForProject(self, project, project_default_branch):
+    def selectBranchForProject(self, project, project_default_branch,
+                               consider_ref=None):
         # Find if the project is one of the job-specified projects.
         # If it is, we can honor the project checkout-override options.
         args = self.arguments
@@ -2491,9 +2492,19 @@ class AnsibleJob(object):
                 args_project = p
                 break
 
+        ref = None
+        if consider_ref:
+            # If this project is the Zuul project and this is a ref
+            # rather than a change, checkout the ref.
+            if (project.canonical_name ==
+                args['zuul']['project']['canonical_name'] and
+                (not args['zuul'].get('branch')) and
+                args['zuul'].get('ref')):
+                ref = args['zuul']['ref']
+
         return self.resolveBranch(
             project.canonical_name,
-            None,
+            ref,
             args['branch'],
             self.job.override_branch,
             self.job.override_checkout,
@@ -2685,7 +2696,8 @@ class AnsibleJob(object):
             project = source.getProject(iv['project'])
 
             branch, selected_desc = self.selectBranchForProject(
-                project, iv['project_default_branch'])
+                project, iv['project_default_branch'],
+                consider_ref=iv.get('use_ref', True))
             self.log.debug("Include-vars project %s using %s %s",
                            project.canonical_name, selected_desc, branch)
             if not iv['trusted']:

zuul/model.py

@@ -3628,6 +3628,7 @@ class Job(ConfigObject):
             project_default_branch=default_branch,
             trusted=trusted,
             required=include_vars.required,
+            use_ref=include_vars.use_ref,
         )
 
     def _deduplicateSecrets(self, context, frozen_playbooks):
@@ -4283,25 +4284,28 @@ class Job(ConfigObject):
 class JobIncludeVars(ConfigObject):
     """ A reference to a variables file from a job. """
 
-    def __init__(self, name, project_canonical_name, required):
+    def __init__(self, name, project_canonical_name, required, use_ref):
         super().__init__()
         self.name = name
         # The repo to look for the file in, or None for the zuul project
         self.project_canonical_name = project_canonical_name
         self.required = required
+        self.use_ref = use_ref
 
     def toDict(self):
         d = dict()
         d['name'] = self.name
         d['project_canonical_name'] = self.project_canonical_name
         d['required'] = self.required
+        d['use_ref'] = self.use_ref
         return d
 
     @classmethod
     def fromDict(cls, data):
         return cls(data['name'],
                    data['canonical_project_name'],
-                   data['required'])
+                   data['required'],
+                   data.get('use_ref', True))
 
     def __hash__(self):
         return hash(json.dumps(self.toDict(), sort_keys=True))