From 7f265fd70ec8d30f5a0edd1c9d183711f139f7df 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

Previously, we always resolved tags to the commit rev. However if this
failed, there was a fallback to the rev of the tag (the output of `git
ls-remote` without the `^{}` modifier).

This is a problem in a deployment where we rely on the rev being the
commit rev since that's part of the S3 url where the frontend assets are
stored. The reason for the fallback that lead to such a deployment
error was a timing issue where the first `ls-remote` failed like this

    ssh: Could not resolve hostname gitlab.customer.com: Try again
    fatal: Could not read from remote repository.
    Please make sure you have the correct access rights
    and the repository exists.

and the second one (resolving to the tag's rev) succeeded.

We now call `ls-remote` without the `^{}` modifier and check if the
refspec got resolved to a tag (i.e. `refs/tags/...`). If that's the
case, a second `ls-remote` is issued with the `^{}` modifier to
get the rev of the commit that got tagged. If this second call fails,
we fail hard to make sure we don't get incorrect results silently.
---
 ...41121_122503_mb_FC_41962_tag_resolution.md |  3 ++
 src/batou_ext/jenkins.py                      | 37 ++++++++++++-------
 2 files changed, 27 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..57bbed0 100644
--- a/src/batou_ext/jenkins.py
+++ b/src/batou_ext/jenkins.py
@@ -7,6 +7,15 @@
 import sys
 
 
+def git_ls_remote(url, ref):
+    cmd = subprocess.Popen(
+        ["git", "ls-remote", url, ref], stdout=subprocess.PIPE
+    )
+    stdout, _ = cmd.communicate()
+    if stdout:
+        return stdout.decode("ascii").split("\t", 1)
+
+
 def git_resolve(url, version):
     if len(version) == 40:
         # revision.
@@ -16,19 +25,21 @@ 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 make sure we get the rev of the commit instead of the tag's rev.
+        if full_refspec.startswith("refs/tags"):
+            ls_remote_tag = git_ls_remote(url, f"{version}^{{}}")
+            if ls_remote_tag:
+                rev = ls_remote_tag[0]
+            else:
+                raise ValueError(
+                    f"git ls-remote {url} {version}^{{}} failed, see above for the error."
+                )
+        return rev
 
 
 class VersionsUpdater: