fix(cli): address code review feedback from Copilot and CodeRabbit

- Consolidate redundant buildx requirement notes in push.md
- Fix duplicate -t latest tag when version == "latest" in buildAndPush
- Add validation for empty repository parameter in ListTags
- Remove duplicate error output in restart command (no styled print + return)
- Extract tag normalization logic into exported sortSemverTags helper
- Add Unwrap() method to ResponseWriter for http.ResponseController
- Fix resolveLogFilePath to fallback to DefaultDataDir when empty

All tests pass after refactoring.

commit: 89e85a913df394f84818c8048f38b7142b8d4408
author: brice · 8mo ago · 2026-02-02 18:17 UTC
parent: 4d75a1f
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

8 files · +32 -47

M docs/cli/push.md +2 -3


@@ -60,9 +60,8 @@ gordon push myapp.example.com --build --platform linux/arm64 --build-arg CGO_ENA
60	60		### Notes
61	61
62	62		- Remote mode required. See [CLI Overview](./index.md) for targeting options.
63		-	- `--build` requires Docker with Buildx available. Docker Desktop includes Buildx;
64		-	on Linux, install the `docker-buildx-plugin` package.
65		-	- `docker buildx` must be available when using `--build`.
	63	+	- `--build` requires Docker with Buildx. Docker Desktop includes it; on Linux,
	64	+	install the `docker-buildx-plugin` package.
66	65
67	66		## Related
68	67


@@ -60,9 +60,8 @@ gordon push myapp.example.com --build --platform linux/arm64 --build-arg CGO_ENA
60		### Notes	60		### Notes
61			61
62		- Remote mode required. See [CLI Overview](./index.md) for targeting options.	62		- Remote mode required. See [CLI Overview](./index.md) for targeting options.
63	-	- `--build` requires Docker with Buildx available. Docker Desktop includes Buildx;	63	+	- `--build` requires Docker with Buildx. Docker Desktop includes it; on Linux,
64	-	on Linux, install the `docker-buildx-plugin` package.	64	+	install the `docker-buildx-plugin` package.
65	-	- `docker buildx` must be available when using `--build`.
66			65
67		## Related	66		## Related
68			67

M internal/adapters/in/cli/push.go +3 -1


