Merge pull request 'feat(build): pass the commit to the image as a build arg (#248)' (#277) from feat/248-commit-build-arg into main

Reviewed-on: #277

Dockerfile

@@ -23,7 +23,15 @@ COPY backend/ ./
 #
 # writeBuildInfo runs against the compiled output, so it has to follow tsc, and
 # it warns rather than fails when it finds no .git.
-RUN npm run build && node dist/writeBuildInfo.js
+# Passed in rather than discovered. Whoever builds knows the commit; the build
+# does not go looking for it, which is what broke every deploy when it did
+# (#235) and what building in CI did not fix (#237). Empty by default, so a
+# build that does not pass one behaves exactly as before — Portainer cannot
+# supply it and still deploys, which is the property that must not regress.
+#
+#   docker build --build-arg GIT_COMMIT="$(git rev-parse --short HEAD)" .
+ARG GIT_COMMIT=
+RUN GIT_COMMIT="$GIT_COMMIT" npm run build && GIT_COMMIT="$GIT_COMMIT" node dist/writeBuildInfo.js
 
 FROM node:20-bookworm-slim
 WORKDIR /app

backend/src/writeBuildInfo.ts

@@ -37,9 +37,32 @@ function findGitDir(explicit?: string): string | null {
   return candidates.find((candidate) => existsSync(candidate)) ?? null;
 }
 
-export function buildStamp(gitDir: string | null): BuildInfo {
+/**
+ * A commit passed in by whoever is building, or null.
+ *
+ * This is the half that actually works in the environments that matter. The
+ * Dockerfile deliberately does not copy `.git` — doing so broke every Portainer
+ * deploy (#235) — and #237 established that building in CI changes nothing,
+ * because the copy is what was missing rather than the history. So the builder
+ * has to hand the commit over rather than the build going to look for it (#248).
+ *
+ * Empty is treated as absent. A `--build-arg GIT_COMMIT=` with nothing after it
+ * is what an unset shell variable expands to, and stamping the image with an
+ * empty string would be worse than saying "unknown" — it reads as a commit that
+ * happens to be blank rather than as one nobody supplied.
+ */
+function passedCommit(value: string | undefined): string | null {
+  return value !== undefined && value.trim() !== '' ? value.trim() : null;
+}
+
+export function buildStamp(gitDir: string | null, passed?: string): BuildInfo {
+  const supplied = passedCommit(passed);
+
   return {
-    commit: gitDir ? resolveCommit(gitSourceAt(gitDir)) : UNKNOWN_COMMIT,
+    // The passed value wins. It is the only one available where this matters,
+    // and reading .git remains the fallback so a local build still stamps
+    // itself without anyone having to remember the argument.
+    commit: supplied ?? (gitDir ? resolveCommit(gitSourceAt(gitDir)) : UNKNOWN_COMMIT),
     // Whole seconds: this is read by a person comparing it to when they
     // pressed a button, not by anything that needs precision.
     builtAt: new Date().toISOString().replace(/\.\d{3}Z$/, 'Z')
@@ -50,15 +73,16 @@ export function buildStamp(gitDir: string | null): BuildInfo {
 // deployment — the same reasoning as backfillImageReencode.ts (#231).
 if (require.main === module) {
   const gitDir = findGitDir(process.argv[2]);
-  const stamp = buildStamp(gitDir);
+  const stamp = buildStamp(gitDir, process.env.GIT_COMMIT);
 
   if (stamp.commit === UNKNOWN_COMMIT) {
     // Loud, because a deploy that cannot say what it is defeats the point of
     // the stamp — but a warning, not a failure.
     const where = gitDir ? ` at ${gitDir}` : '';
     console.warn(
-      `[build-info] no readable .git found${where} — ` +
-        `the admin will report the commit as "${UNKNOWN_COMMIT}"`
+      `[build-info] no GIT_COMMIT passed and no readable .git found${where} — ` +
+        `the admin will report the commit as "${UNKNOWN_COMMIT}". ` +
+        `Pass --build-arg GIT_COMMIT="$(git rev-parse --short HEAD)" to stamp it.`
     );
   }
 

backend/tests/unit/buildInfo.test.ts

@@ -1,4 +1,7 @@
 import { resolveCommit, GitSource, UNKNOWN_COMMIT } from '../../src/buildInfo';
+// Safe to import: writeBuildInfo guards its side effects behind
+// require.main === module, so pulling in buildStamp cannot rewrite a stamp.
+import { buildStamp } from '../../src/writeBuildInfo';
 
 // A source with nothing in it, so each test states only the files it cares
 // about. Every field is deliberately explicit — a missing `.git` is a normal
@@ -107,3 +110,37 @@ describe('resolveCommit', () => {
     expect(resolveCommit(source({ head: '   \n' }))).toBe(UNKNOWN_COMMIT);
   });
 });
+
+describe('buildStamp with a commit passed in', () => {
+  // The half that works where it matters. The Dockerfile does not copy .git —
+  // doing so broke every Portainer deploy (#235) — and building in CI did not
+  // change that (#237), so the builder has to hand the commit over (#248).
+  it('prefers a passed commit over reading .git', () => {
+    expect(buildStamp('/nonexistent/.git', '3085970').commit).toBe('3085970');
+  });
+
+  it('uses a passed commit when there is no .git at all', () => {
+    expect(buildStamp(null, '3085970').commit).toBe('3085970');
+  });
+
+  it('trims a passed commit', () => {
+    expect(buildStamp(null, '  3085970  ').commit).toBe('3085970');
+  });
+
+  // `--build-arg GIT_COMMIT=` with an unset shell variable expands to this.
+  // Stamping an empty string would read as a commit that happens to be blank
+  // rather than as one nobody supplied.
+  it.each(['', '   '])('treats %p as no commit at all', (passed) => {
+    expect(buildStamp(null, passed).commit).toBe('unknown');
+  });
+
+  it('still says unknown when nothing is passed and there is no .git', () => {
+    expect(buildStamp(null).commit).toBe('unknown');
+  });
+
+  // The property #235 bought and that must not regress: a build with neither
+  // still produces a stamp rather than failing.
+  it('always produces a builtAt', () => {
+    expect(buildStamp(null).builtAt).toMatch(/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$/);
+  });
+});