From 5ee7f6110639a78e3aa0eb855af97b22cb0ed68d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D7=A0=CF=85=CE=B1=CE=B7=20=D7=A0=CF=85=CE=B1=CE=B7=D1=95?= =?UTF-8?q?=CF=83=CE=B7?= Date: Sun, 16 Aug 2026 09:35:10 -0700 Subject: [PATCH] docs: standardize CONTRIBUTING against repo-standards The five Go repositories stated the same conventions five different ways. Here they were split across Code standards and Testing, which each described test package layout, suite naming, and table-driven cases -- the same facts twice in one file. Applies the section order repo-standards now fixes: Setup, Project structure, Code style, Code standards, Testing, then the operation walkthrough. Package Structure folds into Project structure, and the Title Case headings become sentence case. The linter list is gone. It named goimports as a linter and omitted unused, while .golangci.yml has enabled unused all along and configures goimports as a formatter. Supersedes #76, which removed these conventions rather than standardizing them. Co-Authored-By: Claude Opus 5 --- CONTRIBUTING.md | 110 +++++++++++++++++++++++++++--------------------- 1 file changed, 62 insertions(+), 48 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e9b6635..e53ef68 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -62,7 +62,7 @@ just fetch just deps ``` -## Project Structure +## Project structure ``` pkg/orchestrator/ # User-facing DSL @@ -90,8 +90,6 @@ examples/ host-status.go, guards.go, broadcast.go, ... ``` -## Package Structure - - **`pkg/orchestrator/`** — User-facing DSL - Typed operation constructors (NodeHostnameGet, CommandExec, etc.) - Uses SDK types directly (`osapi.HostnameResult`, `osapi.Agent`, etc.) @@ -108,6 +106,11 @@ just go-fmt # Auto-fix formatting just go-vet # Run linter ``` +The linters that run are declared in `.golangci.yml`. Read them there rather +than looking for a list here — a copied list goes stale the first time the +configuration changes. Generated files (`*.gen.go`, `*.pb.go`) are excluded from +formatting. + ### Documentation Markdown files are formatted with [mdformat] through `uvx`. This style is @@ -120,15 +123,10 @@ just md-fmt # Auto-fix formatting ## Code standards -These conventions are shared across every Go repository in the organization and -are specified in the `go-code-standards` capability in -[osapi-io/specs](https://github.com/osapi-io/specs). They are restated here -because a contributor should not have to read another repository to learn how to -write code in this one. Where the two disagree, the specification wins. - -### Function Signatures +### Function signatures -Functions with parameters use multi-line format, one parameter per line: +Functions with parameters use multi-line format — one parameter per line, with +the closing parenthesis and the return types on a line of their own: ```go func FunctionName( @@ -138,35 +136,41 @@ func FunctionName( } ``` -Zero-parameter functions stay on one line. +Functions taking no parameters stay on one line: -### Testing +```go +func Name() string { +} +``` -- Public tests: `*_public_test.go` in `package orchestrator_test`, exercising - the exported surface. This is the default. -- Internal tests: `*_test.go` in `package orchestrator`, for what the exported - surface cannot reach. -- Suite naming: `*_public_test.go` → `{Name}PublicTestSuite`, `*_test.go` → - `{Name}TestSuite`. -- `testify/suite` with table-driven cases. -- One suite method per function under test — success, errors, and edge cases are - rows in one table, not separate methods. +Adding a parameter then shows as one added line rather than a rewritten +signature. -Tests exercise a real HTTP server via `httptest.Server` rather than mocking the -SDK client, so this repository declares no mocking library. +### File naming -### Go Patterns +Name a file for what it holds. Avoid `helpers.go`, `utils.go`, and names of that +kind: they describe where code was put rather than what it is, and they +accumulate whatever has no other home. -- Error wrapping: `fmt.Errorf("context: %w", err)` -- Early returns over nested if-else -- Unused parameters: rename to `_` -- Import order: stdlib, third-party, local (blank-line separated) +`types.go` holds only type declarations — structs, interfaces, constants, and +aliases. A function belongs in a file named for what it does. -### Linting +A test file is named for the production file it tests. Where tests grow too +large to read, split the production file first so each test file keeps a +counterpart, rather than splitting tests away from the file they cover. -golangci-lint with: errcheck, errname, goimports, govet, prealloc, predeclared, -revive, staticcheck. Generated files (`*.gen.go`, `*.pb.go`) are excluded from -formatting. +### Go patterns + +- Error wrapping: `fmt.Errorf("context: %w", err)`, so the chain names each + layer it passed through and stays inspectable with `errors.Is` and + `errors.As`. +- Early returns rather than nesting the successful path inside conditionals. +- Unused parameters: rename to `_`. +- Import order: standard library, third party, then local, separated by blank + lines. + +Tests exercise a real HTTP server via `httptest.Server` rather than mocking the +SDK client. ## Testing @@ -189,21 +193,31 @@ module — change both together. ### Test file conventions -- Public tests: `*_public_test.go` in test package (`package orchestrator_test`) - for exported functions. -- Use `testify/suite` with table-driven patterns. -- Table-driven structure with `validateFunc` callbacks. -- **One suite method per function under test.** All scenarios for a function - (success, error codes, transport failures, nil responses) belong as rows in a - single table — never split into separate `TestFoo`, `TestFooError`, - `TestFooNilResponse` methods. +- Public tests: `*_public_test.go` in the package's `_test` package, exercising + the exported surface. This is the default. +- Internal tests: `*_test.go` in the same package, for what the exported surface + cannot reach. +- Suite naming: `*_public_test.go` → `{Name}PublicTestSuite`, `*_test.go` → + `{Name}TestSuite`. +- `testify/suite` with table-driven cases. +- One suite method per function under test — success, errors, and edge cases are + rows in one table, not separate methods. +- `export_test.go` exposes unexported symbols to external tests, by alias or by + setter. Do not use an alias to re-cover behavior the caller's own test already + reaches; a helper with its own contract is what the pattern is for. +- Mocks are generated with `go.uber.org/mock` and committed, never hand-written. + A double that carries a real implementation — signing with a real key, serving + real HTTP — is not a mock and does not need generating. + +External tests in this repository live in `package orchestrator_test`, and +tables carry `validateFunc` callbacks. ## Adding a new operation When adding a new typed constructor (e.g., `NodeRebootDo`), follow these steps in order. Every operation must ship with tests, docs, and an example. -### Step 1: Operation Constructor +### Step 1: operation constructor Add the method to `pkg/orchestrator/ops.go`, following the existing pattern: @@ -252,7 +266,7 @@ Key rules: - The `engine` import is `internal/engine` — only used inside `pkg/orchestrator/`, never by external consumers -### Step 2: Tests +### Step 2: tests Two test files must be updated: @@ -271,7 +285,7 @@ tests that the constructor creates valid steps: Target 100% coverage on both files. -### Step 3: Operation Doc +### Step 3: operation doc Create `docs/operations/{domain}/{operation}.md` following the existing template in that domain directory. Every doc must include these sections: @@ -289,13 +303,13 @@ in that domain directory. Every doc must include these sections: for a complete working example. ``` -### Step 4: Update Domain Landing Page and Operation Index +### Step 4: update the domain landing page and operation index Add the operation to the table in the domain landing page `docs/operations/{domain}/README.md`. Update the operation count in `docs/operations/README.md` if the total changes. -### Step 5: Example +### Step 5: example Add the operation to an existing workflow example in `examples/operations/` that covers the same domain. Domain groupings: @@ -348,12 +362,12 @@ appear in at least one runnable example. `docs/operations/{domain}/` must link to the example file where that operation is demonstrated. -### Step 6: Update README.md +### Step 6: update README.md Update the operation count and tables in the root `README.md` if the total number of operations changes. -### Step 7: Verify +### Step 7: verify ```bash go build ./... # compiles