From 9504e96b5df4bd26d87c36681fecca5b27872a9d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Herv=C3=A9=20Beraud?= <hberaud@redhat.com>
Date: Tue, 15 Jun 2021 14:22:49 +0200
Subject: [PATCH] Fix validation to ensure that first release of series isn't a
 bugfix

We recently observed that this check was skipped. Indeed it was
possible to propose a bugfix version on the first release of a series
without facing validation errors [1].

The described situation was allowed by checks that we introduced
during Wallaby [2]. Those was designed to ensure that we can
build sdist from the proposed tag. Indeed, they temporarly created
the new git tag to build sdist from it. However we never delete this
tag after our tests so this temporary tag is see as an existing tag
and so that led us to the observed bug.

Our validation check that the first version number of series isn't a
bugfix after it tried to build the sdist from the new tag, so, without
this patch the tag newly created by the sdist check remains available
in the repo, and so a couple of next checks are skipped, including the
validation of the version number, because these checks are decorated
to ignore existing tags.

Indeed the function decorated with `skip_existing_tags` detect this
temporary tag as an existing tag, so, the decorated functions are
skipped.

We should notice that it surely impacted other checks in the same manner.
Indeed, all validation functions decorated with this function
`skip_existing_tags` are impacted by this bug.
Hopefully it didn't have too much side effects.

These changes delete the temporary tag after usage to ensure to restore
the initial environment and to stop skipping next checks.

[1] https://review.opendev.org/c/openstack/releases/+/795836
[2] https://opendev.org/openstack/releases/commit/80652b523214ab7215928368246fbf726b78a645

Change-Id: I5d9024528d08487a7582e0a0e73576b22708a6cf
---
 openstack_releases/cmds/validate.py       |  9 +++++++
 openstack_releases/gitutils.py            | 15 +++++++++++
 openstack_releases/tests/test_validate.py | 31 +++++++++++++++++++++++
 3 files changed, 55 insertions(+)

diff --git a/openstack_releases/cmds/validate.py b/openstack_releases/cmds/validate.py
index b9db450078..0e7d6745b3 100644
--- a/openstack_releases/cmds/validate.py
+++ b/openstack_releases/cmds/validate.py
@@ -836,6 +836,15 @@ def validate_build_sdist(deliv, context):
             context.error(
                 'Failed to build sdist for {}: {}'.format(
                     project.repo.name, err))
+        finally:
+            # Removing the temporary tag to avoid to mislead next tests, else
+            # if not deleted the tag will remain active in the repo and it
+            # will be see as an existing tag.
+            gitutils.delete_tag(
+                context.workdir,
+                project.repo.name,
+                release.version
+            )
 
 
 @skip_em_eol_tags
diff --git a/openstack_releases/gitutils.py b/openstack_releases/gitutils.py
index afc6f01568..da2606014a 100644
--- a/openstack_releases/gitutils.py
+++ b/openstack_releases/gitutils.py
@@ -355,6 +355,21 @@ def add_tag(workdir, repo, tag, sha):
         return None
 
 
+def delete_tag(workdir, repo, tag):
+    cmd = ['git', 'tag', '-d', tag]
+    try:
+        LOG.info(' '.join(cmd))
+        return processutils.check_output(
+            cmd,
+            cwd=os.path.join(workdir, repo),
+            stderr=subprocess.STDOUT,
+        ).decode('utf-8').strip()
+    except processutils.CalledProcessError as e:
+        LOG.warning('failed to delete tag: %s [%s]',
+                    e, e.output.strip())
+        return None
+
+
 def get_branches(workdir, repo):
     try:
         output = processutils.check_output(
diff --git a/openstack_releases/tests/test_validate.py b/openstack_releases/tests/test_validate.py
index 6c646df67f..df1b4737fa 100644
--- a/openstack_releases/tests/test_validate.py
+++ b/openstack_releases/tests/test_validate.py
@@ -2710,6 +2710,37 @@ class TestValidateSeriesFirst(base.BaseTestCase):
         self.assertEqual(0, len(self.ctx.warnings))
         self.assertEqual(0, len(self.ctx.errors))
 
+    def test_version_ko(self):
+        series_a_dir = self.tmpdir + '/a'
+        series_a_filename = series_a_dir + '/automaton.yaml'
+        os.makedirs(series_a_dir)
+        deliverable_data = textwrap.dedent('''
+        ---
+        releases:
+          - version: 1.1.1
+            projects:
+              - repo: openstack/automaton
+                hash: be2885f544637e6ee6139df7dc7bf937925804dd
+        ''')
+        with open(series_a_filename, 'w') as f:
+            f.write(deliverable_data)
+        deliv = deliverable.Deliverable(
+            team='team',
+            series='a',
+            name='name',
+            data=yamlutils.loads(deliverable_data),
+        )
+        validate.validate_series_first(
+            deliv,
+            self.ctx,
+        )
+        self.ctx.show_summary()
+        self.assertEqual(0, len(self.ctx.warnings))
+        # Starting a cycle with a bugfix version (1.1.1) isn't allowed,
+        # so, in this case our validation should fail accordingly to that
+        # rule.
+        self.assertEqual(1, len(self.ctx.errors))
+
     def test_ignore_if_second_release(self):
         series_a_dir = self.tmpdir + '/a'
         series_a_filename = series_a_dir + '/automaton.yaml'