From 266a9307d2d42b1dbdcc899dd9f903b4661b3567 Mon Sep 17 00:00:00 2001
From: Rob Watson <rob@netflux.io>
Date: Fri, 4 Apr 2025 20:49:05 +0200
Subject: [PATCH] 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 | 1 +
internal/config/config.go | 10 +++++
internal/config/service.go | 24 +++++++++--
internal/config/service_test.go | 55 +++++++++++++++++--------
internal/config/testdata/no-logfile.yml | 5 +++
main.go | 2 +-
6 files changed, 74 insertions(+), 23 deletions(-)
create mode 100644 internal/config/testdata/no-logfile.yml
diff --git a/go.mod b/go.mod
index c3bb887..798f8e9 100644
--- a/go.mod
+++ b/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
diff --git a/internal/config/config.go b/internal/config/config.go
index bc3247b..c1a3daf 100644
--- a/internal/config/config.go
+++ b/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.
diff --git a/internal/config/service.go b/internal/config/service.go
index 6b7fd1c..3169759 100644
--- a/internal/config/service.go
+++ b/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.
-func (s *Service) setDefaults(cfg *Config) {
+// populateConfigOnBuild is called to set default values for a new, empty
+// 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
diff --git a/internal/config/service_test.go b/internal/config/service_test.go
index c68daf2..e719c69 100644
--- a/internal/config/service_test.go
+++ b/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{
- LogFile: config.LogFile{
- Enabled: true,
- Path: "test.log",
- },
- Sources: config.Sources{
- RTMP: config.RTMPSource{
- Enabled: true,
- StreamKey: "s3cr3t",
+ gocmp.Diff(
+ config.Config{
+ LogFile: config.LogFile{
+ Enabled: true,
+ Path: "test.log",
+ },
+ Sources: config.Sources{
+ RTMP: config.RTMPSource{
+ Enabled: true,
+ StreamKey: "s3cr3t",
+ },
+ },
+ Destinations: []config.Destination{
+ {
+ Name: "my stream",
+ URL: "rtmp://rtmp.example.com:1935/live",
+ },
},
},
- Destinations: []config.Destination{
- {
- Name: "my stream",
- URL: "rtmp://rtmp.example.com:1935/live",
- },
- },
- }, cfg)
+ cfg,
+ cmpopts.IgnoreUnexported(config.LogFile{}),
+ ),
+ )
},
},
{
@@ -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,
diff --git a/internal/config/testdata/no-logfile.yml b/internal/config/testdata/no-logfile.yml
new file mode 100644
index 0000000..1825093
--- /dev/null
+++ b/internal/config/testdata/no-logfile.yml
@@ -0,0 +1,5 @@
+---
+logfile:
+ enabled: true
+destinations:
+- url: rtmp://rtmp.example.com:1935/live
diff --git a/main.go b/main.go
index c242e17..c77860c 100644
--- a/main.go
+++ b/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)
}