Skip to content

refactor(environments, snapshot): go through the Kosli API client - #1271

Draft
dangrondahl wants to merge 5 commits into
refactor/kosli-get-trailfrom
refactor/kosli-environments
Draft

dangrondahl wants to merge 5 commits into
refactor/kosli-get-trailfrom
refactor/kosli-environments

Conversation

@dangrondahl

@dangrondahl dangrondahl commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #1249, next to #1269 (the two don't depend on each other). This adds the Environments endpoints to internal/kosli and moves every caller of them onto the client: get environment, create environment, auto-environment, and the snapshot reporter that all 11 snapshot commands share.

  • internal/kosli/environments.go
    • GetEnvironment(ctx, name) returns *Response[Environment]. Environment only has Name and Type for now, because those are all any caller reads. Value() decodes lazily, so get environment -o json still prints the raw body.
    • CreateOrUpdateEnvironment(ctx, CreateOrUpdateEnvironmentRequest) returns *Result. The name follows the operation and makes the overwrite visible at call sites. The request type moves here from cmd, where it was CreateEnvironmentPayload. OpenAPIContractTestSuite now checks the moved type against CreateEnvironmentPutInput.
    • IncludeScaling and RequireProvenance are marked Deprecated:. On the server, both are deprecated=True in CreateEnvironmentPutInput, accepted and ignored. Scaling is never recorded, and require_provenance was replaced by policies. The deprecated CLI flags still set them, behind a //nolint:staticcheck with the reason. The model forbids extra fields, so the server can only drop them once the CLI stops sending them. The CLI should therefore remove the fields and the flags together.
    • ReportSnapshot(ctx, name, envType, payload) returns *Result. The payload stays any: each snapshot command still builds its own per-type body.
  • get environment and create environment now go through kosliAPI(). Requests with the deprecated flags were compared too, and their bodies are unchanged.
  • Auto-environment
    • getEnvironmentTypeIfExists uses GetEnvironment. As before, only a genuine 404 means the environment doesn't exist; that is now checked with kosli.IsNotFound.
    • createEnvironmentForSnapshot uses CreateEnvironment.
  • snapshotReporter
    • It holds a *kosli.Client instead of client, host, org and token. Its dryRun field now only controls whether the success message is printed, because the dry-run sender already stops the PUT.
    • report and ensure take ctx.
  • The 11 snapshot commands take run(ctx, args), and RunE passes cmd.Context(). ctx also goes through the K8S multi-environment loop and the --watch path loops. This PR doesn't add any new cancellation behaviour.

operationIds

Method operationId
GetEnvironment get_environment
CreateOrUpdateEnvironment create_or_update_environment
ReportSnapshot report_<type>_environment, e.g. report_k8s_environment, report_ecs_environment

There is one report operation per environment type. I couldn't confirm the ids for docker, lambda and server, because my view of the published spec was cut off. So the godoc only names the two I saw.

Verification

  • internal/kosli has 5 new httptest tests:
    • get: path plus lazy Type
    • 404 → IsNotFound
    • create: PUT body
    • report: escaped path plus body
    • a dry run sends neither write
    • The fake server now records request bodies.
  • All 11 snapshotReporter tests pass under -race. They now build the reporter around a kosli.Client, wrapped in DryRun for the dry-run case.
  • make lint is clean.
  • I compared refactor(get trail, evaluate): fetch the trail through the Kosli API client #1249's binary with this branch's binary against a local fake environments server, using 15 cases. Both the command output and the server's request log (method, path, body, in order) are identical. Cases:
    • get environment: table, json and 404
    • create environment, including --exclude-scaling, a logical environment with --included-environments, and --dry-run
    • snapshot server without auto-environment
    • snapshot server with --auto-environment in these situations:
      • creating a new environment, including one whose name has a space
      • --environment-description and --include-scaling
      • a type mismatch, and a logical environment
      • flags that are ignored without -A
      • --dry-run
  • I haven't run the integration suites locally. CI covers them: SnapshotAutoEnvironmentTestSuite, the Snapshot* suites, the get/create environment suites and OpenAPIContractTestSuite.

Not in this PR

  • Typed report requests, one per environment type (Q20). That's a separate slice.
  • The other Environments operations: list, archive, rename, join, policies, events, and so on.

Conflict to expect

#1269 also adds a field to the test fake server's received struct in internal/kosli/client_test.go. Whichever of the two PRs is rebased second will hit a small conflict there.

Checklist

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

claude Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @dangrondahl's task in 1m 28s —— View job


Follow-up review of 8438f1d8 (vs b0b79bd8)

Diff scope: createEnvironment.go, snapshotAutoEnvironment.go, snapshotAutoEnvironment_test.go, testHelpers.go, openapiContract_test.go, internal/kosli/environments.go, internal/kosli/environments_test.go — all driven by the CreateEnvironment → CreateOrUpdateEnvironment rename and the two new Deprecated: field comments.

  • Fetch previous review comments/findings
  • Diff BEFORE..AFTER (b0b79bd8..8438f1d8)
  • Assess status of each previous finding touched by new commits
  • Flag any new bugs/regressions in changed lines only
  • Post summary comment

No inline review threads exist on this PR yet, so there's nothing to resolve/close — the prior findings were raised as a plain summary comment, and dangrondahl's reply at 8438f1d already states their disposition. Status of each, verified against the diff:

  1. Two sources of truth for dry-run reporting — not addressed (intentionally deferred). Confirmed out of scope for this PR; author plans to fold it into a separate error-rendering slice where Result.DryRun would drive the success message.
  2. ctx not plumbed into the --watch loops — not addressed (intentionally deferred). No changes to the watch loop in this diff; recorded as a follow-up for deliberate cancellation work with its own tests.
  3. IsNotFound matching on more than a genuine 404 — resolved, verified directly: internal/kosli/client_test.go:111 and :139 already table-test that IsNotFound/IsConflict/IsForbidden return false for other statuses, plain errors, and nil.
  4. Naming (CreateEnvironment → something reflecting create-or-update) — resolved. CreateEnvironmentRequest/CreateEnvironment are renamed to CreateOrUpdateEnvironmentRequest/CreateOrUpdateEnvironment throughout (internal/kosli/environments.go:31,46, and all 5 call sites/tests), matching the create_or_update_environment operationId.

New in this commit: IncludeScaling and RequireProvenance on CreateOrUpdateEnvironmentRequest are now marked // Deprecated: with accurate rationale (server accepts-but-ignores scaling; require_provenance superseded by policies), and the three call sites that still set them (createEnvironment.go, snapshotAutoEnvironment.go) carry //nolint:staticcheck with a reason, correctly suppressing the SA1019 warnings the new doc comments would otherwise trigger on the only legitimate (deprecated-flag) callers.

Reviewed every changed line in this diff for new bugs/regressions — found none. The rename is mechanical and complete (no stray CreateEnvironment/CreateEnvironmentRequest references left), the nolint placements are correctly scoped to the deprecated-flag assignments only, and the godoc for CreateOrUpdateEnvironment accurately reflects create-or-overwrite semantics. 0 new inline comments posted.

Note: this sandboxed run couldn't execute go build/go vet/make lint (network/build commands require approval not available here), so this review is based on manual reading of the diff rather than a local build/test run.

…recated fields

The method calls create_or_update_environment and overwrites an existing
environment, so its name now says so. IncludeScaling and RequireProvenance
are deprecated on the server (accepted, ignored); the deprecated flags that
set them keep working, behind a reasoned nolint.
@dangrondahl

Copy link
Copy Markdown
Contributor Author

Thanks. Here's what happened to each suggestion, in 8438f1d:

  1. Two sources of truth for dry-run. Agreed, and the idea of having send mark dry-run results with Result.DryRun is a good one. It changes how every write reports success, though, so I've put it in the error-rendering slice rather than this PR.
  2. ctx in the --watch loops. Recorded as a follow-up: cancellation should be added deliberately, with its own tests.
  3. Non-404 errors aren't IsNotFound. Already covered in refactor(kosli): add a typed Kosli API client core #1248: internal/kosli/client_test.go checks that each Is* helper returns false on other statuses, on plain errors and on nil.
  4. Naming. Done. The method is now CreateOrUpdateEnvironment and its request type is CreateOrUpdateEnvironmentRequest, matching create_or_update_environment.

Also in this commit, IncludeScaling and RequireProvenance are now marked Deprecated:.

  • I checked the server first. Both are deprecated=True in CreateEnvironmentPutInput and are accepted but ignored: scaling is never recorded, and require_provenance was replaced by policies.
  • The deprecated flags that set them keep working, behind a //nolint:staticcheck with the reason.
  • A fresh comparison against the fake server shows that requests using those flags still send the same bodies.

@dangrondahl
dangrondahl added this pull request to stack #1272 October 15, 2026 15:11

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