#58 refactor cli commands by rustyeddy · Pull Request #65 · rustyeddy/otto

rustyeddy · 2025-12-29T20:42:07Z

Copilot

Pull request overview

This PR refactors the CLI command structure by separating the server binary (otto) from the CLI control tool (ottoctl). The main changes involve moving command-line functionality from the cmd/ package into a new cmd/ottoctl/ package, centralizing version management, and simplifying the main server entry point.

Key Changes:

Split the monolithic CLI into two distinct binaries: otto (server) and ottoctl (CLI client)
Introduced a centralized version constant in version.go
Added /version HTTP endpoint to the server with GetVersion client method

Reviewed changes

Copilot reviewed 33 out of 34 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
version.go	New file defining the version constant (0.0.12)
otto.go	Removed init function, added /version endpoint handler, added empty ServeHTTP method
otto_test.go	Removed TestOttOBrokerShutdown test
cmd/otto/main.go	Simplified to directly start server without cobra commands
cmd/ottoctl/	New package containing refactored CLI commands (version, stats, stations, cli)
cmd/ottoctl/ottoctl/main.go	New entry point for ottoctl binary
cmd/ottoctl/testdata/	New test data files for version, stats, stations, and help
cmd/cmd_*.go	Removed old command files (serve, version, ticker, stats, stations, file)
client/client.go	Added GetVersion method to retrieve server version
Makefile	Updated to build both otto and ottoctl binaries
.gitignore	Updated to include ottoctl binary

Comments suppressed due to low confidence (4)

cmd/ottoctl/cmd_stats.go:23

The GetClient function now always returns a non-nil client because the serverURL flag has a default value of "http://localhost:8011" (line 31). This means the nil check on line 21 will never be true, causing the error message on line 22 to be unreachable. If a nil client is a valid scenario, the default value should be empty string, or the logic should handle the default differently.
cmd/ottoctl/cmd_root.go:61
The ottoRun function is empty but is set as the Run handler for the root command. According to the removed code history (cmd/cmd_root.go line 60-61), this used to call serveRun. The function should either have a meaningful implementation or display usage/help information when no subcommand is provided.
cmd/ottoctl/cmd_root.go:32
The 'format' flag is defined and documented (line 32) but is never used in any of the command implementations. Commands like stations and stats always output JSON format. Either implement the format flag to allow human-readable output or remove it if not needed.
cmd/ottoctl/cmd_root.go:63
The GetClient comment states it returns nil "if remote mode is enabled" but this is backwards - it should say "returns an Otto client if a server URL is configured, nil otherwise" to accurately reflect the function's behavior.

Copilot · 2025-12-29T20:46:21Z

@@ -0,0 +1 @@
+package ottoctl


The cmd_stations_test.go file only contains a package declaration with no test functions. Since other command files in this package have test coverage (e.g., cmd_version_test.go, cmd_root_test.go, cmd_cli_test.go), the stations command should also have test coverage to maintain consistency and ensure the command works correctly.

Copilot · 2025-12-29T20:46:22Z

+  otto [command]
+
+Available Commands:
+  cli         Run auto in interactive CLI mode


The command description has a typo: "Run auto in interactive CLI mode" should be "Run otto in interactive CLI mode". The word "auto" should be "otto" to match the application name.

Suggested change

cli Run auto in interactive CLI mode

cli Run otto in interactive CLI mode

Copilot · 2025-12-29T20:46:22Z

+		o.Server.HandleFunc("/version", func(w http.ResponseWriter, r *http.Request) {
+			w.Write([]byte(fmt.Sprintf(`{"version": "%s"}`, version)))
+		})


The version handler references an unexported variable 'version' (lowercase), but this file defines an exported 'Version' (uppercase) variable at line 167. This will cause a compilation error. The handler should use the version constant from the new version.go file or the exported Version variable.

Copilot · 2025-12-29T20:46:22Z

+	if client == nil {
+		fmt.Fprintf(cmdOutput, "Failed to get an otto client")
+		return
+	}


The GetClient function now always returns a non-nil client because the serverURL flag has a default value of "http://localhost:8011" (cmd_root.go line 31). This means the nil check on line 20 will never be true, causing the error message on line 21 to be unreachable. If a nil client is a valid scenario, the default value should be empty string, or the logic should handle the default differently.

Suggested change

if client == nil {

fmt.Fprintf(cmdOutput, "Failed to get an otto client")

return

}

Copilot · 2025-12-29T20:46:23Z

+func (o *OttO) ServeHTTP(w *http.ResponseWriter, r http.Request) {
+
+}


The ServeHTTP method is empty and appears to be unused. If this method is intended to implement the http.Handler interface, it should have a proper implementation. If it's not needed, it should be removed. Additionally, the method signature is incorrect - the ResponseWriter parameter should be a value, not a pointer (http.ResponseWriter is already an interface).

Copilot · 2025-12-29T20:46:23Z

@@ -0,0 +1 @@
+package ottoctl


The cmd_stats_test.go file only contains a package declaration with no test functions. Since other command files in this package have test coverage (e.g., cmd_version_test.go, cmd_root_test.go, cmd_cli_test.go), the stats command should also have test coverage to maintain consistency and ensure the command works correctly.

Copilot · 2025-12-29T20:46:23Z

+	go build -o ${OTTOCTL_BINARY} -ldflags "-X github.com/rustyeddy/otto/cmd.version=${VERSION}" ./cmd/ottoctl/ottoctl
+
+otto:
+	go build -o ${OTTO_BINARY} -ldflags "-X github.com/rustyeddy/otto/cmd.version=${VERSION}" ./cmd/otto


The ldflags parameter attempts to set a version variable in the wrong package path. It references "github.com/rustyeddy/otto/cmd.version" but the cmd package no longer exists in this refactoring. Since version is now defined in version.go as an unexported variable in the otto package, the ldflags should either be removed or updated to set the correct variable path like "github.com/rustyeddy/otto.version" (if the variable is made exported).

Suggested change

go build -o ${OTTOCTL_BINARY} -ldflags "-X github.com/rustyeddy/otto/cmd.version=${VERSION}" ./cmd/ottoctl/ottoctl

otto:

go build -o ${OTTO_BINARY} -ldflags "-X github.com/rustyeddy/otto/cmd.version=${VERSION}" ./cmd/otto

go build -o ${OTTOCTL_BINARY} ./cmd/ottoctl/ottoctl

otto:

go build -o ${OTTO_BINARY} ./cmd/otto

Copilot · 2025-12-29T20:46:24Z

+	if err != nil {
+		fmt.Fprintln(cmdOutput, "Failed to get otto client", err)
+	}
+	version, ex := vmap["version"]
+	if !ex {
+		fmt.Fprintf(errOutput, "%s\n", "failed to get version")
+		return errors.New("failed to get version")


The error message format is inconsistent. Line 25 uses "Failed to get otto client" with Fprintln, while line 29 writes to errOutput and line 30 returns a proper error. These should be consolidated - either return the original error from line 24 immediately, or write a consistent error message to errOutput.

Copilot · 2025-12-29T20:46:24Z

+type tstInput struct {
+	response string
+	buffer   *bytes.Buffer
+	errbuf   *bytes.Buffer
+	cmd      *cobra.Command
+	args     []string
+}


The type name 'tstInput' uses an abbreviated name that is unclear. Following Go naming conventions and the project's guidelines for clear naming, this should be renamed to something more descriptive like 'testInput' or 'commandTestInput' to better indicate its purpose.

Copilot · 2025-12-29T20:46:24Z

@@ -25,18 +25,3 @@ func TestOttOInit(t *testing.T) {
 	}()
 	o.Stop()
 }


The TestOttOBrokerShutdown test has been removed without replacement. This test was validating important shutdown behavior for the broker. If this functionality is still relevant, consider adding an equivalent test to ensure proper shutdown handling is maintained.

rustyeddy added 5 commits December 27, 2025 11:25

refactored binaries

c03b173

Got version command working

60bf75d

All commands now use the same cli

15cf0ab

added test json files

45d4ca6

Commands now use REST and otto commands moved to ottoctl

fbf88ab

rustyeddy added this to the Refactor CLI milestone Dec 29, 2025

rustyeddy self-assigned this Dec 29, 2025

Copilot AI review requested due to automatic review settings December 29, 2025 20:42

rustyeddy added the Architecture Design decisions. label Dec 29, 2025

rustyeddy linked an issue Dec 29, 2025 that may be closed by this pull request

Refactor CLI Commands #58

Open

rustyeddy merged commit d670a64 into main Dec 29, 2025
5 checks passed

Copilot started reviewing on behalf of rustyeddy December 29, 2025 20:42 View session

rustyeddy deleted the 58-refactor-cli-commands branch December 29, 2025 20:42

Copilot AI reviewed Dec 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#58 refactor cli commands#65

#58 refactor cli commands#65
rustyeddy merged 5 commits into
mainfrom
58-refactor-cli-commands

rustyeddy commented Dec 29, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 29, 2025

Uh oh!

Copilot AI Dec 29, 2025

Uh oh!

Copilot AI Dec 29, 2025

Uh oh!

Copilot AI Dec 29, 2025

Uh oh!

Copilot AI Dec 29, 2025

Uh oh!

Copilot AI Dec 29, 2025

Uh oh!

Copilot AI Dec 29, 2025

Uh oh!

Copilot AI Dec 29, 2025

Uh oh!

Copilot AI Dec 29, 2025

Uh oh!

Copilot AI Dec 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	cli Run auto in interactive CLI mode
	cli Run otto in interactive CLI mode

		func (o OttO) ServeHTTP(w http.ResponseWriter, r http.Request) {

		}

Conversation

rustyeddy commented Dec 29, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants