Skip to content

feat: add --sort-direction flag to list controls#1012

Open
mbevc1 wants to merge 3 commits into
mainfrom
20260709_list_controls
Open

feat: add --sort-direction flag to list controls#1012
mbevc1 wants to merge 3 commits into
mainfrom
20260709_list_controls

Conversation

@mbevc1

@mbevc1 mbevc1 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Aligns the command with the new list-controls API spec, which adds a sort_direction query parameter (asc/desc, server default asc). The flag follows the existing kosli list environments pattern: empty default, only sent when set.

Aligns the command with the new list-controls API spec, which adds a
sort_direction query parameter (asc/desc, server default asc). The flag
follows the existing kosli list environments pattern: empty default,
only sent when set.
@mbevc1 mbevc1 added enhancement New feature or request go Pull requests that update go code labels Jul 9, 2026
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mbevc1's task in 1m 41s —— View job


Review: --sort-direction flag for list controls

  • Gather context (read changed files, compared to list environments pattern)
  • Phase 1: static analysis
  • Phase 2: code review
  • Post review feedback

Phase 1 — Static analysis ✅

Check Result
go build ./... ✅ pass
go vet ./cmd/kosli/ ✅ pass
go mod tidy (+ git diff go.mod go.sum) ✅ no changes

Phase 2 — Code review

Verdict: LGTM. The change is small, correct, and faithfully mirrors the established list environments pattern.

  • ConsistencylistControls.go:123-125 uses the exact "empty default, only sent when set" idiom already used for sort_direction in listEnvironments.go:144-145. The help text (root.go:308) matches envSortDirectionFlag verbatim in wording. 👍
  • Docs — the new example block (listControls.go:53-57) keeps the --help examples in sync with the new flag.
  • Validation — no client-side asc/desc check; invalid values are rejected server-side. Already discussed on the inline thread and accepted as consistent with the existing pattern. No change required.

Testing — nicely done. TestListControlsSortDirectionIsHonored asserts the two directions against each other rather than each against the server default. As the comment notes, this means a silently-dropped flag would collapse both runs to the default order and fail the reversal assertion — a genuinely meaningful test rather than one that trivially passes.

One thing worth confirming: the switch from SetupTestSetupSuite (listControls_test.go:18). This is a sensible optimization — fixtures are now created once instead of before every test method, avoiding repeated CreateControl calls. It's safe here because every test passes org/token explicitly via suite.defaultKosliArguments / suite.acmeOrgKosliArguments, so the one-time mutation of the package-level global (to acme-org at the end of setup) doesn't leak between tests. Just flagging it since it's a behavioral change beyond the flag itself — no action needed, but good to be aware the new --sort-direction test relies on the two list-control-{1,2} fixtures created once in SetupSuite.

No bugs, security, or performance concerns. Ship it. 🚢
· 20260709_list_controls

@mbevc1 mbevc1 enabled auto-merge (squash) July 9, 2026 18:00
Comment thread cmd/kosli/listControls.go
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant