Sprint/hardening - #1
Merged
Merged
Conversation
internal/tui has had zero test files for the whole sprint — every
round of validation-dialog scroll/cursor changes (there were several,
including one outright regression of my own in the middle of them)
was only ever checked by an isolated-build type-check plus manual
reasoning, never by a real regression test. This is the gap flagged
when asked directly what's still weak about the tool.
16 tests across two areas:
- The scroll/scrollbar math (validationScrollWindow,
renderValidationScrollbar, validationDialogContentWidth): clamping
at both ends, never exceeding the report's actual length, never
dropping below a usable minimum budget, and — directly covering
the sprint's dialogChrome bug — that accounting for the optional
banner/legend rows actually produces a smaller budget than not
accounting for them, for the same terminal height and report.
Also covers the scrollbar's full-bar-when-everything-fits behavior
and that the thumb actually reaches both ends at the two extremes
of scroll, plus a zero-visible-rows edge case that would otherwise
risk a panic.
- updateValidation's cursor behavior: Down/Up move the selection one
warning at a time and stop at the ends; Down at the last warning
falls through to plain scrolling instead of getting stuck (this
one initially caught a real bug in the test itself, not the code —
written against too generous a terminal height, the whole tiny
report already fit on screen and there was nothing below the last
warning to scroll to, so scroll legitimately staying 0 looked like
a failure until the window was shrunk enough to actually have room
below); Enter on a fixable warning opens the secret-strategy picker
with the right service/key; Enter on a non-fixable warning is a
true no-op (doesn't close the dialog); Esc closes it.
These construct Model/validationDialog values directly and call the
Update-layer methods without any bubbletea runtime — no terminal, no
program loop, just Go function calls, which is all this needed.
Verified in the same isolated-module setup used for every internal/tui
change this sprint (Go 1.22, replace directives for golang.org/x/*,
gopkg.in/yaml.v3, gopkg.in/check.v1, older API-compatible bubbletea/
bubbles/lipgloss): go build/vet clean, all 16 new tests passing, full
module test suite (pkg/composer included) still green. Please still
run the real 'go build ./... && go vet ./... && go test ./...' against
your actual go.mod before relying on this.
…t reminding
The whole point of the secrets picker is moving a hardcoded value out
of the compose file; leaving the place it landed unignored defeats
that if the person forgets the follow-up step. Previously the only
safeguard was a text reminder in the success banner ("make sure .env
is in your .gitignore") — easy to miss, and the exact kind of thing
that ends with a secret committed to history anyway.
convertSecretCmd now calls a new ensureGitignoreEntry(path, entry)
right after each disk write: ".env" for the .env strategy, "secrets/"
for the Compose-secret strategy (the whole directory, not the one file
just written, since more secrets can land there later). Creates
.gitignore if it doesn't exist; appends without duplicating if the
entry (with or without a trailing slash — "secrets" and "secrets/"
count as the same entry) is already there on its own line. A failure
to update .gitignore doesn't undo or fail the secret conversion
itself, which already succeeded — the banner just says so instead
("Couldn't update .gitignore automatically — make sure ... yourself"),
same as before for that one case.
7 new tests, using t.TempDir() rather than touching a real .gitignore:
ensureGitignoreEntry (create-if-missing, append-without-clobbering,
no duplicate on an exact match, no duplicate across the trailing-slash
variants) plus two more for appendEnvFileLine and writeSecretFile,
which had disk-IO logic of their own with no coverage either.
Verified the same way as the test-only patch just before this one on
this branch: isolated-module build/vet clean, all new tests passing,
full module suite still green. Please still run the real
'go build ./... && go vet ./... && go test ./...' against your actual
go.mod before relying on this.
README not touched here per your note that you'll handle it yourself
later.
…ecrets picker
The healthcheck warning added earlier already spells out a suggested
healthcheck in its message text, but doing anything about it meant
leaving the dialog and typing it in by hand — the secrets picker got a
one-keystroke fix a while back and this didn't, which was flagged
directly as an inconsistency.
pkg/composer additions:
- HealthCheckSuggestion{Service, Check} — the structured counterpart
to the "no healthcheck configured" warning text, same role
HardcodedSecret plays for secret warnings.
- ComposeConfig.SuggestedHealthCheck(serviceName) — exported,
service-name-keyed wrapper around the existing (unexported)
suggestedHealthCheck(image), so the TUI doesn't need to re-parse
the warning message to figure out what to offer.
- ComposeConfig.ApplyHealthCheck(serviceName, hc) — sets it, copying
hc.Test so a caller mutating their copy afterward can't reach back
and change what was actually applied.
TUI wiring: validationDialog gets a healthCheckRefs slice parallel to
warnings (same shape as secretRefs), matched in showValidation by a
direct Service+Field=="healthcheck" lookup — simpler than the
quoted-key matching secrets needed, since at most one such warning
fires per service, no ordering ambiguity to resolve. Enter on one
applies the suggestion immediately rather than opening a sub-dialog
the way secrets does — there's only one way to fix this, not three, so
a picker would just be an extra step. Unlike the secrets conversion
this has no disk IO of its own (a healthcheck is just part of the
in-memory config, written out on the next save), so no async tea.Cmd
needed — a synchronous mutation inside updateValidation's Enter case
is enough. Both fixable checks (secretRefs / healthCheckRefs) are now
bounds-checked in the render loop and Enter handler rather than bare
0-indexed lookups.
That bounds-check exists because writing the tests actually panicked
without it: dialogs_test.go's earlier updateValidation tests
constructed a validationDialog with only warnings/secretRefs set,
predating this patch, and buildValidationBodyLines' new
`m.validationDialog.healthCheckRefs[i]` lookup ran off the end of a
nil/shorter slice. Fixed at the source (bounds-check both slices
wherever they're indexed by warning position) rather than patching up
every existing test's fixture, since a real caller constructing one by
hand shouldn't be able to trigger the same panic either.
11 new tests: pkg/composer covers SuggestedHealthCheck (by name, known
image, unrecognized image, unknown service) and ApplyHealthCheck
(applies correctly, error on unknown service, copies rather than
aliases the Test slice — plus that the warning is actually gone from
Validate() after applying); internal/tui covers the Enter-applies-
immediately behavior end to end against a real ComposeConfig.
Verified in the same isolated-module setup as the rest of this branch:
go build/vet clean across the whole module, all new tests passing, the
full existing suite (90+ tests across both packages) still green after
the bounds-check fix. Please still run the real
'go build ./... && go vet ./... && go test ./...' against your actual
go.mod before relying on this.
README not touched here per your note that you'll handle it yourself
later.
pkg/composer/schema/compose-spec.json is vendored rather than fetched at runtime (see spec.go's doc comment for why), which means it goes stale silently the moment compose-spec/compose-spec publishes a schema change — nothing in the existing CI or test suite would ever notice. Flagged directly as a known gap; this closes it. .github/workflows/compose-spec-schema-check.yml runs weekly (Mondays 06:00 UTC) plus on-demand via workflow_dispatch: downloads the current schema/compose-spec.json from compose-spec/compose-spec's master branch, diffs it against the vendored copy, and if they differ, opens a tracking issue (or comments on the existing one if it's still open, rather than opening a duplicate every week) with concrete update instructions. If a later run finds the vendored copy current again, it comments and closes that issue automatically — nothing to remember to close by hand either direction. This workflow never touches the vendored file itself or fails the build; it only ever reports. No --label on the created issue: a label like "dependencies" only exists if the repo happens to have already created it, and `gh issue create` hard-fails on a label that doesn't exist rather than skipping it — not worth a scheduled workflow breaking over label bookkeeping it doesn't control. Verified everything checkable without actually running it in GitHub Actions (no way to trigger a real workflow run from here): the YAML parses (python3's yaml.safe_load), each of the three `run:` blocks extracted and checked both for bash syntax (bash -n) and with shellcheck (0.9.0) — clean on both. More usefully, ran the actual diff logic for real: fetched the live schema from raw.githubusercontent.com/compose-spec/compose-spec/master and diffed it against the vendored copy exactly the way the workflow does. It found a real, substantial difference (76668 vs 89808 bytes, well beyond whitespace) — the vendored copy is already stale, which is either a great sign this workflow is worth having or a mildly embarrassing one, possibly both. Also confirmed the not-stale path: diffing the fetched copy against itself matches, as it should. Not included here: updating the vendored schema itself to fix the staleness this just found — that's a separate, more involved change (needs the full pkg/composer/... test suite run against whatever the new schema actually changes) and out of scope for "add the CI job", which is what was actually asked for. README not touched here per your note that you'll handle it yourself later.
TestWriteSecretFile_CreatesParentDirAndWritesValue asserted info.Mode().Perm() == 0600 after writing a secret file — true on POSIX systems, but Windows/NTFS doesn't have POSIX permission bits at all: os.Stat there reports a fixed, coarse mode (essentially read-write or read-only) regardless of what was passed to WriteFile, never a specific value like 0600. Confirmed as the failure from the windows-latest leg of CI's test matrix (.github/workflows/ci.yml runs ubuntu-latest/macos-latest/windows-latest). The assertion itself was reasonable to write (0600 vs the compose file's usual 0644 is real, meaningful behavior worth covering) — it just needed to only run on platforms where the concept applies, same as any other Go test asserting POSIX permission bits. Guarded with the standard runtime.GOOS != "windows" check rather than dropping the assertion or skipping the whole test, so it still verifies the thing it's meant to verify everywhere that's actually meaningful. Verified with 'go test ./... -race -v' (matching the exact command that surfaced this) across the whole module in the same isolated-build setup used throughout this branch — Go 1.22, replace directives routing golang.org/x/*, gopkg.in/yaml.v3, and gopkg.in/check.v1 through their GitHub mirrors, since this sandbox has no route to proxy.golang.org or gopkg.in. All 100 tests across both packages pass, -race included, no data races. I obviously can't run the real Windows leg of CI myself to double-confirm this was the only failure — please push this and check that windows-latest goes green; if something else is still failing there, paste the actual error and I'll keep going.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.