fix: address PR review comments from Copilot

- Use sentence case for proxy error messages (proxy/service.go)
- Rollback route if Save() fails in AddRoute (config/service.go)
- Improve ensureImage comment to clarify return values (container/service.go)
- Handle file.Close() error for writable files (admin/secrets.go)
- Add missing viper defaults for config options (app/run.go)
- Update test to use temp config file for AddRoute

commit: ee1db199f3ae9bf0f5ce56597e77af682bdfec11
author: bnema · 9mo ago · 2026-01-16 21:18 UTC
parent: d22c734
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

6 files · +40 -7

M internal/adapters/in/http/admin/secrets.go +6 -2


@@ -127,7 +127,7 @@ func (h *Handler) deleteSecret(domain, key string) error {
127	127		}
128	128
129	129		// writeSecrets writes all secrets to the domain's env file.
130		-	func (h *Handler) writeSecrets(domain string, secrets map[string]string) error {
	130	+	func (h *Handler) writeSecrets(domain string, secrets map[string]string) (err error) {
131	131		envFile := h.domainToEnvFile(domain)
132	132
133	133		// Create/truncate file with secure permissions

@@ -135,7 +135,11 @@ func (h *Handler) writeSecrets(domain string, secrets map[string]string) error {
135	135		if err != nil {
136	136		return fmt.Errorf("failed to create env file: %w", err)
137	137		}
138		-	defer file.Close()
	138	+	defer func() {
	139	+	if cerr := file.Close(); cerr != nil && err == nil {
	140	+	err = fmt.Errorf("failed to close env file: %w", cerr)
	141	+	}
	142	+	}()
139	143
140	144		// Write header comment
141	145		if _, err := fmt.Fprintf(file, "# Environment variables for %s\n", domain); err != nil {

 }
 // writeSecrets writes all secrets to the domain's env file.
-func (h *Handler) writeSecrets(domain string, secrets map[string]string) error {
+func (h *Handler) writeSecrets(domain string, secrets map[string]string) (err error) {
 	envFile := h.domainToEnvFile(domain)
 	// Create/truncate file with secure permissions
 	if err != nil {
 		return fmt.Errorf("failed to create env file: %w", err)
 	}
-	defer file.Close()
+	defer func() {
+		if cerr := file.Close(); cerr != nil && err == nil {
+			err = fmt.Errorf("failed to close env file: %w", cerr)
+		}
+	}()
 	// Write header comment
 	if _, err := fmt.Fprintf(file, "# Environment variables for %s\n", domain); err != nil {

M internal/app/run.go +7 -0


@@ -1098,6 +1098,13 @@ func loadConfig(v *viper.Viper, configPath string) error {
1098	1098		v.SetDefault("registry_auth.enabled", false)
1099	1099		v.SetDefault("registry_auth.type", "password")
1100	1100		v.SetDefault("registry_auth.token_expiry", "720h")
	1101	+	v.SetDefault("auto_route.enabled", false)
	1102	+	v.SetDefault("network_isolation.enabled", false)
	1103	+	v.SetDefault("network_isolation.network_prefix", "gordon")
	1104	+	v.SetDefault("network_isolation.dns_suffix", ".internal")
	1105	+	v.SetDefault("volumes.auto_create", true)
	1106	+	v.SetDefault("volumes.prefix", "gordon")
	1107	+	v.SetDefault("volumes.preserve", true)
1101	1108		v.SetDefault("deploy.pull_policy", container.PullPolicyIfTagChanged)
1102	1109
1103	1110		ConfigureViper(v, configPath)


@@ -1098,6 +1098,13 @@ func loadConfig(v *viper.Viper, configPath string) error {
1098	v.SetDefault("registry_auth.enabled", false)	1098		v.SetDefault("registry_auth.enabled", false)
1099	v.SetDefault("registry_auth.type", "password")	1099		v.SetDefault("registry_auth.type", "password")
1100	v.SetDefault("registry_auth.token_expiry", "720h")	1100		v.SetDefault("registry_auth.token_expiry", "720h")
		1101	+	v.SetDefault("auto_route.enabled", false)
		1102	+	v.SetDefault("network_isolation.enabled", false)
		1103	+	v.SetDefault("network_isolation.network_prefix", "gordon")
		1104	+	v.SetDefault("network_isolation.dns_suffix", ".internal")
		1105	+	v.SetDefault("volumes.auto_create", true)
		1106	+	v.SetDefault("volumes.prefix", "gordon")
		1107	+	v.SetDefault("volumes.preserve", true)
1101	v.SetDefault("deploy.pull_policy", container.PullPolicyIfTagChanged)	1108		v.SetDefault("deploy.pull_policy", container.PullPolicyIfTagChanged)
1102		1109
1103	ConfigureViper(v, configPath)	1110		ConfigureViper(v, configPath)

M internal/usecase/config/service.go +12 -2


@@ -185,16 +185,26 @@ func (s *Service) AddRoute(ctx context.Context, route domain.Route) error {
185	185		})
186	186		log := zerowrap.FromCtx(ctx)
187	187
	188	+	// Store previous value for rollback
188	189		s.mu.Lock()
	190	+	previousImage, existed := s.config.Routes[route.Domain]
189	191		if s.config.Routes == nil {
190	192		s.config.Routes = make(map[string]string)
191	193		}
192	194		s.config.Routes[route.Domain] = route.Image
193	195		s.mu.Unlock()
194	196
195		-	// Persist to disk
	197	+	// Persist to disk - rollback on failure
196	198		if err := s.Save(ctx); err != nil {
197		-	log.Warn().Err(err).Msg("failed to persist route to disk")
	199	+	log.Warn().Err(err).Msg("failed to persist route to disk, rolling back")
	200	+	s.mu.Lock()
	201	+	if existed {
	202	+	s.config.Routes[route.Domain] = previousImage
	203	+	} else {
	204	+	delete(s.config.Routes, route.Domain)
	205	+	}
	206	+	s.mu.Unlock()
	207	+	return err
198	208		}
199	209
200	210		log.Info().Str("image", route.Image).Msg("route added to configuration")

 	})
 	log := zerowrap.FromCtx(ctx)
+	// Store previous value for rollback
 	s.mu.Lock()
+	previousImage, existed := s.config.Routes[route.Domain]
 	if s.config.Routes == nil {
 		s.config.Routes = make(map[string]string)
 	}
 	s.config.Routes[route.Domain] = route.Image
 	s.mu.Unlock()
-	// Persist to disk
+	// Persist to disk - rollback on failure
 	if err := s.Save(ctx); err != nil {
-		log.Warn().Err(err).Msg("failed to persist route to disk")
+		log.Warn().Err(err).Msg("failed to persist route to disk, rolling back")
+		s.mu.Lock()
+		if existed {
+			s.config.Routes[route.Domain] = previousImage
+		} else {
+			delete(s.config.Routes, route.Domain)
+		}
+		s.mu.Unlock()
+		return err
 	}
 	log.Info().Str("image", route.Image).Msg("route added to configuration")

M internal/usecase/config/service_test.go +11 -1


@@ -2,11 +2,14 @@ package config
2	2
3	3		import (
4	4		"context"
	5	+	"os"
	6	+	"path/filepath"
5	7		"testing"
6	8
7	9		"github.com/bnema/zerowrap"
8	10		"github.com/spf13/viper"
9	11		"github.com/stretchr/testify/assert"
	12	+	"github.com/stretchr/testify/require"
10	13
11	14		"gordon/internal/boundaries/out/mocks"
12	15		"gordon/internal/domain"

@@ -96,7 +99,14 @@ func TestService_GetRoutes(t *testing.T) {
96	99		}
97	100
98	101		func TestService_AddRoute(t *testing.T) {
	102	+	// Create temp config file for Save() to work
	103	+	tmpDir := t.TempDir()
	104	+	configFile := filepath.Join(tmpDir, "gordon.toml")
	105	+	err := os.WriteFile(configFile, []byte("[routes]\n"), 0600)
	106	+	require.NoError(t, err)
	107	+
99	108		v := viper.New()
	109	+	v.SetConfigFile(configFile)
100	110		eventBus := mocks.NewMockEventPublisher(t)
101	111		svc := NewService(v, eventBus)
102	112		ctx := testContext()

@@ -108,7 +118,7 @@ func TestService_AddRoute(t *testing.T) {
108	118		Image: "newapp:latest",
109	119		}
110	120
111		-	err := svc.AddRoute(ctx, route)
	121	+	err = svc.AddRoute(ctx, route)
112	122
113	123		assert.NoError(t, err)
114	124

 import (
 	"context"
+	"os"
+	"path/filepath"
 	"testing"
 	"github.com/bnema/zerowrap"
 	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"gordon/internal/boundaries/out/mocks"
 	"gordon/internal/domain"
 }
 func TestService_AddRoute(t *testing.T) {
+	// Create temp config file for Save() to work
+	tmpDir := t.TempDir()
+	configFile := filepath.Join(tmpDir, "gordon.toml")
+	err := os.WriteFile(configFile, []byte("[routes]\n"), 0600)
+	require.NoError(t, err)
 	v := viper.New()
+	v.SetConfigFile(configFile)
 	eventBus := mocks.NewMockEventPublisher(t)
 	svc := NewService(v, eventBus)
 	ctx := testContext()
 		Image:  "newapp:latest",
 	}
-	err := svc.AddRoute(ctx, route)
+	err = svc.AddRoute(ctx, route)
 	assert.NoError(t, err)

M internal/usecase/container/service.go +3 -1


@@ -545,7 +545,9 @@ func (s *Service) pullRefForDeploy(ctx context.Context, imageRef string) (string
545	545		}
546	546
547	547		// ensureImage ensures the image is available locally, pulling if needed.
548		-	// Returns the image reference to use for container operations.
	548	+	// Returns the image reference to use for container operations:
	549	+	// - For digest references (@sha256:...), returns the pullRef since Docker can't tag digests
	550	+	// - For tagged images, returns the original imageRef after tagging the pulled image
549	551		func (s *Service) ensureImage(ctx context.Context, imageRef string) (string, error) {
550	552		ctx = zerowrap.CtxWithField(ctx, "image", imageRef)
551	553		log := zerowrap.FromCtx(ctx)

 }
 // ensureImage ensures the image is available locally, pulling if needed.
-// Returns the image reference to use for container operations.
+// Returns the image reference to use for container operations:
+//   - For digest references (@sha256:...), returns the pullRef since Docker can't tag digests
+//   - For tagged images, returns the original imageRef after tagging the pulled image
 func (s *Service) ensureImage(ctx context.Context, imageRef string) (string, error) {
 	ctx = zerowrap.CtxWithField(ctx, "image", imageRef)
 	log := zerowrap.FromCtx(ctx)

M internal/usecase/proxy/service.go +1 -1


@@ -307,7 +307,7 @@ func (s Service) proxyToTarget(w http.ResponseWriter, r http.Request, target *
307	307
308	308		proxy := newReverseProxy(targetURL,
309	309		func(w http.ResponseWriter, _ *http.Request, err error) {
310		-	log.Error().Err(err).Str("target", targetURL.String()).Msg("PROXY ERROR - connection failed")
	310	+	log.Error().Err(err).Str("target", targetURL.String()).Msg("proxy error: connection failed")
311	311		http.Error(w, "Service Unavailable", http.StatusServiceUnavailable)
312	312		},
313	313		func(resp *http.Response) error {


@@ -307,7 +307,7 @@ func (s Service) proxyToTarget(w http.ResponseWriter, r http.Request, target *
307			307
308		proxy := newReverseProxy(targetURL,	308		proxy := newReverseProxy(targetURL,
309		func(w http.ResponseWriter, _ *http.Request, err error) {	309		func(w http.ResponseWriter, _ *http.Request, err error) {
310	-	log.Error().Err(err).Str("target", targetURL.String()).Msg("PROXY ERROR - connection failed")	310	+	log.Error().Err(err).Str("target", targetURL.String()).Msg("proxy error: connection failed")
311		http.Error(w, "Service Unavailable", http.StatusServiceUnavailable)	311		http.Error(w, "Service Unavailable", http.StatusServiceUnavailable)
312		},	312		},
313		func(resp *http.Response) error {	313		func(resp *http.Response) error {