update exists check to look for a commit to be on a named branch

All patches pushed to gerrit are mirrored and then visible through cgit,
so using that web service to check for a "valid" patch does not do what
we thought. This patch changes the logic to look for any named branch to
contain the patch, meaning it has been merged into a branch already
somewhere.

In the course of working on the tests, I also discovered that the one
verifying ancestry of patches being tagged as part of a series was using
the stable/mitaka branch in automaton, which is no EOL. I rewrote the
test to try to be more future-proof.

Change-Id: I2a1c797de4d61649dd047aca28a241c8901e18f3
Signed-off-by: Doug Hellmann <doug@doughellmann.com>

openstack_releases/cmds/validate.py

@@ -354,10 +354,23 @@ def validate_releases(deliverable_info, zuul_layout,
                          }
                 )
             else:
+
+                # Ensure we have a local copy of the repository so we
+                # can scan for values that are more difficult to get
+                # remotely.
+                try:
+                    gitutils.clone_repo(workdir, project['repo'], project['hash'])
+                except Exception as err:
+                    mk_error('Could not clone repository %s at %s: %s' % (
+                        project['repo'], project['hash'], err))
+                    # No point in running extra checks if we can't
+                    # clone the repository.
+                    continue
+
                 # Report if the SHA exists or not (an error if it
                 # does not).
                 sha_exists = gitutils.commit_exists(
-                    project['repo'], project['hash'],
+                    workdir, project['repo'], project['hash'],
                 )
                 if not sha_exists:
                     mk_error('No commit %(hash)r in %(repo)r'
@@ -366,11 +379,6 @@ def validate_releases(deliverable_info, zuul_layout,
                     # doesn't exist.
                     continue
 
-                # Ensure we have a local copy of the repository so we
-                # can scan for values that are more difficult to get
-                # remotely.
-                gitutils.clone_repo(workdir, project['repo'], project['hash'])
-
                 version_exists = gitutils.tag_exists(
                     project['repo'], release['version'],
                 )
@@ -591,7 +599,8 @@ def validate_branch_prefixes(deliverable_info, mk_waring, mk_error):
                 branch['name'], _VALID_BRANCH_PREFIXES))
 
 
