From 2e530f803b20885096233a15d275168eff0c2b8c Mon Sep 17 00:00:00 2001
From: Maximilian Bosch <mb@flyingcircus.io>
Date: Thu, 21 Nov 2024 11:38:43 +0100
Subject: [PATCH] jenkins: always resolve tags to rev of the commit that got
 tagged

FC-41962
Closes #200

Some tags have a their own revision which is not the revision of the
commit that got tagged. This happens e.g. for tags with messages or
signatures.

Previously, we tried to resolve the latter (that's what the `^{}`-syntax
is for) and if this "fails" (i.e. stdout is empty, but the exit-code is
zero), we made a fallback to `git ls-remote url tag`.

This however had the unintended side-effect that an annotated tag was
resolved to the tag's revision (fallback) if the first call failed due
to a transient error like a DNS outage. In some cases this broke
subsequent deployment steps if the S3 URL for frontend artifacts
contains the commit revision (and not the tag's revision).

This patch explicitly checks for errors in `git ls-remote` (if something
isn't found, we get zero as exit-code and no stdout) and fail hard.
Also, the `ls-remote` without the `^{}` is done first. This avoids a
second `ls-remote` call for refs that aren't tags.
---
 ...41121_122503_mb_FC_41962_tag_resolution.md |  3 ++
 src/batou_ext/jenkins.py                      | 45 +++++++++++++------
 2 files changed, 35 insertions(+), 13 deletions(-)
 create mode 100644 CHANGES.d/20241121_122503_mb_FC_41962_tag_resolution.md

diff --git a/CHANGES.d/20241121_122503_mb_FC_41962_tag_resolution.md b/CHANGES.d/20241121_122503_mb_FC_41962_tag_resolution.md
new file mode 100644
index 0000000..2581abf
--- /dev/null
+++ b/CHANGES.d/20241121_122503_mb_FC_41962_tag_resolution.md
@@ -0,0 +1,3 @@
+- Correctness fix for `jenkins set-version`: if a tag is resolved, make sure it's _always_
+  resolved to the rev of the tagged commit (instead of the tag's rev) or fail hard to avoid
+  incorrect revs.
diff --git a/src/batou_ext/jenkins.py b/src/batou_ext/jenkins.py
index af00f27..ef53622 100644
--- a/src/batou_ext/jenkins.py
+++ b/src/batou_ext/jenkins.py
@@ -7,6 +7,19 @@
 import sys
 
 
+def git_ls_remote(url, ref):
+    cmd = subprocess.Popen(
+        ["git", "ls-remote", url, ref], stdout=subprocess.PIPE
+    )
+    stdout, _ = cmd.communicate()
+    if cmd.returncode != 0:
+        raise ValueError(
+            f"'git ls-remote {url} {ref}' failed with exit code {cmd.returncode}. Stderr is printed above."
+        )
+    if stdout:
+        return stdout.decode("ascii").split("\t", 1)
+
+
 def git_resolve(url, version):
     if len(version) == 40:
         # revision.
@@ -16,19 +29,25 @@ def git_resolve(url, version):
             pass
         else:
             return version
-    # Symbolic name?
-    cmd = subprocess.Popen(
-        ["git", "ls-remote", url, version + "^{}"], stdout=subprocess.PIPE
-    )
-    stdout, stderr = cmd.communicate()
-    # if its not a tag, start another more generic attempt
-    if not stdout:
-        cmd = subprocess.Popen(
-            ["git", "ls-remote", url, version], stdout=subprocess.PIPE
-        )
-        stdout, stderr = cmd.communicate()
-    stdout = stdout.decode("ascii")
-    return stdout.split("\t", 1)[0]
+
+    ls_remote = git_ls_remote(url, version)
+    if ls_remote:
+        rev, full_refspec = ls_remote
+        # If full_refspec indicates that a tag was found, retry with `^{}`
+        # to check if the tag is annotated: an annotated tag (e.g. due to its
+        # own message or a signature) has its own revision which is not equal
+        # to the revision of the commit that got tagged.
+        # The difference is crucial in some cases however, e.g. if the commit's
+        # rev is part of the S3 URL containing the frontend artifacts for this
+        # deploy.
+        if full_refspec.startswith("refs/tags/"):
+            ls_remote_tag = git_ls_remote(url, f"{version}^{{}}")
+            # If the result is empty here, the tag is not annotated, thus using the
+            # rev from the first invocation is correct. We don't end up here because of
+            # an error anymore since non-zero returns now fail with a ValueError earlier.
+            if ls_remote_tag:
+                rev = ls_remote_tag[0]
+        return rev
 
 
 class VersionsUpdater: