Skip to content

refactor(kosli): add a typed Kosli API client core - #1248

Draft
dangrondahl wants to merge 10 commits into
mainfrom
refactor/kosli-client
Draft

dangrondahl wants to merge 10 commits into
mainfrom
refactor/kosli-client

Conversation

@dangrondahl

Copy link
Copy Markdown
Contributor

Commands build every Kosli API call themselves. Each one joins global.Host with api/v2 and global.Org, passes global.ApiToken, and guards against the (nil, nil) that requests.Client.Do returns on a dry run. Most of them then decode the body by hand. There are 80 such call sites, and some endpoints are built in two or three places.

This PR adds internal/kosli, a client that will hold one file per tag of the OpenAPI spec. It follows the conventions of terraform-provider-kosli/pkg/client: ctx first, typed structs next to each endpoint, and Is* error helpers. This first slice is only the core. It has no endpoints and no callers yet, and nothing in cmd/kosli changes.

  • New(sender, host, org, token). Endpoint files will build on three unexported helpers: endpoint(segments…), send(ctx, params) and decode[T].
  • Result{Raw, Created} and Response[T]{Result; Value}. Raw is the server's body exactly as it arrived, so --output json can keep printing it unchanged. Created replaces the res.Resp.StatusCode == 200 checks.
  • DryRun(sender) is a sender adapter. GETs still reach the server, as they do today. Writes are logged and not sent, using the existing payload printing in requests. A dry-run result is an empty Result, not nil.
  • APIError is an alias of requests.APIError, with IsNotFound, IsConflict and IsForbidden added.

Changes to internal/requests

  • RequestParams.Context is new. Nil means context.Background(), so existing callers are unaffected.
  • Transport errors are now wrapped with %w instead of %v. The message is the same, and errors.Is(err, context.Canceled) now works.

Worth knowing

A 409, 5xx or 429 is retried. When the retries run out, the caller gets a plain "giving up after N attempt(s)" error and not an APIError. IsConflict therefore only matches a request that sets DisableConflictRetry. This is the same as on main.

Next slices

Each slice is its own PR, stacked on this one:

  1. GetTrail, used by get trail and evaluate. This slice also builds the client in PersistentPreRunE.
  2. Artifacts
  3. Environments, with snapshotReporter moved into the client
  4. Evaluations
  5. Attestations
  6. The remaining tags, one per PR

Tests

There are 12 httptest tests in internal/kosli. make lint is clean. I have not run the integration suite locally for this PR.

Checklist

@github-actions github-actions Bot added go Pull requests that update go code refactor labels Oct 5, 2026
@claude

claude Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @dangrondahl's task in 55s —— View job


Follow-up review of new commits (98849b38..c624320e)

  • Fetch previous review findings and diff the new commits
  • Check each previous finding against the new commits
  • Scan the changed hunks for new bugs/regressions
  • Post summary

Previous findings — status

  1. Race in newFakeServer's handler (client_test.go:35) — Resolved. require.NoError → assert.NoError in 131a747, so FailNow is no longer called off the test goroutine. Author confirmed go test -race ./internal/kosli/... passes (I wasn't able to run tests myself in this job — no tool access to execute go test).

  2. != http.MethodGet misclassifies HEAD/OPTIONS as writes, and mutates the caller's params (dryrun.go) — Resolved. c624320 replaces the negative check with an explicit allow-list switch (GET, HEAD, OPTIONS pass through; everything else is a write), and writes now go through a local copy (write := *params; write.DryRun = true) instead of mutating the caller's struct. New tests TestDryRunStillSendsTheOtherSafeMethods and TestDryRunLeavesTheCallersParamsUnchanged cover both points directly.

  3. Dry-run write returns Created: false, indistinguishable from a real update (client.go:73) — Not addressed, as expected — this was flagged as something to plan for in a later slice (needs a DryRun bool on Result or similar), not a fix requested for this PR.

New issues in the changed lines: none found. The allow-list switch and copy-before-mutate approach are correct; the shallow copy of RequestParams is safe since no field it shares (maps/context) is touched by DryRun.

Comment thread internal/kosli/client_test.go
Comment thread internal/kosli/dryrun.go Outdated
Comment thread internal/kosli/client.go

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go Pull requests that update go code refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant