fix(config): ensure log file path is set

Fix a bug introduced in 6952516 which led to the app being unable to
start if logging was enabled but no explicit path was set. In this case,
the expected behaviour is to fallback to a log file in the XDG file
hierarchy, but this was lost due to broken config file defaults
handling.

This commit separates the behaviour when setting defaults when reading
an existing configuration, from those set when creating a brand new
configuration.

go.mod

@@ -6,6 +6,7 @@ require (
 	github.com/docker/docker v28.0.1+incompatible
 	github.com/docker/go-connections v0.5.0
 	github.com/gdamore/tcell/v2 v2.8.1
+	github.com/google/go-cmp v0.7.0
 	github.com/opencontainers/image-spec v1.1.1
 	github.com/rivo/tview v0.0.0-20241227133733-17b7edb88c57
 	github.com/stretchr/testify v1.10.0

internal/config/config.go

@@ -1,5 +1,7 @@
 package config
 
+import "cmp"
+
 // Destination holds the configuration for a destination.
 type Destination struct {
 	Name string `yaml:"name"`
@@ -10,6 +12,14 @@ type Destination struct {
 type LogFile struct {
 	Enabled bool   `yaml:"enabled"`
 	Path    string `yaml:"path,omitempty"`
+
+	defaultPath string
+}
+
+// GetPath returns the path to the log file. If the path is not set, it
+// returns the default log path.
+func (l LogFile) GetPath() string {
+	return cmp.Or(l.Path, l.defaultPath)
 }
 
 // RTMPSource holds the configuration for the RTMP source.

internal/config/service.go

@@ -47,6 +47,7 @@ func NewService(configDirFunc ConfigDirFunc, chanSize int) (*Service, error) {
 		return nil, fmt.Errorf("app config dir: %w", err)
 	}
 
+	// TODO: inject StateDirFunc
 	appStateDir, err := createAppStateDir()
 	if err != nil {
 		return nil, fmt.Errorf("app state dir: %w", err)
@@ -58,7 +59,7 @@ func NewService(configDirFunc ConfigDirFunc, chanSize int) (*Service, error) {
 		configC:      make(chan Config, chanSize),
 	}
 
-	svc.setDefaults(&svc.current)
+	svc.populateConfigOnBuild(&svc.current)
 
 	return svc, nil
 }
@@ -127,6 +128,7 @@ func (s *Service) readConfig() (cfg Config, _ error) {
 		return cfg, fmt.Errorf("unmarshal: %w", err)
 	}
 
+	s.populateConfigOnRead(&cfg)
 	if err = validate(cfg); err != nil {
 		return cfg, err
 	}
@@ -138,7 +140,7 @@ func (s *Service) readConfig() (cfg Config, _ error) {
 
 func (s *Service) writeDefaultConfig() (Config, error) {
 	var cfg Config
-	s.setDefaults(&cfg)
+	s.populateConfigOnBuild(&cfg)
 
 	cfgBytes, err := marshalConfig(cfg)
 	if err != nil {
@@ -175,10 +177,24 @@ func (s *Service) writeConfig(cfgBytes []byte) error {
 	return nil
 }
 
-// setDefaults is called to set default values for a new, empty configuration.
+// populateConfigOnBuild is called to set default values for a new, empty
-func (s *Service) setDefaults(cfg *Config) {
+// configuration.
+//
+// This function may set exported fields to arbitrary values.
+func (s *Service) populateConfigOnBuild(cfg *Config) {
 	cfg.Sources.RTMP.Enabled = true
 	cfg.Sources.RTMP.StreamKey = "live"
+
+	s.populateConfigOnRead(cfg)
+}
+
+// populateConfigOnRead is called to set default values for a configuration
+// read from an existing file.
+//
+// This function should not update any exported values, which would be a
+// confusing experience for the user.
+func (s *Service) populateConfigOnRead(cfg *Config) {
+	cfg.LogFile.defaultPath = filepath.Join(s.appStateDir, "octoplex.log")
 }
 
 // TODO: validate URL format

internal/config/service_test.go

@@ -4,10 +4,13 @@ import (
 	_ "embed"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"git.netflux.io/rob/octoplex/internal/config"
 	"git.netflux.io/rob/octoplex/internal/shortid"
+	gocmp "github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gopkg.in/yaml.v3"
@@ -19,6 +22,9 @@ var configComplete []byte
 //go:embed testdata/logfile.yml
 var configLogfile []byte
 
+//go:embed testdata/no-logfile.yml
+var configNoLogfile []byte
+
 //go:embed testdata/invalid-destination-url.yml
 var configInvalidDestinationURL []byte
 
@@ -49,7 +55,8 @@ func TestConfigServiceCreateConfig(t *testing.T) {
 
 	cfg, err := service.ReadOrCreateConfig()
 	require.NoError(t, err)
-	require.Empty(t, cfg.LogFile, "expected no log file")
+	require.False(t, cfg.LogFile.Enabled, "expected logging to be disabled")
+	require.Empty(t, cfg.LogFile.Path, "expected no log file")
 
 	p := filepath.Join(systemConfigDir, "octoplex", "config.yaml")
 	cfgBytes, err := os.ReadFile(p)
@@ -71,26 +78,31 @@ func TestConfigServiceReadConfig(t *testing.T) {
 			name:        "complete",
 			configBytes: configComplete,
 			want: func(t *testing.T, cfg config.Config) {
-				require.Equal(
+				require.Empty(
 					t,
-					config.Config{
+					gocmp.Diff(
-						LogFile: config.LogFile{
+						config.Config{
-							Enabled: true,
+							LogFile: config.LogFile{
-							Path:    "test.log",
+								Enabled: true,
-						},
+								Path:    "test.log",
-						Sources: config.Sources{
+							},
-							RTMP: config.RTMPSource{
+							Sources: config.Sources{
-								Enabled:   true,
+								RTMP: config.RTMPSource{
-								StreamKey: "s3cr3t",
+									Enabled:   true,
+									StreamKey: "s3cr3t",
+								},
+							},
+							Destinations: []config.Destination{
+								{
+									Name: "my stream",
+									URL:  "rtmp://rtmp.example.com:1935/live",
+								},
 							},
 						},
-						Destinations: []config.Destination{
+						cfg,
-							{
+						cmpopts.IgnoreUnexported(config.LogFile{}),
-								Name: "my stream",
+					),
-								URL:  "rtmp://rtmp.example.com:1935/live",
+				)
-							},
-						},
-					}, cfg)
 			},
 		},
 		{
@@ -100,6 +112,13 @@ func TestConfigServiceReadConfig(t *testing.T) {
 				assert.Equal(t, "/tmp/octoplex.log", cfg.LogFile.Path)
 			},
 		},
+		{
+			name:        "logging enabled, no logfile",
+			configBytes: configNoLogfile,
+			want: func(t *testing.T, cfg config.Config) {
+				assert.True(t, strings.HasSuffix(cfg.LogFile.GetPath(), "/octoplex/octoplex.log"), "expected %q to end with /tmp/octoplex.log", cfg.LogFile.GetPath())
+			},
+		},
 		{
 			name:        "invalid destination URL",
 			configBytes: configInvalidDestinationURL,

internal/config/testdata/no-logfile.yml

@@ -0,0 +1,5 @@
+---
+logfile:
+  enabled: true
+destinations:
+- url: rtmp://rtmp.example.com:1935/live

main.go

@@ -162,7 +162,7 @@ func buildLogger(cfg config.LogFile) (*slog.Logger, error) {
 		return slog.New(slog.DiscardHandler), nil
 	}
 
-	fptr, err := os.OpenFile(cfg.Path, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666)
+	fptr, err := os.OpenFile(cfg.GetPath(), os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666)
 	if err != nil {
 		return nil, fmt.Errorf("error opening log file: %w", err)
 	}