Skip to content

chore: conform to the documented conventions - #486

Open
retr0h wants to merge 31 commits into
mainfrom
chore/conform-to-contributing
Open

chore: conform to the documented conventions#486
retr0h wants to merge 31 commits into
mainfrom
chore/conform-to-contributing

Conversation

@retr0h

@retr0h retr0h commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Brings the repository in line with CONTRIBUTING.md, in one pass.

What the audit found

  • Licence header. Four files had none; eight darwin files separated the
    build constraint from the header with a // line instead of a blank one.
  • Signatures. 488 multi-line signatures across 421 files did not put one
    parameter per line with the closing paren and return types on their own.
  • Logging. One logger.Error passed a positional pair where the
    conventions ask for a typed slog.Any.
  • Pointer helpers. Twenty local ptrTo-style helpers, with 315 call
    sites, duplicated k8s.io/utils/ptr.To.
  • Test helper files. Seven support_public_test.go files held only those
    helpers and went away with them; the rest are named for what they hold.
  • Tables. 1,036 of 1,298 tables asserted in the loop rather than in the
    row. They now carry validateFunc: the loop makes the call, the row says
    what it expects.

Tables

1,286 of 1,298 tables carry validateFunc. The twelve that do not start a
server or a goroutine and return nothing — there is no result to hand over,
and the mock expectations do the checking. gohai and nats-server have the
same handful for the same reason.

One commit here reverts another: test: drop the tables that had nothing to table removed 37 one-row tables, which no other repository in the
organisation does, so it is reverted rather than dropped from the history.

test: restore the assertions the handler tables had lost fixes a defect the
conversion itself introduced: moving wantContains into the row flattened
[]string{"error", drain initiated} down to a single unquoted
s.Contains(rec.Body.String(), "error"). Both the quotes and the second
string are back, taken from the rows as they stood on main. Every raw
string literal on main is present again in the file that held it, and the
only interpreted literals gone are three fmt imports that a simplification
made unnecessary.

Two notes

  • just ready fails on main as well: go generate regenerates against
    oapi-codegen v2.7.1, which the committed *.gen.go files predate. Not
    touched here.
  • go.mod moves more than k8s.io/utils. Every fmt, lint and coverage
    recipe in the shared go module runs go get -tool, so the tool pins
    follow whatever was current when the recipes last ran.

Verification

just go-fmt, just go-vet (0 issues) and just test (coverage 100.0%,
target 99.9%) after every commit.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP

retr0h and others added 25 commits September 7, 2026 18:49
CONTRIBUTING requires the MIT header on every .go file. Four had none:
three in the Docker container provider and one in telemetry's process
conditions.

Eight more carried the header but separated the copyright line from it
with a bare // rather than a blank line, which is how the other twelve
hundred files in the repository write it. They are the darwin files
under the ping and netplan DNS providers.

The logging rules ask for typed attributes; one call in the audit export
command passed a positional pair, which compiles but bypasses the type
safety the rule is there for.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
The conventions ask for the multi-line form: one parameter per line,
with the closing parenthesis and return types on a line of their own, so
adding a parameter shows as one added line rather than a rewritten
signature.

Four hundred and eighty-eight declarations were written on one line.
This rewrites them; no signature changes, only how it is spelled.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
The public suites send raw HTTP through the Echo stack and then checked
the recorder in the loop: a status code from one column and a list of
substrings from another, iterated over.

The row says what it expects of the response now, and the loop hands the
recorder over. Two hundred and two tables across a hundred and two files.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
The agent processors and the job client branched on an expectError
column, compared the message against another column, and then called an
optional validate the row might not supply. A row supplying none checked
only that the result was non-nil.

Each row states what it expects of the result and the error. Where the
inlined body declared its own err for a later decode, that one is named
decodeErr so it no longer shadows the callback's parameter.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
The gate is stated as a percentage and the file declaring the target is
named, but not the file narrowing what the percentage covers.
.coverignore excludes cmd, gen, mocks, ui and main.go, so a reading taken
without it does not match what the gate measures.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
Thirty-nine files held tables whose loop finished by comparing a want
column against the call. The comparison is the assertion, so it moves
into the row.

A nil written in one of those columns is emitted as the column's typed
zero. testify compares types as well as values, and a bare nil against a
*bool reads as unequal; two rows in the agent handler table showed that
before the types were carried through.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
The platform stubs assert that every Darwin method returns
ErrUnsupported, which the rules ask for by name. The row said only its
own name and the loop held the whole expectation; the row states it now.

The CLI tables assert through the package-level assert with suite.T()
rather than the suite's own methods, which the earlier pass did not
read. Same conversion, same shape.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
Twenty-nine more files of the same shape. Some assert only that the
result is nil and the error is ErrUnsupported, without the separate
Error call the earlier pass required, which is why they were left
behind.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
validation's Struct and Var tables read an ok column and then, only when
the call failed, iterated a contains column. Each row states both.

The job subject label table branched on wantErr for the same two-line
choice.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
Twenty declarations of boolPtr, strPtr, intPtr and intervalPtr sat
across sixteen files, all the same two lines, feeding three hundred and
fifteen call sites. Generics made that one function years ago and the
ecosystem settled on ptr.To, so the copies go and the calls read
ptr.To(v).

Seven support_public_test.go files held nothing else and are deleted
rather than renamed. The remaining three are named for what they hold:
sha256_public_test.go for the digest helper, test_agent_public_test.go
for the agent builder its eight callers share, and the job client's mock
setup folded into the one file that used it.

File naming said a test file is named for the production file it tests,
which says nothing about a test file holding no tests. That absence is
what let support, fixture and mock_support all appear; the rule now
covers it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
Missed in the previous pass. It holds newTestAuditHandler, which three
of the audit test files share.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
Fifty more files. The loops branched on wantErr, expectError or
expectErr and held both arms; each row now carries the arm it takes,
with the row's own literals substituted in.

The stub tables that assert Empty rather than Nil are converted too;
the earlier pass only read the Nil form.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
Some stubs assert Equal(0, got) rather than Nil or Empty; the matcher
now keeps whatever the loop asserted about the result.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
The general case: everything after the call is what the row expects, and
the columns only that part reads are expectations rather than inputs.
Those move into the row with their literals substituted; columns the
setup also reads stay where they are.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
Twenty-eight files carried the row's assertion in a column called
validate. It is the same thing under a different name, so it takes the
one the conventions use.

Where the loop guarded the call, the guard is gone with it: a row that
supplied no callback asserted nothing and passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
Thirty-seven tables declared one column, name, and held one row. The
loop ran once and the row varied nothing, so the table said only what
the sub-test name already said.

The conventions ask for table-driven cases because rows are where
success, errors and edge cases live. A table of one is ceremony around a
single test, and bolting a callback onto it would only add more. The
sub-test keeps its name and the loop goes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
The loop was asserting on what the call returned, so the expectation sat
in the loop rather than in the row it belonged to. The rows now carry a
validateFunc and the loop only makes the call.

Three files spelled the column validateFn or validateResult; the
convention names it validateFunc.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
Where the loop called and then asserted, the assertion belonged to the
row: it is the row's expectation, not the loop's. The rows now carry a
validateFunc holding what the loop used to assert, with the row's own
values written in, so the guards that selected between them are gone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
The loop served the request and then asserted the status the row had
named. Serving is the call; the status is the row's expectation, so the
recorder now goes to the row's validateFunc.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
The same move as the previous commits, in the tables that reach for the
assert and require packages rather than the suite's own methods.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
These rows named a whole struct or a built error as the thing they
expected, so the expectation moves into the row's validateFunc with it.
An error built only to have Error() called on it is now the string.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
The validation loops chose which call to make from the row and then
asserted for it; choosing stays in the loop, the assertion moves to the
row. The job tables follow the same move.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
These loops assert and then tear down, so only the assertions move; the
teardown stays. The config tables spelled the column wantCheck, which is
the same idea under another name.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
The converters treated a range clause as a call and a redeclared err as
a borrowed variable, so these tables were passed over. They follow the
same move as the rest.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
retr0h and others added 6 commits September 7, 2026 19:05
Moving wantContains into the row flattened it: a row that named
[]string{`"error"`, `drain initiated`} came out asserting only that the
body contained error, without the quotes and without the second string.
Both are back, taken from the rows as they stood before the move.

Every raw string literal on main is again present in the file that held
it, and the only interpreted literals gone are three fmt imports the
simplification made unnecessary.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
The middleware loops built a flag and a recorder and then asserted on
them; both now go to the row. The processor tables carried an optional
validate callback, which is the same thing under another name, so it is
no longer optional and no longer needs the branch that guarded it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
The remaining tables the converters could take: the agent constructor,
the heartbeat time, the compact table's headers, and the readiness
body. What is left asserts nothing about a result — it starts a server
or a goroutine and lets the mock expectations do the checking.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
Where the loop cannot give up the call — it needs the server again to
stop it, or what it checks is a value the call left behind — the call
stays and the variables go to the row instead.

The signing table is written out by hand: its error row returned early,
so the rows that do not error carry the envelope checks the return used
to skip.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
The seed and template rows named what they expected and the loop read
it back; the rows now hold the assertion and are handed the error, the
result and the filesystem the loop built. HandleError does the same
with the log it captured and the exit code it recorded.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
Two tables asserted that a call does not panic. The call is what the
loop makes; not panicking is what the row expects, so the row takes the
call and says so.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CM5qrNtYQvdKcemkS5YpiP
@retr0h
retr0h force-pushed the chore/conform-to-contributing branch from b91993f to d15d50e Compare September 8, 2026 04:45
@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #486      +/-   ##
==========================================
- Coverage   99.94%   99.94%   -0.01%     
==========================================
  Files         484      484              
  Lines       22877    22876       -1     
==========================================
- Hits        22865    22864       -1     
  Misses         12       12              
Files with missing lines Coverage Δ
internal/controller/api/file/file_stale.go 100.00% <ø> (ø)
internal/controller/api/job/job_get.go 100.00% <100.00%> (ø)
internal/facts/keys.go 100.00% <ø> (ø)
internal/provider/container/docker/docker.go 100.00% <ø> (ø)
internal/provider/network/netplan/dns/darwin.go 100.00% <ø> (ø)
...netplan/dns/darwin_get_by_interface_resolv_conf.go 100.00% <ø> (ø)
...plan/dns/darwin_update_resolv_conf_by_interface.go 100.00% <ø> (ø)
internal/provider/network/ping/darwin.go 100.00% <ø> (ø)
internal/provider/network/ping/darwin_do.go 100.00% <ø> (ø)

... and 4 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e7e623...d15d50e. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

1 participant