Skip to content

fix(docker): report the real version and tree state in published images - #1137

Merged
mbevc1 merged 4 commits into
mainfrom
20260828_docker_fix
Aug 29, 2026
Merged

mbevc1 merged 4 commits into
mainfrom
20260828_docker_fix

Conversation

@mbevc1

@mbevc1 mbevc1 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Published container images reported themselves as dev+<sha> with GitTreeState dirty, so kosli version inside a container could not name the release it was running. Two independent causes, both in the Docker build path rather than in the Go code:

  • The build context was a deny-all allow-list keeping only cmd/, internal/, Makefile, go.* and .git/. With .git/ present but 153 tracked files absent, git status --porcelain reported every excluded file as deleted and the Makefile's GIT_DIRTY resolved to "dirty". Topping the allow-list up was not an option: the tracked tree has 43 distinct top-level entries, so a correct allow-list names all 43, and any new top-level file would silently make images "dirty" again. Reversed to a deny-list of git-ignored artefacts. Measured cost: 1.10 MiB of extra context (6.17 -> 7.27 MiB of tracked files), against the 2.20 MiB .git/ the allow-list already shipped.

  • The build job checked out with fetch-depth: 3 and no fetch-tags, which per actions/checkout fetches no tags at all, so git describe --exact-match found nothing, BINARY_VERSION was empty and the version ldflag was skipped entirely, leaving the "dev" default in internal/version/version.go.

Both failures were silent: an empty BINARY_VERSION omits the ldflag rather than failing, so a release build looked identical to a dev build, and GIT_DIRTY cannot distinguish an edited file from a missing one. Nothing ever read the published image's version back. This was affecting releases since at least May 2024.

A version smoke case now asserts the image reports exactly the tag it is published under (dev+<sha> for non-release builds) from a clean tree, so neither cause can return unnoticed.

Fixes #1133

Checklist

@mbevc1 mbevc1 added the fix label Aug 28, 2026
@mbevc1
mbevc1 enabled auto-merge (squash) August 28, 2026 15:47
@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mbevc1's task in 6m 28s —— View job


Review: PR #1137 — docker version/tree-state fix

  • Gather context (diff, changed files)
  • Static analysis (go build, go vet, go mod tidy)
  • Review .dockerignore deny-list
  • Review .github/workflows/docker.yml
  • Review scripts/docker-smoke-tests.sh
  • Post review

Static analysis

Check Result
go build ./... ✅ pass
go vet ./... ✅ pass
go mod tidy + git diff --exit-code go.mod go.sum ✅ clean

No Go code changed, as expected — the whole fix is in the build path.

Verification I ran against this commit

  • No tracked file is excluded by the new deny-list. Ran git ls-files against all 27 patterns (including the ones that look risky — kosli, dist, licenses, npm/cli*/bin, tmp, TODO.md, .idea) — zero matches. npm/wrapper/bin/kosli and bin/ survive because dockerignore patterns are root-anchored full-path matches, not gitignore basename matches. So the dirty-from-phantom-deletions cause is genuinely closed.
  • .gitignore.dockerignore mirror is complete — every one of the 27 .gitignore entries has a counterpart.
  • .gitmodules is empty, so no submodule-gitlink surprise from shipping the full tree.
  • version --short really is the right assertion target: formatVersionversion.GetVersion()version + "+" + metadata; the Makefile clears VERSION_METADATA iff GIT_TAG is non-empty, so a tagged build prints exactly v2.x.y and an untagged one prints dev+<short-sha>. The EXPECTED_VERSION / ^dev\+[0-9a-f]{7,}$ split matches that exactly.
  • persist-credentials: false is safe on both checkouts. I walked every subsequent step in build (buildx → login → build → digest → upload) and merge (setup-go → download-artifact → buildx → login → setup-kosli-cli → imagetools → attest → sbom → snyk → 3× kosli attest → smoke tests): none uses git over HTTP. kosli attest artifact reads remote.origin.url locally, which persist-credentials: false leaves intact — it only drops the http.*.extraheader. fetch-tags: true still works, since the credential is used during checkout.
  • Only docker.yml uses the repo root as a build contextdocker-compose.yml builds from ./mongo and ./minio, .goreleaser.yml has no dockers: block, and Dockerfile.alpine consumes the published image. So widening the context has no other blast radius.

On your reply — "Would miss the original failure"

Agreed, and it's the right call. Deriving EXPECTED_VERSION from the merge job's own git describe would make the verifier use the same mechanism as the thing under test: if tag resolution broke again, merge would compute "expect dev+<sha>", the image would report dev+<sha>, and the test would pass on a broken release. Keeping the expectation anchored to inputs.tag — a source independent of git metadata — is what gives the test its teeth. Withdrawing that earlier suggestion; and merge correctly does not get fetch-tags: true.

Findings

1. Release path still leans on tag auto-following — the caller can make it deterministic (medium) — inline

release.yml:116-125 calls this workflow without checkout_ref, so build checks out github.sha at --depth=3 and the tag reaches .git/refs/tags only via git's auto-following. Passing checkout_ref: ${{ github.ref }} (refs/tags/vX.Y.Z) makes actions/checkout fetch that tag ref explicitly — same commit, no behavioural change for merge or the kosli attest steps, but git describe --exact-match can no longer come up empty inside the builder. Keep fetch-tags: true here for main.yml's sha path either way.

2. The arm64 image is never version-checked (medium, unresolved from the previous round)

test_version runs only in merge on ubuntu-latest, and docker run against a multi-arch manifest resolves to the runner's own platform — so it always exercises amd64. The arm64 leg is built on ubuntu-24.04-arm from an independent actions/checkout. If fetch-tags behaves differently there, or that workspace picks up a stray file, arm64 ships dev+<sha> / dirty while this step stays green — the same bug class, on the leg the test doesn't see. version is a static Go binary and the case is two docker runs, so QEMU is cheap here: add docker/setup-qemu-action to merge, pass PLATFORMS: ${{ inputs.platforms }}, and loop test_version over ${PLATFORMS//,/ } with --platform (defaulting to native when unset, so the script still runs standalone). Fix this →

3. Nothing enforces the .gitignore.dockerignore sync (low, unresolved)

The header says "keep the two in sync" and today they are, but only a human maintains that. A new .gitignore line that isn't mirrored reintroduces #1133 for whoever generates that artefact, and the first signal is a published image. A pr-checks.yml step that fails when a .gitignore line has no counterpart would make this file self-maintaining. Fix this →

4. Re-run false-failure (low, accepted risk)

Re-running a main.yml docker job for a commit that has since been tagged now finds the tag (it didn't before this PR), so the image reports v2.x.y while EXPECTED_VERSION is unset and the test demands dev+<sha>. Rare, fails loudly rather than silently, and — per your point above — not worth trading the test's independence to fix. An explicit expected_version workflow input, set from release.yml and empty from main.yml, would close it without that trade-off, but it's cosmetic. The ^v[0-9] regex itself is safe: main.yml's tag is 8 hex chars, which can never match.

5. Nit: commit="$(git … rev-parse HEAD)" (scripts/docker-smoke-tests.sh:77) could move above the first docker run, so a mismatched-workspace failure reports before you pay for a container start.

What's good here

  • The root-cause write-up is exemplary — two independent causes, both silent (an empty BINARY_VERSION omits the ldflag rather than failing; GIT_DIRTY can't tell an edited file from a missing one), with the measured 1.10 MiB context cost stated rather than hand-waved. The "43 top-level entries" argument for why topping up the allow-list wasn't viable is the right call.
  • Inverting to a deny-list is the correct structural fix: the allow-list failed open (a new top-level file silently makes images dirty), the deny-list fails closed.
  • The .dockerignore header earns its keep — # Careful: bin/ and npm/wrapper/bin/kosli are tracked is exactly the trap the next person would fall into.
  • Including smoke-test-results.json pre-empts the "untracked file makes the tree dirty" case the header warns about — the merge job writes it straight into $GITHUB_WORKSPACE.
  • The GitCommit assertion is the one that closes the stale-.git-in-context angle, which clean + dev+<sha> alone couldn't catch.
  • persist-credentials: false is a real security improvement beyond the stated scope: .git/config's checkout token was being baked into the builder layer and exported to a public mode=max registry cache. Pre-existing, now gone.
  • set -uo pipefail with local declared separately from assignment throughout — $? and || return 1 both behave correctly, which is easy to get wrong in this style.

Net: the fix is correct and well-verified for the amd64 path. The arm64 blind spot (2) is the one I'd want closed before this is the thing you rely on to catch a regression.
· branch 20260828_docker_fix

Comment thread .dockerignore
Comment thread .dockerignore
Comment thread scripts/docker-smoke-tests.sh
Comment thread .github/workflows/docker.yml
Comment thread .dockerignore
Comment thread .github/workflows/docker.yml
Comment thread .github/workflows/docker.yml
Comment thread scripts/docker-smoke-tests.sh
Comment thread .github/workflows/docker.yml
Comment thread .github/workflows/docker.yml
Comment thread .dockerignore
@mbevc1
mbevc1 force-pushed the 20260828_docker_fix branch 2 times, most recently from 50bc4c5 to 2cf8f84 Compare August 28, 2026 18:37
Comment thread .github/workflows/docker.yml
mbevc1 added 4 commits August 28, 2026 22:36
Published container images reported themselves as `dev+<sha>` with
GitTreeState `dirty`, so `kosli version` inside a container could not name
the release it was running. Two independent causes, both in the Docker build
path rather than in the Go code:

- The build context was a deny-all allow-list keeping only cmd/, internal/,
  Makefile, go.* and .git/. With .git/ present but 153 tracked files absent,
  `git status --porcelain` reported every excluded file as deleted and the
  Makefile's GIT_DIRTY resolved to "dirty". Topping the allow-list up was not
  an option: the tracked tree has 43 distinct top-level entries, so a correct
  allow-list names all 43, and any new top-level file would silently make
  images "dirty" again. Reversed to a deny-list of git-ignored artefacts.
  Measured cost: 1.10 MiB of extra context (6.17 -> 7.27 MiB of tracked
  files), against the 2.20 MiB .git/ the allow-list already shipped.

- The build job checked out with fetch-depth: 3 and no fetch-tags, which per
  actions/checkout fetches no tags at all, so `git describe --exact-match`
  found nothing, BINARY_VERSION was empty and the version ldflag was skipped
  entirely, leaving the "dev" default in internal/version/version.go.

Both failures were silent: an empty `BINARY_VERSION` omits the ldflag rather
than failing, so a release build looked identical to a dev build, and
`GIT_DIRTY` cannot distinguish an edited file from a missing one. Nothing ever
read the published image's version back. This was affecting releases since at
least May 2024.

A `version` smoke case now asserts the image reports exactly the tag it is
published under (`dev+<sha>` for non-release builds) from a clean tree, so
neither cause can return unnoticed.

Fixes #1133
Review follow-ups:

- `.dockerignore` was missing six `.gitignore` entries: `/reporter`, `/merkely`,
  `kosli.yaml`, `merkely.yaml`, `pipe.json` and `*~`. The two yaml files can hold an
  API token, and `COPY . .` would carry them into the builder stage on a local
  build. None is tracked at any depth, and no tracked file matches any of the
  27 resulting patterns.

- `test_version` asserted Version and GitTreeState but not GitCommit. The merge
  job checks out the same ref as the build job, so HEAD is the built commit and
  the assertion costs nothing. It closes the case where the context is wholly
  an older commit, which is self-consistent and would otherwise read as clean.
With the allow-list gone, an untracked file left in the workspace reaches the
build context and reads as dirty, alongside the excluded-tracked-path case the
header already described. Comment only.
actions/checkout leaves an x-access-token extraheader in .git/config. The build
job copies .git/ into the build context and cache-to mode=max exports every
layer to the public buildcache ref; the merge job mounts its workspace into the
smoke-test container. Nothing in either job talks to git over HTTP after
checkout. Pre-existing, not introduced by the .dockerignore change.
@mbevc1
mbevc1 force-pushed the 20260828_docker_fix branch from 2cf8f84 to 71d952f Compare August 28, 2026 21:36
@mbevc1

mbevc1 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Done. persist-credentials: false on both docker checkouts (2cf8f84). Real
and free: .git/ ships in the build context and cache-to mode=max exports
every layer to the public buildcache ref, and the merge job mounts its
workspace into the smoke-test container. Nothing in either job talks to git
over HTTP after checkout. As you say, pre-existing rather than introduced here.

Correction accepted on the drift point. .gitignore.dockerignore drift can't reintroduce #1133: .gitignore is
tracked, isn't excluded by the deny-list, and so ships in the context, where
git status --porcelain still honours it inside the builder. The only hard
rule this file carries is never list a tracked path; drift's real costs are
context size and a local kosli.yaml token riding into the exported cache —
which is why the six entries were worth adding on their own merit. I've left
the header as is rather than growing it: its first sentence already states the
hard rule, and the wrong inference to guard against is one a reader would draw
in a review thread, not from this file.

Not taking: arm64. The version mechanic is arch-independent

Deferred: an explicit expected_version input. Agreed it's cleaner than
pattern-matching the tag, and it would incidentally fix the re-run case. But
that case needs someone to re-run a stale main.yml docker job for a commit
tagged since, and it fails loudly with GitCommit pinning identity regardless.
Happy to do it if you'd rather have it now.

Comment thread .github/workflows/docker.yml
@mbevc1
mbevc1 merged commit a769c43 into main Aug 29, 2026
22 checks passed
@mbevc1
mbevc1 deleted the 20260828_docker_fix branch August 29, 2026 06:16
mbevc1 pushed a commit that referenced this pull request Aug 29, 2026
… tags

The docker image derived its version from `git describe` inside the build,
while the smoke test inferred the expected version from the image tag. Those
two disagree whenever a release tag is cut at the same commit as an in-flight
push-to-main pipeline: the build picks up the new tag and reports v<x.y.z>,
the test still expects dev+<sha>, and the version smoke test fails.

Resolve the version once in the `prepare` job and feed it to both sides — the
build bakes it in as a VERSION build arg, the smoke test asserts the image
reports it. This is what #1133 originally suggested; #1137 closed that issue
with `fetch-tags: true` instead, which is what let a stray tag reach the
build. The build job no longer fetches tags, so the image is a function of
the build request rather than of whichever tags the checkout happens to have
by the time it runs.

The Makefile needs `origin` rather than `ifdef` to tell "VERSION not passed"
(local build, fall back to the tag at HEAD) from "VERSION passed empty" (CI,
not a release). Metadata clearing moves from GIT_TAG to BINARY_VERSION so an
explicit version is self-consistent: `make build VERSION=v2.39.0` on an
untagged tree previously produced v2.39.0+<sha>, matching neither goreleaser
nor the smoke test.

`make docker` passes the tag at HEAD so a local image build at a release tag
still reports it; a bare `docker build .` reports dev+<sha>.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XqXZamA5LHB3b3hGbBEig1
mbevc1 added a commit that referenced this pull request Aug 29, 2026
… tags

The docker image derived its version from `git describe` inside the build,
while the smoke test inferred the expected version from the image tag. Those
two disagree whenever a release tag is cut at the same commit as an in-flight
push-to-main pipeline: the build picks up the new tag and reports v<x.y.z>,
the test still expects dev+<sha>, and the version smoke test fails.

Resolve the version once in the `prepare` job and feed it to both sides — the
build bakes it in as a VERSION build arg, the smoke test asserts the image
reports it. This is what #1133 originally suggested; #1137 closed that issue
with `fetch-tags: true` instead, which is what let a stray tag reach the
build. The build job no longer fetches tags, so the image is a function of
the build request rather than of whichever tags the checkout happens to have
by the time it runs.

The Makefile needs `origin` rather than `ifdef` to tell "VERSION not passed"
(local build, fall back to the tag at HEAD) from "VERSION passed empty" (CI,
not a release). Metadata clearing moves from GIT_TAG to BINARY_VERSION so an
explicit version is self-consistent: `make build VERSION=v2.39.0` on an
untagged tree previously produced v2.39.0+<sha>, matching neither goreleaser
nor the smoke test.

`make docker` passes the tag at HEAD so a local image build at a release tag
still reports it; a bare `docker build .` reports dev+<sha>.
mbevc1 added a commit that referenced this pull request Aug 29, 2026
… tags

The docker image derived its version from `git describe` inside the build,
while the smoke test inferred the expected version from the image tag. Those
two disagree whenever a release tag is cut at the same commit as an in-flight
push-to-main pipeline: the build picks up the new tag and reports v<x.y.z>,
the test still expects dev+<sha>, and the version smoke test fails.

Resolve the version once in the `prepare` job and feed it to both sides — the
build bakes it in as a VERSION build arg, the smoke test asserts the image
reports it. This is what #1133 originally suggested; #1137 closed that issue
with `fetch-tags: true` instead, which is what let a stray tag reach the
build. The build job no longer fetches tags, so the image is a function of
the build request rather than of whichever tags the checkout happens to have
by the time it runs.

The Makefile needs `origin` rather than `ifdef` to tell "VERSION not passed"
(local build, fall back to the tag at HEAD) from "VERSION passed empty" (CI,
not a release). Metadata clearing moves from GIT_TAG to BINARY_VERSION so an
explicit version is self-consistent: `make build VERSION=v2.39.0` on an
untagged tree previously produced v2.39.0+<sha>, matching neither goreleaser
nor the smoke test.

`make docker` passes the tag at HEAD so a local image build at a release tag
still reports it; a bare `docker build .` reports dev+<sha>.
mbevc1 added a commit that referenced this pull request Aug 30, 2026
… tags (#1140)

* fix(docker): build the image version from the workflow input, not git tags

The docker image derived its version from `git describe` inside the build,
while the smoke test inferred the expected version from the image tag. Those
two disagree whenever a release tag is cut at the same commit as an in-flight
push-to-main pipeline: the build picks up the new tag and reports v<x.y.z>,
the test still expects dev+<sha>, and the version smoke test fails.

Resolve the version once in the `prepare` job and feed it to both sides — the
build bakes it in as a VERSION build arg, the smoke test asserts the image
reports it. This is what #1133 originally suggested; #1137 closed that issue
with `fetch-tags: true` instead, which is what let a stray tag reach the
build. The build job no longer fetches tags, so the image is a function of
the build request rather than of whichever tags the checkout happens to have
by the time it runs.

The Makefile needs `origin` rather than `ifdef` to tell "VERSION not passed"
(local build, fall back to the tag at HEAD) from "VERSION passed empty" (CI,
not a release). Metadata clearing moves from GIT_TAG to BINARY_VERSION so an
explicit version is self-consistent: `make build VERSION=v2.39.0` on an
untagged tree previously produced v2.39.0+<sha>, matching neither goreleaser
nor the smoke test.

`make docker` passes the tag at HEAD so a local image build at a release tag
still reports it; a bare `docker build .` reports dev+<sha>.

* fix(docker): harden the version classifier against silent mis-stamping

Address review feedback on the version build-arg change.

The Makefile now honours VERSION only as a command-line assignment. `origin`
reports `environment` for an inherited variable, so an ambient VERSION in the
caller's shell beat the tag at HEAD — and with metadata cleared it stamped a
fake clean release. The Dockerfile passes VERSION on the command line, so the
one caller that needs the override still has it.

The tag classifier in `prepare` is anchored at both ends: the value reaches
single-quoted ldflags, so a tag carrying a quote or whitespace is no longer
treated as a release.

Build arg and smoke-test expectation come from one workflow output, so nothing
was checking the mapping itself — a misclassification would make both halves
agree on the wrong version and pass. The smoke test now re-derives the release
case from the image tag: a tag matching the looser ^v[0-9] must carry its own
version, so a tag the strict classifier rejects fails loudly instead of being
published as dev+<sha>.

* fix(docker): close both nits in the version classifier

`origin` returns "environment override" when the makefile assigns VERSION and
`-e` makes the environment win. `filter` matched the trailing `override` and
treated that as a command-line assignment, re-opening the hole the comment
above it claims to close. `filter-out` keys on the leftover word instead, so
only a genuine command-line or override-directive assignment counts. The
Makefile does not assign VERSION today, so this is not currently reachable —
it stops a later `VERSION ?=` default from quietly restoring the bug.

The smoke-test tripwire only checked one direction: a release tag with a
wrong or empty version. The mirror case — a sha tag carrying a release
version, which would publish a dev image reporting v<x.y.z> — passed. Derive
the expectation from the image tag once and compare, covering both.

* fix(docker): report the published tag as the image version

Drop the release classifier. The image now reports whatever tag it is
published under — a release version from release.yml, a short sha from
main.yml — so the build arg and the smoke-test expectation are the same
expression and cannot drift. That removes the prepare output, both tag
regexes and the smoke-test tripwire they needed.

Non-release images now report the sha instead of dev+<sha>, so the update
check keys on the v prefix a release always has rather than on the "dev"
string that no longer appears. It skips for dev builds, dev+<sha> and shas
like, and now does so before the HTTP call rather than after it.

EXPECTED_VERSION is always set by CI, so the smoke test requires it and
asserts equality; its dev+<sha> branch is gone.

* fix(docker): mark non-release images as dev+<tag>

Resolve the version once in prepare rather than passing the tag straight
through: a v-prefixed tag is reported as-is, anything else becomes
dev+<tag>. Both the build arg and the smoke-test expectation read that one
output, so the baked version and the asserted one still cannot drift.

Branch images therefore keep reporting dev+<sha> exactly as before, which
makes the update check's existing "dev" prefix test correct again — so the
v-prefix change to update_check.go and its test are reverted.

The match is deliberately loose. This repo cuts -rc tags, which the stricter
^v[0-9]+\.[0-9]+\.[0-9]+$ used in install-script-tests.yml would misread as
dev builds.
dangrondahl added a commit to kosli-dev/docs that referenced this pull request Sep 7, 2026
## Summary

Adds the changelog entry for CLI **v2.39.2**, plus the Docker image
version fix from v2.39.0 that was missing.

Rebased on `main` after #376 merged. #376 had already documented v2.39.0
and v2.39.1, so this PR is now purely additive — no deletions.

## Conflict resolution

Four conflicts in `changelog/index.mdx`, resolved as follows:

- **v2.39.2** — new, kept as generated. Placed above the August 31
Platform entry, keeping the file newest-first.
- **v2.39.0 and v2.39.1 prose** — kept the versions already on `main`
from #376 and discarded this branch's rewordings, since they had been
through review and use the correct command name (see below).
- **The August 24–28 Platform entries** — kept `main`'s; this branch
predated them.

## The Docker version fix

This branch carried a Docker version bullet on both v2.39.0 and v2.39.1,
and `main` had neither. Checking upstream, only one of them is a
user-facing fix:

- **v2.39.0** ([cli#1137](kosli-dev/cli#1137)) —
the real fix. Published images identified themselves as `dev+<sha>`
instead of their release tag, so `kosli version` in a container could
not name the release. Two causes in the Docker build path, both silent.
Any image published before v2.39.0 is affected. **Kept**, reworded
around that symptom.
- **v2.39.1** ([cli#1140](kosli-dev/cli#1140)) —
not a user-facing fix. It resolves the version once in the workflow
instead of via `git describe` in the build, superseding cli#1137's
`fetch-tags` approach, and fixes a smoke-test flake when a release tag
is cut at the same commit as an in-flight main pipeline. Its own
description says images "report what they did before … except the racy
case". **Dropped** as an internal build change.

That also removes the problem of two consecutive releases each claiming
to fix the image version with different causes.

Both bullets appeared only in the releases' "What's Changed" lists
rather than their curated sections, which is likely why the first pass
missed them.

## One correction worth noting

This branch described the v2.39.1 help-text change as affecting `kosli
snapshot azure-apps`. That name comes from the upstream release note,
but the command is `kosli snapshot azure` — the generated reference page
is `client_reference/kosli_snapshot_azure.md` and there is no
`azure-apps` page. `main`'s wording is correct and links the reference;
that is what was kept.

## Checks

- `mint broken-links` — clean apart from the pre-existing
`/getting_started/service-accounts` break in
`tutorials/working_with_controls.mdx`, which is on `main` and untouched
here.
- `<Update>` open/close tags balance at 117 each; no empty headings;
entries remain in newest-first date order.

---------

Co-authored-by: mintlify[bot] <109931778+mintlify[bot]@users.noreply.github.com>
Co-authored-by: Dan Grøndahl <dan@kosli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Published container images report their version as dev, and their tree state as dirty

2 participants