-def validate_stable_branches(deliverable_info, series_name,
+def validate_stable_branches(deliverable_info, workdir,
+                             series_name,
                              mk_warning, mk_error):
     "Apply the rules for stable branches."
     if ('launchpad' in deliverable_info and
@@ -652,7 +661,7 @@ def validate_stable_branches(deliverable_info, series_name,
                          'like a SHA' % (
                              (loc, repo, branch['name'])))
                     )
-                if not gitutils.commit_exists(repo, loc):
+                if not gitutils.commit_exists(workdir, repo, loc):
                     mk_error(
                         ('stable branches should be created from merged '
                          'commits but location %s for branch %s of %s '
@@ -709,7 +718,7 @@ def validate_feature_branches(deliverable_info, workdir, mk_warning, mk_error):
                      'like a SHA' % (
                          (loc, repo, branch['name'])))
                 )
-            if not gitutils.commit_exists(repo, loc):
+            if not gitutils.commit_exists(workdir, repo, loc):
                 mk_error(
                     ('feature branches should be created from merged commits '
                      'but location %s for branch %s of %s does not exist' % (
@@ -757,7 +766,7 @@ def validate_driverfixes_branches(deliverable_info, workdir, mk_warning, mk_erro
                      'like a SHA' % (
                          (loc, repo, branch['name'])))
                 )
-            if not gitutils.commit_exists(repo, loc):
+            if not gitutils.commit_exists(workdir, repo, loc):
                 mk_error(
                     ('driverfixes branches should be created from merged commits '
                      'but location %s for branch %s of %s does not exist' % (
@@ -846,6 +855,10 @@ def main():
         validate_release_notes(deliverable_info, mk_warning, mk_error)
         validate_type(deliverable_info, mk_warning, mk_error)
         validate_model(deliverable_info, series_name, mk_warning, mk_error)
+        # NOTE(dhellmann): A side-effect of validate_releases() is
+        # that all of the repos mentioned in the deliverable file are
+        # cloned. No validation that needs the repo to be checked out
+        # locally should happen before validate_releases() is called.
         validate_releases(
             deliverable_info,
             zuul_layout,
@@ -883,6 +896,7 @@ def main():
         )
         validate_stable_branches(
             deliverable_info,
+            workdir,
             series_name,
             mk_warning,
             mk_error,

openstack_releases/gitutils.py

@@ -40,16 +40,13 @@ def find_modified_deliverable_files():
     return filenames
 
 
-def commit_exists(repo, ref):
+def commit_exists(workdir, repo, ref):
     """Return boolean specifying whether the reference exists in the repository.
 
-    Uses a cgit query instead of looking locally to avoid cloning a
-    repository or having Depends-On settings in a commit message allow
-    someone to fool the check.
+    The commit must exist on a named branch.
 
     """
-    url = CGIT_SHA_TEMPLATE % (repo, ref)
-    return links.link_exists(url)
+    return bool(branches_containing(workdir, repo, ref))
 
 
 def tag_exists(repo, ref):

openstack_releases/tests/test_validate.py

@@ -15,6 +15,7 @@
 from __future__ import unicode_literals
 
 import os
+import subprocess
 import textwrap
 
 import fixtures
@@ -22,6 +23,7 @@ import mock
 from oslotest import base
 
 from openstack_releases import defaults
+from openstack_releases import gitutils
 from openstack_releases.cmds import validate
 from openstack_releases import yamlutils
 
@@ -414,6 +416,7 @@ class TestValidateReleases(base.BaseTestCase):
     def setUp(self):
         super(TestValidateReleases, self).setUp()
         self.tmpdir = self.useFixture(fixtures.TempDir()).path
+        gitutils.clone_repo(self.tmpdir, 'openstack/automaton')
 
     @mock.patch('openstack_releases.project_config.require_release_jobs_for_repo')
     def test_check_release_jobs(self, check_jobs):
@@ -642,19 +645,54 @@ class TestValidateReleases(base.BaseTestCase):
         self.assertEqual(0, len(errors))
 
     def test_not_descendent(self):
+        # INFO(dhellmann): Unfortunately this test only works using
+        # named branches, and the available set of branches changes
+        # over time as we mark some EOL. So, start by getting a list
+        # of branches and then pick some commits that are on different
+        # branches for the test.
+        known_branches = gitutils.get_branches(
+            self.tmpdir,
+            'openstack/automaton',
+        )
+        stable_branches = list(sorted(
+            b
+            for b in known_branches
+            if b.startswith('stable/')
+        ))
+        # Pick an early branch, find the most recent tagged release,
+        # get the version, then get the sha of that commit.
+        branch1 = stable_branches[0]
+        series = branch1.rpartition('/')[-1]
+        version1 = subprocess.check_output(
+            ['git', 'describe', '--abbrev=0', branch1],
+            cwd=self.tmpdir + '/openstack/automaton',
+        ).decode('utf-8').strip()
+        commit1 = gitutils.sha_for_tag(
+            self.tmpdir,
+            'openstack/automaton',
+            version1,
+        )
+        # Pick a later branch and take the most recent commit. Make up
+        # a later verison by adding to the other version string.
+        branch2 = stable_branches[1]
+        commit2 = gitutils.sha_for_tag(
+            self.tmpdir,
+            'openstack/automaton',
+            branch2,
+        )
+        version2 = version1 + '9'
         deliverable_info = {
             'artifact-link-mode': 'none',
             'releases': [
-                {'version': '1.4.0',
+                {'version': version1,
                  'projects': [
                      {'repo': 'openstack/automaton',
-                      'hash': 'c6278ba1a8167447a5f52bdb92c2790abc5d0f87'},
+                      'hash': commit1},
                  ]},
-                {'version': '1.4.999',
+                {'version': version2,
                  'projects': [
                      {'repo': 'openstack/automaton',
-                      # a commit on stable/mitaka instead of stable/newton
-                      'hash': 'e92b85ec64ac74598983a90bd2f3e1cf232ba9d5'},
+                      'hash': commit2},
                  ]},
             ],
         }
@@ -663,11 +701,12 @@ class TestValidateReleases(base.BaseTestCase):
         validate.validate_releases(
             deliverable_info,
             {'validate-projects-by-name': {}},
-            'ocata',
+            series,
             self.tmpdir,
             warnings.append,
             errors.append,
         )
+        print(warnings, errors)
         self.assertEqual(0, len(warnings))
         self.assertEqual(2, len(errors))
 
@@ -1141,6 +1180,11 @@ class TestValidateBranchPrefixes(base.BaseTestCase):
 
 class TestValidateStableBranches(base.BaseTestCase):
 
+    def setUp(self):
+        super(TestValidateStableBranches, self).setUp()
+        self.tmpdir = self.useFixture(fixtures.TempDir()).path
+        gitutils.clone_repo(self.tmpdir, 'openstack/automaton')
+
     def test_version_in_deliverable(self):
         deliverable_data = textwrap.dedent('''
         releases:
@@ -1157,6 +1201,7 @@ class TestValidateStableBranches(base.BaseTestCase):
         deliverable_info = yamlutils.loads(deliverable_data)
         validate.validate_stable_branches(
             deliverable_info,
+            self.tmpdir,
             'ocata',
             warnings.append,
             errors.append,
@@ -1180,6 +1225,7 @@ class TestValidateStableBranches(base.BaseTestCase):
         deliverable_info = yamlutils.loads(deliverable_data)
         validate.validate_stable_branches(
             deliverable_info,
+            self.tmpdir,
             'ocata',
             warnings.append,
             errors.append,
@@ -1203,6 +1249,7 @@ class TestValidateStableBranches(base.BaseTestCase):
         deliverable_info = yamlutils.loads(deliverable_data)
         validate.validate_stable_branches(
             deliverable_info,
+            self.tmpdir,
             'ocata',
             warnings.append,
             errors.append,
@@ -1226,6 +1273,7 @@ class TestValidateStableBranches(base.BaseTestCase):
         deliverable_info = yamlutils.loads(deliverable_data)
         validate.validate_stable_branches(
             deliverable_info,
+            self.tmpdir,
             'ocata',
             warnings.append,
             errors.append,
@@ -1250,6 +1298,7 @@ class TestValidateStableBranches(base.BaseTestCase):
         deliverable_info = yamlutils.loads(deliverable_data)
         validate.validate_stable_branches(
             deliverable_info,
+            self.tmpdir,
             '_independent',
             warnings.append,
             errors.append,
@@ -1275,6 +1324,7 @@ class TestValidateStableBranches(base.BaseTestCase):
         deliverable_info = yamlutils.loads(deliverable_data)
         validate.validate_stable_branches(
             deliverable_info,
+            self.tmpdir,
             '_independent',
             warnings.append,
             errors.append,
@@ -1299,6 +1349,7 @@ class TestValidateStableBranches(base.BaseTestCase):
         deliverable_info = yamlutils.loads(deliverable_data)
         validate.validate_stable_branches(
             deliverable_info,
+            self.tmpdir,
             'ocata',
             warnings.append,
             errors.append,
@@ -1323,6 +1374,7 @@ class TestValidateStableBranches(base.BaseTestCase):
         deliverable_info = yamlutils.loads(deliverable_data)
         validate.validate_stable_branches(
             deliverable_info,
+            self.tmpdir,
             'ocata',
             warnings.append,
             errors.append,
@@ -1347,6 +1399,7 @@ class TestValidateStableBranches(base.BaseTestCase):
         deliverable_info = yamlutils.loads(deliverable_data)
         validate.validate_stable_branches(
             deliverable_info,
+            self.tmpdir,
             'ocata',
             warnings.append,
             errors.append,
@@ -1372,6 +1425,7 @@ class TestValidateStableBranches(base.BaseTestCase):
         deliverable_info = yamlutils.loads(deliverable_data)
         validate.validate_stable_branches(
             deliverable_info,
+            self.tmpdir,
             'ocata',
             warnings.append,
             errors.append,
@@ -1397,6 +1451,7 @@ class TestValidateStableBranches(base.BaseTestCase):
         deliverable_info = yamlutils.loads(deliverable_data)
         validate.validate_stable_branches(
             deliverable_info,
+            self.tmpdir,
             'ocata',
             warnings.append,
             errors.append,
@@ -1410,6 +1465,7 @@ class TestValidateFeatureBranches(base.BaseTestCase):
     def setUp(self):
         super(TestValidateFeatureBranches, self).setUp()
         self.tmpdir = self.useFixture(fixtures.TempDir()).path
+        gitutils.clone_repo(self.tmpdir, 'openstack/automaton')
 
     def test_location_not_a_dict(self):
         deliverable_data = textwrap.dedent('''
@@ -1541,6 +1597,7 @@ class TestValidateDriverfixesBranches(base.BaseTestCase):
     def setUp(self):
         super(TestValidateDriverfixesBranches, self).setUp()
         self.tmpdir = self.useFixture(fixtures.TempDir()).path
+        gitutils.clone_repo(self.tmpdir, 'openstack/automaton')
 
     def test_unknown_series(self):
         deliverable_data = textwrap.dedent('''