@@ -132,10 +132,12 @@ func buildAndPush(ctx context.Context, version, platform string, buildArgs []str
132	132		args := []string{
133	133		"buildx", "build",
134	134		"--platform", platform,
135		-	"-t", versionRef,
136	135		"-t", latestRef,
137	136		"--build-arg", "VERSION=" + version,
138	137		}
	138	+	if version != "latest" {
	139	+	args = append(args, "-t", versionRef)
	140	+	}
139	141		for _, ba := range buildArgs {
140	142		args = append(args, "--build-arg", ba)
141	143		}

 	args := []string{
 		"buildx", "build",
 		"--platform", platform,
-		"-t", versionRef,
 		"-t", latestRef,
 		"--build-arg", "VERSION=" + version,
 	}
+	if version != "latest" {
+		args = append(args, "-t", versionRef)
+	}
 	for _, ba := range buildArgs {
 		args = append(args, "--build-arg", ba)
 	}

M internal/adapters/in/cli/remote/client.go +3 -0


@@ -455,6 +455,9 @@ func (c *Client) Restart(ctx context.Context, restartDomain string, withAttachme
455	455
456	456		// ListTags returns available tags for a repository.
457	457		func (c *Client) ListTags(ctx context.Context, repository string) ([]string, error) {
	458	+	if repository == "" {
	459	+	return nil, fmt.Errorf("repository cannot be empty")
	460	+	}
458	461		resp, err := c.request(ctx, http.MethodGet, "/tags/"+url.PathEscape(repository), nil)
459	462		if err != nil {
460	463		return nil, err


@@ -455,6 +455,9 @@ func (c *Client) Restart(ctx context.Context, restartDomain string, withAttachme
455		455
456	// ListTags returns available tags for a repository.	456		// ListTags returns available tags for a repository.
457	func (c *Client) ListTags(ctx context.Context, repository string) ([]string, error) {	457		func (c *Client) ListTags(ctx context.Context, repository string) ([]string, error) {
		458	+	if repository == "" {
		459	+	return nil, fmt.Errorf("repository cannot be empty")
		460	+	}
458	resp, err := c.request(ctx, http.MethodGet, "/tags/"+url.PathEscape(repository), nil)	461		resp, err := c.request(ctx, http.MethodGet, "/tags/"+url.PathEscape(repository), nil)
459	if err != nil {	462		if err != nil {
460	return nil, err	463		return nil, err

M internal/adapters/in/cli/restart.go +0 -1


@@ -29,7 +29,6 @@ Examples:
29	29
30	30		client, isRemote := GetRemoteClient()
31	31		if !isRemote {
32		-	fmt.Println(styles.RenderError("restart command requires --remote flag or GORDON_REMOTE env var"))
33	32		return fmt.Errorf("restart requires remote mode")
34	33		}
35	34


@@ -29,7 +29,6 @@ Examples:
29			29
30		client, isRemote := GetRemoteClient()	30	client, isRemote := GetRemoteClient()
31		if !isRemote {	31	if !isRemote {
32	-	fmt.Println(styles.RenderError("restart command requires --remote flag or GORDON_REMOTE env var"))
33		return fmt.Errorf("restart requires remote mode")	32	return fmt.Errorf("restart requires remote mode")
34		}	33	}
35			34

M internal/adapters/in/cli/rollback.go +8 -3


@@ -83,7 +83,13 @@ func fetchAndSortTags(ctx context.Context, client *remote.Client, imageName stri
83	83		return nil, fmt.Errorf("no tags found for %s", imageName)
84	84		}
85	85
86		-	// Separate semver tags from non-semver tags
	86	+	return sortSemverTags(tags), nil
	87	+	}
	88	+
	89	+	// sortSemverTags normalizes, classifies, and sorts tags.
	90	+	// Semver tags (with or without 'v' prefix) are sorted in descending order first,
	91	+	// followed by non-semver tags in descending lexicographic order.
	92	+	func sortSemverTags(tags []string) []string {
87	93		var semverTags, otherTags []string
88	94		for _, tag := range tags {
89	95		sv := tag

@@ -115,8 +121,7 @@ func fetchAndSortTags(ctx context.Context, client *remote.Client, imageName stri
115	121		})
116	122
117	123		// Combine: semver tags first, then other tags
118		-	tags = append(semverTags, otherTags...)
119		-	return tags, nil
	124	+	return append(semverTags, otherTags...)
120	125		}
121	126
122	127		func selectTag(targetTag string, tags []string, currentTag, rollbackDomain string) (string, error) {

 		return nil, fmt.Errorf("no tags found for %s", imageName)
 	}
-	// Separate semver tags from non-semver tags
+	return sortSemverTags(tags), nil
+}
+// sortSemverTags normalizes, classifies, and sorts tags.
+// Semver tags (with or without 'v' prefix) are sorted in descending order first,
+// followed by non-semver tags in descending lexicographic order.
+func sortSemverTags(tags []string) []string {
 	var semverTags, otherTags []string
 	for _, tag := range tags {
 		sv := tag
 	})
 	// Combine: semver tags first, then other tags
-	tags = append(semverTags, otherTags...)
+	return append(semverTags, otherTags...)
-	return tags, nil
 }
 func selectTag(targetTag string, tags []string, currentTag, rollbackDomain string) (string, error) {

M internal/adapters/in/cli/rollback_test.go +6 -38


@@ -6,11 +6,7 @@ import (
6	6		"github.com/stretchr/testify/assert"
7	7		)
8	8
9		-	func TestFetchAndSortTags_SemverNormalization(t *testing.T) {
10		-	// Test the semver tag classification and sorting logic directly
11		-	// by testing validateTagExists and the sort behavior through fetchAndSortTags internals
12		-
13		-	// Test that tags without "v" prefix are still recognized as semver
	9	+	func TestSortSemverTags(t *testing.T) {
14	10		tests := []struct {
15	11		name string
16	12		tags []string

@@ -29,7 +25,7 @@ func TestFetchAndSortTags_SemverNormalization(t *testing.T) {
29	25		{
30	26		name: "mixed v and non-v prefixed",
31	27		tags: []string{"v1.0.0", "2.0.0", "1.5.0"},
32		-	wantTags: []string{"2.0.0", "v1.5.0", "v1.0.0"}, // all recognized as semver, sorted desc
	28	+	wantTags: []string{"2.0.0", "1.5.0", "v1.0.0"}, // all recognized as semver, sorted desc
33	29		},
34	30		{
35	31		name: "pre-release tags",

@@ -45,41 +41,13 @@ func TestFetchAndSortTags_SemverNormalization(t *testing.T) {
45	41
46	42		for _, tt := range tests {
47	43		t.Run(tt.name, func(t *testing.T) {
48		-	// We can't call fetchAndSortTags directly since it needs a remote client,
49		-	// but we can test the sorting logic by extracting it.
50		-	// For now, test validateTagExists as a simpler unit.
	44	+	gotTags := sortSemverTags(tt.tags)
	45	+	assert.Equal(t, tt.wantTags, gotTags)
51	46		})
52	47		}
	48	+	}
53	49
54		-	// At minimum, test validateTagExists
	50	+	func TestValidateTagExists(t *testing.T) {
55	51		assert.True(t, validateTagExists("v1.0.0", []string{"v1.0.0", "v2.0.0"}))
56	52		assert.False(t, validateTagExists("v3.0.0", []string{"v1.0.0", "v2.0.0"}))
57	53		}
58		-
59		-	func TestNormalizeSemver(t *testing.T) {
60		-	// Test the normalization helper used in fetchAndSortTags
61		-	tests := []struct {
62		-	tag string
63		-	wantSV string
64		-	isValid bool
65		-	}{
66		-	{"v1.0.0", "v1.0.0", true},
67		-	{"1.0.0", "v1.0.0", true},
68		-	{"v0.4.2-pre", "v0.4.2-pre", true},
69		-	{"0.4.2-dev", "v0.4.2-dev", true},
70		-	{"latest", "vlatest", false},
71		-	{"dev", "vdev", false},
72		-	}
73		-
74		-	for _, tt := range tests {
75		-	t.Run(tt.tag, func(t *testing.T) {
76		-	sv := tt.tag
77		-	if len(sv) == 0 \|\| sv[0] != 'v' {
78		-	sv = "v" + sv
79		-	}
80		-	assert.Equal(t, tt.wantSV, sv)
81		-	// semver.IsValid is from golang.org/x/mod/semver
82		-	// We just verify the normalization logic is correct
83		-	})
84		-	}
85		-	}


@@ -6,11 +6,7 @@ import (
6		"github.com/stretchr/testify/assert"	6		"github.com/stretchr/testify/assert"
7		)	7		)
8			8
9	-	func TestFetchAndSortTags_SemverNormalization(t *testing.T) {	9	+	func TestSortSemverTags(t *testing.T) {
10	-	// Test the semver tag classification and sorting logic directly
11	-	// by testing validateTagExists and the sort behavior through fetchAndSortTags internals
12	-
13	-	// Test that tags without "v" prefix are still recognized as semver
14		tests := []struct {	10		tests := []struct {
15		name string	11		name string
16		tags []string	12		tags []string

@@ -29,7 +25,7 @@ func TestFetchAndSortTags_SemverNormalization(t *testing.T) {
29		{	25		{
30		name: "mixed v and non-v prefixed",	26		name: "mixed v and non-v prefixed",
31		tags: []string{"v1.0.0", "2.0.0", "1.5.0"},	27		tags: []string{"v1.0.0", "2.0.0", "1.5.0"},
32	-	wantTags: []string{"2.0.0", "v1.5.0", "v1.0.0"}, // all recognized as semver, sorted desc	28	+	wantTags: []string{"2.0.0", "1.5.0", "v1.0.0"}, // all recognized as semver, sorted desc
33		},	29		},
34		{	30		{
35		name: "pre-release tags",	31		name: "pre-release tags",

@@ -45,41 +41,13 @@ func TestFetchAndSortTags_SemverNormalization(t *testing.T) {
45			41
46		for _, tt := range tests {	42		for _, tt := range tests {
47		t.Run(tt.name, func(t *testing.T) {	43		t.Run(tt.name, func(t *testing.T) {
48	-	// We can't call fetchAndSortTags directly since it needs a remote client,	44	+	gotTags := sortSemverTags(tt.tags)
49	-	// but we can test the sorting logic by extracting it.	45	+	assert.Equal(t, tt.wantTags, gotTags)
50	-	// For now, test validateTagExists as a simpler unit.
51		})	46		})
52		}	47		}
			48	+	}
53			49
54	-	// At minimum, test validateTagExists	50	+	func TestValidateTagExists(t *testing.T) {
55		assert.True(t, validateTagExists("v1.0.0", []string{"v1.0.0", "v2.0.0"}))	51		assert.True(t, validateTagExists("v1.0.0", []string{"v1.0.0", "v2.0.0"}))
56		assert.False(t, validateTagExists("v3.0.0", []string{"v1.0.0", "v2.0.0"}))	52		assert.False(t, validateTagExists("v3.0.0", []string{"v1.0.0", "v2.0.0"}))
57		}	53		}
58	-
59	-	func TestNormalizeSemver(t *testing.T) {
60	-	// Test the normalization helper used in fetchAndSortTags
61	-	tests := []struct {
62	-	tag string
63	-	wantSV string
64	-	isValid bool
65	-	}{
66	-	{"v1.0.0", "v1.0.0", true},
67	-	{"1.0.0", "v1.0.0", true},
68	-	{"v0.4.2-pre", "v0.4.2-pre", true},
69	-	{"0.4.2-dev", "v0.4.2-dev", true},
70	-	{"latest", "vlatest", false},
71	-	{"dev", "vdev", false},
72	-	}
73	-
74	-	for _, tt := range tests {
75	-	t.Run(tt.tag, func(t *testing.T) {
76	-	sv := tt.tag
77	-	if len(sv) == 0 \|\| sv[0] != 'v' {
78	-	sv = "v" + sv
79	-	}
80	-	assert.Equal(t, tt.wantSV, sv)
81	-	// semver.IsValid is from golang.org/x/mod/semver
82	-	// We just verify the normalization logic is correct
83	-	})
84	-	}
85	-	}

M internal/adapters/in/http/middleware/logging.go +5 -0


@@ -57,6 +57,11 @@ func (rw *ResponseWriter) Flush() {
57	57		}
58	58		}
59	59
	60	+	// Unwrap returns the underlying ResponseWriter.
	61	+	func (rw *ResponseWriter) Unwrap() http.ResponseWriter {
	62	+	return rw.ResponseWriter
	63	+	}
	64	+
60	65		// RequestLogger is a middleware that logs HTTP requests using zerowrap.
61	66		// It also attaches the logger to the request context for downstream handlers.
62	67		// The trustedNets parameter controls which proxy headers are trusted for IP extraction.


@@ -57,6 +57,11 @@ func (rw *ResponseWriter) Flush() {
57	}	57		}
58	}	58		}
59		59
		60	+	// Unwrap returns the underlying ResponseWriter.
		61	+	func (rw *ResponseWriter) Unwrap() http.ResponseWriter {
		62	+	return rw.ResponseWriter
		63	+	}
		64	+
60	// RequestLogger is a middleware that logs HTTP requests using zerowrap.	65		// RequestLogger is a middleware that logs HTTP requests using zerowrap.
61	// It also attaches the logger to the request context for downstream handlers.	66		// It also attaches the logger to the request context for downstream handlers.
62	// The trustedNets parameter controls which proxy headers are trusted for IP extraction.	67		// The trustedNets parameter controls which proxy headers are trusted for IP extraction.

M internal/app/run.go +5 -1


@@ -384,7 +384,11 @@ func resolveLogFilePath(cfg Config) string {
384	384		if cfg.Logging.File.Path != "" {
385	385		return cfg.Logging.File.Path
386	386		}
387		-	return filepath.Join(cfg.Server.DataDir, "logs", "gordon.log")
	387	+	dataDir := cfg.Server.DataDir
	388	+	if dataDir == "" {
	389	+	dataDir = DefaultDataDir()
	390	+	}
	391	+	return filepath.Join(dataDir, "logs", "gordon.log")
388	392		}
389	393
390	394		// createOutputAdapters creates the Docker runtime and event bus.

 	if cfg.Logging.File.Path != "" {
 		return cfg.Logging.File.Path
 	}
-	return filepath.Join(cfg.Server.DataDir, "logs", "gordon.log")
+	dataDir := cfg.Server.DataDir
+	if dataDir == "" {
+		dataDir = DefaultDataDir()
+	}
+	return filepath.Join(dataDir, "logs", "gordon.log")
 }
 // createOutputAdapters creates the Docker runtime and event bus.