Stamp the tag or short commit in a plain docker build (closes #211)

A plain `docker build .` stamped `dev`: `.dockerignore` left out `.git`
and the Dockerfile defaulted VERSION to `dev`. `.dockerignore` is now
the canonical one plus this repo's entries, sending `.git` without
`.git/config`. Given no build arguments, the builder stamps `git
describe --tags --always` and the commit and date from git, and fails
if `.git` is present but yields no version. The empty CHECK_EPOCH
refusal is gone so the plain build succeeds; the scripts still pass an
epoch. `script/version` now prints `git describe --tags --always
--dirty`, so make, the scripts and a plain build agree.

Model: opus-5-5

cmd/vaultik/dockerversion_test.go

@@ -11,9 +11,10 @@ import (
 // This file guards the version stamping of the product image (issue
 // #75). The failure it protects against is silent: the image still
 // builds and runs, but `vaultik version` inside it reports "commit:
-// unknown", so an operator cannot tell which source produced a given
-// backup. .dockerignore excludes .git, so the build cannot derive the
-// commit itself; the values must be computed on the host and passed in.
+// unknown" or a version of "dev", so an operator cannot tell which
+// source produced a given backup. The build takes the values as build
+// args, which script/docker computes on the host, and otherwise derives
+// them from the .git in its context.
 //
 // These are parses of the committed files, for the same reason the lint
 // guards next door are: shelling out to docker would nest a build
@@ -32,35 +33,41 @@ func versionArgs() []string {
 }
 
 // TestProductDockerfileTakesVersionAsBuildArgs fails unless the build
-// declares each version arg and stamps it into the binary by ldflag
-// reference, rather than computing it in the container.
+// declares each version arg, with no default, and stamps it into the
+// binary whenever it is given, ahead of the value derived in the
+// container.
 func TestProductDockerfileTakesVersionAsBuildArgs(t *testing.T) {
 	t.Parallel()
 
 	found := instructions(t, productDockerfile)
 
 	for _, arg := range versionArgs() {
-		require.GreaterOrEqual(t, indexOf(found, "ARG "+arg), 0,
-			"%s must declare `ARG %s` so the host can pass it in",
-			productDockerfile, arg)
+		require.Contains(t, found, "ARG "+arg,
+			"%s must declare `ARG %s`, with no default, so the host can"+
+				" pass it in", productDockerfile, arg)
 
 		assertLdflagReferences(t, found, arg)
 	}
 }
 
-// TestProductDockerfileDoesNotDeriveVersionItself is the anti-regression
-// for the original defect: the container ran `git rev-parse`, but .git
-// is not in the build context, so it always resolved to "unknown". No
-// git command may reach into a build that cannot see the history.
-func TestProductDockerfileDoesNotDeriveVersionItself(t *testing.T) {
+// TestProductDockerfileDerivesVersionFromGit fails unless a build given
+// no VERSION, such as a plain `docker build .` of a clone, takes it from
+// `git describe` of the .git in its context, and fails rather than
+// stamp "dev" when that .git yields no version.
+func TestProductDockerfileDerivesVersionFromGit(t *testing.T) {
 	t.Parallel()
 
-	text := instructionText(readRepoFile(t, productDockerfile))
+	found := instructions(t, productDockerfile)
 
-	assert.NotContains(t, text, "git ",
-		"%s must not run git: .git is excluded from the build context, so"+
-			" any value it derives is wrong. Pass version, commit and date"+
-			" in as build args instead.", productDockerfile)
+	buildAt := indexContaining(found, "go build")
+	require.GreaterOrEqual(t, buildAt, 0, "%s must build", productDockerfile)
+
+	assert.Contains(t, found[buildAt], "git describe --tags --always",
+		"%s must derive the version from git when no VERSION is given",
+		productDockerfile)
+	assert.Contains(t, found[buildAt], "[ -e .git ]",
+		"%s must fail when the context carries .git but yields no version",
+		productDockerfile)
 }
 
 // TestDockerScriptComputesVersionOnTheHost fails unless script/docker
@@ -78,25 +85,25 @@ func TestDockerScriptComputesVersionOnTheHost(t *testing.T) {
 	}
 
 	assert.Contains(t, script, "/version",
-		"%s must take VERSION from script/version, the source of truth"+
-			" shared with the Makefile", dockerScript)
+		"%s must take VERSION from script/version, as the Makefile does",
+		dockerScript)
 }
 
-// assertLdflagReferences fails unless some build instruction stamps the
-// named variable from the ARG (a ${arg} reference), not from a value
-// computed inside the container.
+// assertLdflagReferences fails unless the build instruction uses the
+// named ARG whenever it is given (a ${arg:- reference), so a value
+// passed in is not overridden by one derived inside the container.
 func assertLdflagReferences(t *testing.T, found []string, arg string) {
 	t.Helper()
 
 	for _, instruction := range found {
 		if strings.HasPrefix(instruction, "RUN ") &&
 			strings.Contains(instruction, "go build") &&
-			strings.Contains(instruction, "${"+arg+"}") {
+			strings.Contains(instruction, "${"+arg+":-") {
 			return
 		}
 	}
 
 	assert.Fail(t, "version arg is declared but never stamped",
-		"the go build in %s must reference ${%s} in its ldflags, or the"+
-			" arg is passed and discarded", productDockerfile, arg)
+		"the go build in %s must use ${%s:-...}, or the arg is passed and"+
+			" discarded", productDockerfile, arg)
 }

cmd/vaultik/lintdocker_test.go

@@ -152,20 +152,21 @@ func TestLintDockerfileVerifiesTheLinterConfig(t *testing.T) {
 	assertEpochExpandedInto(t, found, verify)
 }
 
-// TestProductDockerfileCannotBeCachedGreen holds the same line for the
-// checks that remain in the product image build.
-func TestProductDockerfileCannotBeCachedGreen(t *testing.T) {
+// TestProductDockerfileKeysChecksOnTheEpoch holds the same line for the
+// checks that remain in the product image build, but without the guard:
+// a plain `docker build .` with no build arguments must succeed.
+func TestProductDockerfileKeysChecksOnTheEpoch(t *testing.T) {
 	t.Parallel()
 
 	found := instructions(t, productDockerfile)
 
 	argAt := indexOf(found, checkEpochARG)
 	require.GreaterOrEqual(t, argAt, 0,
-		"%s must declare `%s` with no default value",
-		productDockerfile, checkEpochARG)
+		"%s must declare `%s`", productDockerfile, checkEpochARG)
 
-	assert.GreaterOrEqual(t, indexOf(found, checkEpochGuard), argAt,
-		"%s must guard against an empty CHECK_EPOCH", productDockerfile)
+	assert.Equal(t, -1, indexOf(found, checkEpochGuard),
+		"%s must not refuse an empty CHECK_EPOCH: a plain `docker build .`"+
+			" must succeed", productDockerfile)
 
 	assertEpochExpandedInto(t, found[argAt:], "make fmt-check")
 	assertEpochExpandedInto(t, found[argAt:], "make test")