Merge pull request #84 from Icinga/go-yaml-compat

`logging#Config`: Fix `Options` can no longer be parsed by `go-yaml`

Author: lippserd

logging/config.go

@@ -2,6 +2,7 @@ package logging
 
 import (
 	"fmt"
+	"github.com/creasty/defaults"
 	"github.com/pkg/errors"
 	"go.uber.org/zap/zapcore"
 	"os"
@@ -38,39 +39,52 @@ func (o *Options) UnmarshalText(text []byte) error {
 	return nil
 }
 
+// UnmarshalYAML implements yaml.InterfaceUnmarshaler to allow Options to be parsed go-yaml.
+func (o *Options) UnmarshalYAML(unmarshal func(any) error) error {
+	optionsMap := make(map[string]zapcore.Level)
+
+	if err := unmarshal(&optionsMap); err != nil {
+		return err
+	}
+
+	*o = optionsMap
+
+	return nil
+}
+
 // Config defines Logger configuration.
 type Config struct {
 	// zapcore.Level at 0 is for info level.
 	Level  zapcore.Level `yaml:"level" env:"LEVEL" default:"0"`
 	Output string        `yaml:"output" env:"OUTPUT"`
 	// Interval for periodic logging.
 	Interval time.Duration `yaml:"interval" env:"INTERVAL" default:"20s"`
-
-	Options `yaml:"options" env:"OPTIONS"`
+	Options  Options       `yaml:"options" env:"OPTIONS"`
 }
 
-// Validate checks constraints in the configuration and returns an error if they are violated.
-// Also configures the log output if it is not configured:
+// SetDefaults implements defaults.Setter to configure the log output if it is not set:
 // systemd-journald is used when Icinga DB is running under systemd, otherwise stderr.
-func (l *Config) Validate() error {
-	if l.Interval <= 0 {
-		return errors.New("periodic logging interval must be positive")
-	}
-
-	if l.Output == "" {
+func (c *Config) SetDefaults() {
+	if defaults.CanUpdate(c.Output) {
 		if _, ok := os.LookupEnv("NOTIFY_SOCKET"); ok {
 			// When started by systemd, NOTIFY_SOCKET is set by systemd for Type=notify supervised services,
 			// which is the default setting for the Icinga DB service.
 			// This assumes that Icinga DB is running under systemd, so set output to systemd-journald.
-			l.Output = JOURNAL
+			c.Output = JOURNAL
 		} else {
 			// Otherwise set it to console, i.e. write log messages to stderr.
-			l.Output = CONSOLE
+			c.Output = CONSOLE
 		}
 	}
+}
+
+// Validate checks constraints in the configuration and returns an error if they are violated.
+func (c *Config) Validate() error {
+	if c.Interval <= 0 {
+		return errors.New("periodic logging interval must be positive")
+	}
 
-	// To be on the safe side, always call AssertOutput.
-	return AssertOutput(l.Output)
+	return AssertOutput(c.Output)
 }
 
 // AssertOutput returns an error if output is not a valid logger output.

logging/config_test.go

@@ -1,52 +1,82 @@
 package logging
 
 import (
+	"fmt"
+	"github.com/creasty/defaults"
 	"github.com/icinga/icinga-go-library/config"
+	"github.com/icinga/icinga-go-library/testutils"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/zap/zapcore"
+	"os"
 	"testing"
 	"time"
 )
 
 func TestConfig(t *testing.T) {
-	subtests := []struct {
-		name     string
-		opts     config.EnvOptions
-		expected Config
-		error    bool
-	}{
+	var defaultConfig Config
+	require.NoError(t, defaults.Set(&defaultConfig), "setting default config")
+
+	configTests := []testutils.TestCase[Config, testutils.ConfigTestData]{
+		{
+			Name: "Defaults",
+			Data: testutils.ConfigTestData{
+				// An empty YAML file causes an error,
+				// so specify a valid key without a value to trigger fallback to the default.
+				Yaml: `level:`,
+			},
+			Expected: defaultConfig,
+		},
 		{
-			name: "empty",
-			opts: config.EnvOptions{},
-			expected: Config{
-				Output:   "console",
-				Interval: 20 * time.Second,
+			Name: "periodic logging interval must be positive",
+			Data: testutils.ConfigTestData{
+				Yaml: `interval: 0s`,
+				Env:  map[string]string{"INTERVAL": "0s"},
 			},
+			Error: testutils.ErrorContains("periodic logging interval must be positive"),
 		},
 		{
-			name:  "invalid-output",
-			opts:  config.EnvOptions{Environment: map[string]string{"OUTPUT": "☃"}},
-			error: true,
+			Name: "invalid logger output",
+			Data: testutils.ConfigTestData{
+				Yaml: `output: invalid`,
+				Env:  map[string]string{"OUTPUT": "invalid"},
+			},
+			Error: testutils.ErrorContains("invalid is not a valid logger output"),
 		},
 		{
-			name: "customized",
-			opts: config.EnvOptions{Environment: map[string]string{
-				"LEVEL":    zapcore.DebugLevel.String(),
-				"OUTPUT":   JOURNAL,
-				"INTERVAL": "3m14s",
-			}},
-			expected: Config{
+			Name: "Customized",
+			Data: testutils.ConfigTestData{
+				Yaml: fmt.Sprintf(
+					`
+level: debug
+output: %s
+interval: 3m14s`,
+					JOURNAL,
+				),
+				Env: map[string]string{
+					"LEVEL":    zapcore.DebugLevel.String(),
+					"OUTPUT":   JOURNAL,
+					"INTERVAL": "3m14s",
+				},
+			},
+			Expected: Config{
 				Level:    zapcore.DebugLevel,
 				Output:   JOURNAL,
 				Interval: 3*time.Minute + 14*time.Second,
 			},
 		},
 		{
-			name: "options",
-			opts: config.EnvOptions{Environment: map[string]string{"OPTIONS": "foo:debug,bar:info,buz:panic"}},
-			expected: Config{
-				Output:   "console",
-				Interval: 20 * time.Second,
+			Name: "Options",
+			Data: testutils.ConfigTestData{
+				Yaml: `
+options:
+  foo: debug
+  bar: info
+  buz: panic`,
+				Env: map[string]string{"OPTIONS": "foo:debug,bar:info,buz:panic"},
+			},
+			Expected: Config{
+				Output:   defaultConfig.Output,
+				Interval: defaultConfig.Interval,
 				Options: map[string]zapcore.Level{
 					"foo": zapcore.DebugLevel,
 					"bar": zapcore.InfoLevel,
@@ -55,21 +85,41 @@ func TestConfig(t *testing.T) {
 			},
 		},
 		{
-			name:  "options-invalid-levels",
-			opts:  config.EnvOptions{Environment: map[string]string{"OPTIONS": "foo:foo,bar:0"}},
-			error: true,
+			Name: "Options with invalid level",
+			Data: testutils.ConfigTestData{
+				Yaml: `
+options:
+  foo: foo`,
+				Env: map[string]string{"OPTIONS": "foo:foo"},
+			},
+			Error: testutils.ErrorContains(`unrecognized level: "foo"`),
 		},
 	}
 
-	for _, test := range subtests {
-		t.Run(test.name, func(t *testing.T) {
-			var out Config
-			if err := config.FromEnv(&out, test.opts); test.error {
-				require.Error(t, err)
-			} else {
-				require.NoError(t, err)
-				require.Equal(t, test.expected, out)
-			}
-		})
-	}
+	t.Run("FromEnv", func(t *testing.T) {
+		for _, tc := range configTests {
+			t.Run(tc.Name, tc.F(func(data testutils.ConfigTestData) (Config, error) {
+				var actual Config
+
+				err := config.FromEnv(&actual, config.EnvOptions{Environment: data.Env})
+
+				return actual, err
+			}))
+		}
+	})
+
+	t.Run("FromYAMLFile", func(t *testing.T) {
+		for _, tc := range configTests {
+			t.Run(tc.Name+"/FromYAMLFile", tc.F(func(data testutils.ConfigTestData) (Config, error) {
+				var actual Config
+
+				var err error
+				testutils.WithYAMLFile(t, data.Yaml, func(file *os.File) {
+					err = config.FromYAMLFile(file.Name(), &actual)
+				})
+
+				return actual, err
+			}))
+		}
+	})
 }

testutils/testutils.go

@@ -15,7 +15,7 @@ import (
 // TestCase represents a generic test case structure.
 // It is parameterized by T, the type of the expected result, and D, the type of the test data.
 // This struct is useful for defining test cases with expected outcomes and associated data.
-type TestCase[T comparable, D any] struct {
+type TestCase[T any, D any] struct {
 	// Name is the identifier for the test case, used for reporting purposes.
 	Name string
 	// Expected is the anticipated result of the test. It should be left empty if an error is expected.