Skip to content

Eliminate global mutable state, move CLI logic to pkg/#423

Draft
birdayz wants to merge 2 commits intomasterfrom
jb/eliminate-global-state
Draft

Eliminate global mutable state, move CLI logic to pkg/#423
birdayz wants to merge 2 commits intomasterfrom
jb/eliminate-global-state

Conversation

@birdayz
Copy link
Copy Markdown
Owner

@birdayz birdayz commented Feb 14, 2026

What

Gut the monolithic cmd/kaf package. Replace ~65 package-level mutable variables and 7 os.Exit calls with a proper architecture that is actually testable. Fix 20+ bugs found during code review.

Why

The old code was a single package main blob where every command shared mutable globals through package-level vars and init() functions. This made unit testing impossible, parallel test execution impossible, and the whole thing painful to maintain. The only os.Exit should be in main().

Implementation details

Follows the rpk CLI pattern:

  • cmd/kaf/main.go is now 19 lines -- calls pkg/cmd.Execute() and that is it
  • pkg/app.App struct owns all shared mutable state (I/O, config, decode caches, display flags)
  • Each command lives in its own package under pkg/cmd/ (consume, produce, topic, group, node, query, config, completion)
  • Each exports NewCommand(a *app.App) *cobra.Command -- command-specific flags are local variables, not struct fields
  • pkg/cmd/root.go wires the command tree and sets up PersistentPreRunE for config init

Key changes to eliminate os.Exit:

  • readLines/readFull in produce use error channels instead of os.Exit
  • Group commit goroutines use errgroup instead of WaitGroup + os.Exit
  • Config select-cluster returns nil instead of os.Exit(0) on user cancel

Also extracts kafka admin operations into proper pkg/ packages (pkg/client, pkg/topic, pkg/group, pkg/node) and deletes dead code (pkg/streams, pkg/partitioner/jvm.go, SASL/OAuth code consolidated into pkg/client/auth.go).

Backward compatibility

All commands, subcommands, flags, short flags, and defaults are preserved from the old code. Verified by full audit against master. The --verbose flag is retained as a deprecated no-op (sarama was removed). Two intentional default changes:

  • group commit --partition default changed from 0 to -1: old default silently committed only partition 0 when no partition was specified, which was a bug.
  • topic delete now prompts for confirmation (with --noconfirm to skip).

Bug fixes

Crashes:

  • Fix nil dereference when --key-proto-type used without --proto-type
  • Fix config truncation: yaml encoder was not flushed before file close

Data integrity:

  • Hex produce: abort on decode failure instead of silently producing undecoded data
  • Confluent Cloud import: fix JAAS validation that could never fail, fix password quote stripping

Consume:

  • --limit-messages now stops printing exactly at the limit (was printing extra records per poll batch)
  • --limit-messages is now a total count instead of per-partition (prevents hang on sparse partitions)
  • --tail now respects --partitions flag (was always consuming all partitions)
  • Warn when --offset is used with --group (offset is ignored in group mode)

Produce:

  • json-each-row headers are now appended to CLI headers instead of being overwritten

Topic:

  • --partition-assignments now works in combination with -p (was dead code in else branch)
  • Detect compact,delete in cleanup.policy, not just compact
  • Use kmsg.ConfigSourceDefaultConfig constant instead of magic number 5

Group:

  • --topic is now required for group commit (was silently committing against empty topic)
  • Restore missing delete-offsets subcommand (rewritten for franz-go using kadm.DeleteOffsets)
  • Skip groups with errors in group ls instead of including broken entries
  • Don't accumulate negative offsets into TotalOffset in describe

Client:

  • Reject PEM files with no valid certificates instead of silently creating empty cert pool
  • Reject partial mTLS config (cert without key or vice versa)
  • Move all warnings from os.Stderr in library code to returned warning slices

Avro:

  • Schema cache no longer permanently poisons entries on transient failures (errors are evicted, next call retries)

Other:

  • Node list output sorted by ID (was non-deterministic)
  • Fix receiver shadowing in Config.ActiveCluster
  • Fix double ActiveCluster() call in SetCurrentCluster
  • Fix aws_config import alias to awsconfig (Go convention)
  • Remove dead ReadOnly field from ConfigEntry
  • Remove redundant sort on already-sorted partitions
  • go mod tidy

References

Architectural pattern: rpk CLI structure

@birdayz birdayz force-pushed the jb/eliminate-global-state branch 3 times, most recently from ca13545 to 1fd5c47 Compare February 27, 2026 14:04
The cmd/kaf package had ~65 package-level mutable variables and 7
os.Exit calls outside of main(). This made the CLI completely
untestable and impossible to reason about.

Restructure following rpk's pattern:
- cmd/kaf/main.go is now a 19-line wrapper that calls pkg/cmd.Execute()
- pkg/app/ holds the App struct owning all shared mutable state
- pkg/cmd/{consume,produce,topic,group,node,query,config,completion}/
  each export NewCommand(a *app.App) returning *cobra.Command
- pkg/cmd/root.go wires everything together

All command-specific flags are local variables inside factory functions,
not struct fields. os.Exit only appears in main(). The old os.Exit in
readLines/readFull is replaced with error channels, the group commit
goroutines use errgroup, and config select-cluster returns nil instead
of os.Exit(0).

Also extracts kafka operations into proper pkg/ packages:
- pkg/client: franz-go client construction with SASL/TLS/OAuth
- pkg/topic, pkg/group, pkg/node: admin operations
- pkg/avro, pkg/proto, pkg/config: existing packages cleaned up

Deletes dead code: pkg/streams, pkg/partitioner/jvm.go,
cmd/kaf/oauth.go, cmd/kaf/scram_client.go (functionality moved to
pkg/client/auth.go).
@birdayz birdayz force-pushed the jb/eliminate-global-state branch from 1fd5c47 to b5c749b Compare February 27, 2026 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant