Skip to content

feat(cli): add Service Account commands#973

Open
mbevc1 wants to merge 3 commits into
mainfrom
20260625_cmd_sa
Open

feat(cli): add Service Account commands#973
mbevc1 wants to merge 3 commits into
mainfrom
20260625_cmd_sa

Conversation

@mbevc1

@mbevc1 mbevc1 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Add CLI commands wrapping the new service account management endpoints (kosli-dev/server#5951): create, list, get, update and delete service accounts. Previously the CLI could only manage a service account's API keys, not the accounts themselves.

  • create/list/get/delete added under the existing verbs
  • update added under a new update parent command (registered in root)
  • shared models and table printers in serviceAccount.go, mirroring apiKey.go
  • httpfake-backed tests plus response fixtures under testdata/service-account

Add CLI commands wrapping the new service account management endpoints
(kosli-dev/server#5951): create, list, get, update and delete service
accounts. Previously the CLI could only manage a service account's API
keys, not the accounts themselves.

- create/list/get/delete added under the existing verbs
- update added under a new `update` parent command (registered in root)
- shared models and table printers in `serviceAccount.go`, mirroring `apiKey.go`
- httpfake-backed tests plus response fixtures under testdata/service-account
@mbevc1 mbevc1 added enhancement New feature or request tech-investment go Pull requests that update go code labels Jun 25, 2026
@claude

claude Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mbevc1's task in 2m 48s —— View job


Review: Service Account commands

  • Static analysis (go build, go vet, go mod tidy)
  • Review new command files & shared models
  • Review tests & fixtures
  • Post review feedback

Static analysis ✅

Check Result
go build ./... passes
go vet ./cmd/... clean
go mod tidy + git diff go.mod go.sum no changes

What's good 👍

  • Faithful mirroring of apiKey.go conventions — same new<Verb>Cmd factory shape, PreRunE/RequireGlobalFlags, dry-run handling, and the sa alias throughout. Easy to review because it reads like the rest of the package.
  • Strong test coverage for new code: table printers (incl. empty-description → N/A), dry-run URL/payload assertions, alias/shorthand paths, httpfake-backed happy paths, and API error surfacing (404/403). The partial-batch-delete test (TestServiceAccountDeletePartialFailure) is a nice touch and verifies the "already deleted before this failure" reporting.
  • Thoughtful destructive-op UX in deleteServiceAccount.go: confirmation prompt, hidden --yes alias bound to the same flag, and reportAlreadyDeleted making mid-batch failures unambiguous.
  • PATCH semantics done right in updateServiceAccount.go: only Changed() flags are added to the payload, so omitted fields are untouched while an explicit --description "" still clears the value. RequireAtLeastOneOfFlags guards the empty-update case.
  • Single source of truth (validServiceAccountPrivilegesList) for the privilege list, and the prior review note about the --privilege help text was addressed — it now reads One of: [admin, member, snapshotter, reader].

Minor / optional

  1. Shell completion for --privilege — the help text now lists values (resolves discoverability), but the remaining half of the earlier review note is RegisterFlagCompletionFunc. Optional; see the inline comment on createServiceAccount.go. Applies to updateServiceAccount.go too.
  2. No client-side --privilege validation — a typo (--privilege membr) only fails after a server round-trip. The // the server is the authority comment makes this a deliberate choice, so it's fine; just noting a faster local error is possible since the valid set is already known.
  3. Unused out io.Writer params in newCreateServiceAccountCmd/newDeleteServiceAccountCmd/newUpdateServiceAccountCmd (the runs use logger directly). This matches the existing create/delete commands in the package, so it's consistent — flagging only for awareness, no change needed.
  4. Shared serviceAccount struct for both single + list responses — correctly documented as a subset; JSON unmarshalling tolerates the missing fields, so this is fine.

Bug check

No correctness issues found. Dry-run correctly skips both the confirmation prompt and the "was deleted" message; batch delete reports the right prefix (args[:i]) on failure; EOF/empty-stdin confirmation returns "cancelled" as expected and is covered by a test.

Overall this is a clean, well-tested PR that follows the repo's established patterns. The only items are optional polish.
· 20260625_cmd_sa

Comment thread cmd/kosli/root.go Outdated
Comment thread cmd/kosli/createServiceAccount.go
@mbevc1 mbevc1 enabled auto-merge (squash) June 25, 2026 16:32
@mbevc1 mbevc1 changed the title feat(cli): add Service Sccount commands feat(cli): add Service Account commands Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request go Pull requests that update go code tech-investment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant