Skip to content

refactor(ci): make docker image smoke tests table-driven - #1111

Merged
sami-alajrami merged 13 commits into
mainfrom
docker-smoke-test-refactor
Aug 20, 2026
Merged

refactor(ci): make docker image smoke tests table-driven#1111
sami-alajrami merged 13 commits into
mainfrom
docker-smoke-test-refactor

Conversation

@sami-alajrami

@sami-alajrami sami-alajrami commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Extracts the docker image smoke tests (previously one hand-written CI step pair per test) into scripts/docker-smoke-tests.sh, a table-driven runner where a new check is a test_* function plus one run_case line.
  • Adds a smoke test for attest artifact on a directory artifact, alongside the existing list environments connectivity check.
  • Kosli attestation reporting (kosli attest generic) stays visible in the workflow YAML, driven off a JSON results file the script writes.

Postmortem follow-up from kosli-dev/server#6508.

Test plan

  • make lint
  • CI run of .github/workflows/docker.yml on this branch passes, including the new attest artifact dir smoke test
  • Confirm smoke-test attestation is reported to Kosli when report_to_kosli != none

🤖 Generated with Claude Code

Extract docker image smoke tests into scripts/docker-smoke-tests.sh so
new checks can be added as a function + one run_case call instead of
a new workflow step pair each time. Also adds a smoke test for the
`attest artifact` dir command. Kosli attestation reporting stays in
the workflow, driven off the script's JSON results file.

Postmortem follow-up from kosli-dev/server#6508.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @sami-alajrami's task in 4m 26s —— View job


Review of docker-smoke-test-refactor (latest push: 14e7b806)

  • Gather context (diff, prior review threads)
  • Static analysis
  • Review scripts/docker-smoke-tests.sh
  • Review .github/workflows/docker.yml
  • Post findings

Static analysis

Check Result
go build ./... pass
go vet ./... pass
go mod tidy diff n/a — no Go files changed
Script executable bit 100755
shellcheck not installed on the runner, and not wired into CI — reviewed by hand instead

Findings

1. Removing list-environments drops the only check that the shipped image can reach Kosliinline

Dockerfile:23-25 is FROM scratch plus a single COPY of ca-certificates.crt. A scratch image has no trust store of its own, so if that COPY ever breaks, every TLS call from the released image fails — and nothing left in this suite would notice, because the sole remaining case is --dry-run and never opens a socket. The attestation is still named smoke-test but now attests strictly less than it does on main.

@pbeckham's "don't depend on the production web service" is the right instinct; removing the case is one way to satisfy it, but not the only one. Recording the case's outcome without letting it flip EXIT_CODE, or pointing it at a non-prod host, both keep the CA-bundle coverage without a prod blip failing the release. Your call — flagging it because it's a deliberate coverage trade-off, not an oversight.

2. requests.go:254 is already off by oneinline

The banner is logged at requests.go:255. This is @pbeckham's stale-line-number point demonstrating itself before merge; the surrounding prose works fine without the pointer.

3. The --api-token rationale was trimmed away, and the token now looks redundantinline

PreRunE validates the token before --dry-run can short-circuit, so -e KOSLI_API_TOKEN is load-bearing. That exact removal broke CI once already (ddd7df3f) and now sits next to --dry-run with no explanation. Suggestion also fixes the missing space after # on line 64.


What's good here

The hardening across this PR's review rounds landed well, and the end state is genuinely hard to fool:

  • Seed-then-flush (lines 101-112) is the right shape. Writing every case as not-run up front means a partially-executed run produces a file that reads as incomplete rather than as a clean pass — the postmortem's failure mode, closed properly rather than papered over.
  • Asserting the dry-run banner is present (line 90) instead of asserting an error string is absent. Fails closed on a reword, a crash, or an early exit; the negative form silently fails open on all three.
  • Every jq and every write has an explicit || exit — the correct discipline given set -e is deliberately off so all cases run.
  • RESULTS_FILE hoisted to job-level env, removing the rename-desync hazard between the two steps.
  • if: ${{ success() || failure() }} on the smoke step so a Snyk finding no longer masks the functional checks as a smoke-test failure.

EXIT_CODE accumulation is correctly done in the parent shell rather than a subshell, and local status=$? is properly split from the local output declaration — the usual exit-status-masking trap is avoided.

Findings 2 and 3 are small. Finding 1 is a scope decision worth a deliberate answer before merge.
· branch docker-smoke-test-refactor

sami-alajrami and others added 2 commits August 20, 2026 10:17
Report one smoke-tests attestation instead of one per test case, with
the compliance verdict derived from whether all cases passed, and the
per-test results attached as evidence.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread .github/workflows/docker.yml Outdated
Comment thread scripts/docker-smoke-tests.sh Outdated
Comment thread scripts/docker-smoke-tests.sh Outdated
Comment thread scripts/docker-smoke-tests.sh Outdated
Comment thread scripts/docker-smoke-tests.sh Outdated
Comment thread scripts/docker-smoke-tests.sh
Comment thread .github/workflows/docker.yml Outdated
Comment thread .github/workflows/docker.yml Outdated
Comment thread .github/workflows/docker.yml Outdated
Apply the outstanding review findings on the docker smoke tests
refactor:

- Report step now falls back to a failing result and rejects a vacuous
  empty results file, so a script crash before it writes results.json
  can no longer produce a missing or falsely-compliant attestation —
  the exact failure class this refactor is a postmortem for.
- REPO_ROOT falls back from GITHUB_WORKSPACE and hard-fails if empty,
  instead of silently mounting an empty path into the container.
- jq failures during result accumulation now abort instead of writing
  an empty, non-JSON results file; results are flushed after every
  case so a hung/cancelled run still reports what already passed.
- Renamed run_case labels (list-environments, attest-artifact-dir) so
  they read as case labels and don't collide with the aggregate
  attestation's name; updated the stale comment describing them.
- attest-artifact-dir case now uses the real HEAD sha for --commit-url
  and drops the redundant KOSLI_API_TOKEN=DRY_RUN (the --dry-run flag
  alone is sufficient), exercising the real input path more honestly.
- Dropped the now-unused step id, ignored the local results file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread .github/workflows/docker.yml
- Run the smoke tests step regardless of upstream job status
  (success() || failure()), matching the Snyk/report steps' own
  guards. Previously a Snyk finding (non-zero exit, no
  continue-on-error) would skip the smoke tests step entirely, and
  the reporting step's new "no results" fallback would then report a
  false non-compliant smoke-test attestation for tests that never ran.
- Hoist RESULTS_FILE to a job-level env on merge: instead of declaring
  it identically on both steps, so a rename is a one-line change.
- Guard the results-file write in the script with || exit, matching
  the jq call above it, so a failed write isn't silently swallowed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread scripts/docker-smoke-tests.sh Outdated
Comment thread scripts/docker-smoke-tests.sh Outdated
sami-alajrami and others added 2 commits August 20, 2026 11:01
Seed every case as "not-run" in the results file before any case
executes, and have run_case update entries in place instead of
appending. Previously an abort after a passing case (jq failure, a
step timeout on a hung docker run, a future test_* that kills the
script instead of returning non-zero) left a results file that was
non-empty and all-success, so the aggregate attestation went green for
a run that never finished — the same failure shape as the postmortem
this PR follows up on. The workflow's `all(.outcome == "success")`
check needs no change, since "not-run" already isn't "success".

Also: guard the git rev-parse for commit_sha with || return 1 (was
silently producing a malformed but non-empty --commit-url on failure),
and drop the now-provably-unused KOSLI_API_TOKEN from the
attest-artifact-dir case, since --dry-run short-circuits before any
request needs it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread scripts/docker-smoke-tests.sh Outdated
sami-alajrami and others added 2 commits August 20, 2026 11:24
CI failed with "--api-token is not set" for this case. --api-token is
a required flag checked before --dry-run's own request short-circuit,
so dropping it (done in a prior commit on the mistaken belief that
--dry-run made the token unused) broke the command outright.

Worse, main.go turns any command error into a logged warning plus exit
0 when --dry-run is set, so the missing-token failure was silently
reported as this smoke test passing, without ever reaching the
fingerprinting/git-resolution code this case exists to exercise. Adds
an explicit check for that warning string so a future required-flag
regression fails the smoke test instead of being swallowed by
--dry-run's exit-0 behaviour.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread scripts/docker-smoke-tests.sh Outdated
Comment thread scripts/docker-smoke-tests.sh Outdated
Comment thread scripts/docker-smoke-tests.sh
The previous guard asserted the *absence* of the "--dry-run is enabled"
warning, which fails open: reword that message in main.go, or die
before reaching it at all, and the negation makes the assertion true
again — a green result for a case that never did its work.

Assert the dry-run banner is present instead. requests.go:254 only
prints it once the request is built, which is downstream of dir
fingerprinting, git resolution and payload assembly, so it is positive
evidence this case exercised what it exists to exercise, and it fails
closed. Verified both directions locally: with a token the banner
appears and the payload carries real git info; without one the command
still exits 0 (swallowed by --dry-run) but the banner is absent and the
case correctly fails.

Also restores the note that --api-token is required by PreRunE even
under --dry-run, since that is what a cleanup pass removed last time.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread scripts/docker-smoke-tests.sh
Comment thread scripts/docker-smoke-tests.sh
@sami-alajrami
sami-alajrami enabled auto-merge (squash) August 20, 2026 11:19
Comment thread scripts/docker-smoke-tests.sh Outdated
Comment thread scripts/docker-smoke-tests.sh Outdated
Comment thread scripts/docker-smoke-tests.sh
Comment thread scripts/docker-smoke-tests.sh
Comment thread scripts/docker-smoke-tests.sh
@sami-alajrami
sami-alajrami merged commit 2ef5ab0 into main Aug 20, 2026
37 of 40 checks passed
@sami-alajrami
sami-alajrami deleted the docker-smoke-test-refactor branch August 20, 2026 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants