Refactor validation for alert and monitor to return errors

alert.go

@@ -37,13 +37,32 @@ type AlertNotice struct {
 	LastCheckOutput string
 }
 
-// IsValid returns a boolean indicating if the Alert has been correctly
+// Validate checks that the Alert is properly configured and returns errors if not
-// configured
+func (alert Alert) Validate() error {
-func (alert Alert) IsValid() bool {
+	hasCommand := len(alert.Command) > 0
-	hasAtLeastOneCommand := alert.Command != nil || alert.ShellCommand != ""
+	hasShellCommand := alert.ShellCommand != ""
-	hasAtMostOneCommand := alert.Command == nil || alert.ShellCommand == ""
 
-	return hasAtLeastOneCommand && hasAtMostOneCommand
+	var err error
+
+	hasAtLeastOneCommand := hasCommand || hasShellCommand
+	if !hasAtLeastOneCommand {
+		err = errors.Join(err, fmt.Errorf(
+			"%w: alert %s has no command or shell_command configured",
+			ErrInvalidAlert,
+			alert.Name,
+		))
+	}
+
+	hasAtMostOneCommand := !(hasCommand && hasShellCommand)
+	if !hasAtMostOneCommand {
+		err = errors.Join(err, fmt.Errorf(
+			"%w: alert %s has both command and shell_command configured",
+			ErrInvalidAlert,
+			alert.Name,
+		))
+	}
+
+	return err
 }
 
 // BuildTemplates compiles command templates for the Alert

alert_test.go

@@ -1,20 +1,24 @@
 package main_test
 
 import (
+	"errors"
 	"testing"
 
 	m "git.iamthefij.com/iamthefij/minitor-go"
 )
 
-func TestAlertIsValid(t *testing.T) {
+func TestAlertValidate(t *testing.T) {
+	t.Parallel()
+
 	cases := []struct {
 		alert    m.Alert
-		expected bool
+		expected error
 		name     string
 	}{
-		{m.Alert{Command: []string{"echo", "test"}}, true, "Command only"},
+		{m.Alert{Command: []string{"echo", "test"}}, nil, "Command only"},
-		{m.Alert{ShellCommand: "echo test"}, true, "CommandShell only"},
+		{m.Alert{ShellCommand: "echo test"}, nil, "CommandShell only"},
-		{m.Alert{}, false, "No commands"},
+		{m.Alert{Command: []string{"echo", "test"}, ShellCommand: "echo test"}, m.ErrInvalidAlert, "Both commands"},
+		{m.Alert{}, m.ErrInvalidAlert, "No commands"},
 	}
 
 	for _, c := range cases {
@@ -23,8 +27,11 @@ func TestAlertIsValid(t *testing.T) {
 		t.Run(c.name, func(t *testing.T) {
 			t.Parallel()
 
-			actual := c.alert.IsValid()
+			actual := c.alert.Validate()
-			if actual != c.expected {
+			hasErr := (actual != nil)
+			expectErr := (c.expected != nil)
+
+			if hasErr != expectErr || !errors.Is(actual, c.expected) {
 				t.Errorf("expected=%t actual=%t", c.expected, actual)
 			}
 		})

config.go

@@ -73,9 +73,7 @@ func (config Config) IsValid() error {
 	}
 
 	for _, alert := range config.Alerts {
-		if !alert.IsValid() {
+		err = errors.Join(err, alert.Validate())
-			err = errors.Join(err, fmt.Errorf("%w: %s", ErrInvalidAlert, alert.Name))
-		}
 	}
 
 	// Validate monitors
@@ -84,9 +82,7 @@ func (config Config) IsValid() error {
 	}
 
 	for _, monitor := range config.Monitors {
-		if !monitor.IsValid() {
+		err = errors.Join(err, monitor.Validate())
-			err = errors.Join(err, fmt.Errorf("%w: %s", ErrInvalidMonitor, monitor.Name))
-		}
 
 		// Check that all Monitor alerts actually exist
 		for _, isUp := range []bool{true, false} {

monitor.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"errors"
 	"fmt"
 	"math"
 	"os/exec"
@@ -65,21 +66,51 @@ func (monitor *Monitor) Init(defaultAlertAfter int, defaultAlertEvery *int, defa
 	return nil
 }
 
-// IsValid returns a boolean indicating if the Monitor has been correctly configured
+// Validate checks that the Monitor is properly configured and returns errors if not
-func (monitor Monitor) IsValid() bool {
+func (monitor Monitor) Validate() error {
-	// TODO: Refactor and return an error containing more information on what was invalid
 	hasCommand := len(monitor.Command) > 0
 	hasShellCommand := monitor.ShellCommand != ""
 	hasValidAlertAfter := monitor.AlertAfter > 0
 	hasAlertDown := len(monitor.AlertDown) > 0
 
-	hasAtLeastOneCommand := hasCommand || hasShellCommand
+	var err error
-	hasAtMostOneCommand := !(hasCommand && hasShellCommand)
 
-	return hasAtLeastOneCommand &&
+	hasAtLeastOneCommand := hasCommand || hasShellCommand
-		hasAtMostOneCommand &&
+	if !hasAtLeastOneCommand {
-		hasValidAlertAfter &&
+		err = errors.Join(err, fmt.Errorf(
-		hasAlertDown
+			"%w: monitor %s has no command or shell_command configured",
+			ErrInvalidMonitor,
+			monitor.Name,
+		))
+	}
+
+	hasAtMostOneCommand := !(hasCommand && hasShellCommand)
+	if !hasAtMostOneCommand {
+		err = errors.Join(err, fmt.Errorf(
+			"%w: monitor %s has both command and shell_command configured",
+			ErrInvalidMonitor,
+			monitor.Name,
+		))
+	}
+
+	if !hasValidAlertAfter {
+		err = errors.Join(err, fmt.Errorf(
+			"%w: monitor %s has invalid alert_after value %d. Must be greater than 0",
+			ErrInvalidMonitor,
+			monitor.Name,
+			monitor.AlertAfter,
+		))
+	}
+
+	if !hasAlertDown {
+		err = errors.Join(err, fmt.Errorf(
+			"%w: monitor %s has no alert_down configured. Configure one here or add a default_alert_down",
+			ErrInvalidMonitor,
+			monitor.Name,
+		))
+	}
+
+	return err
 }
 
 func (monitor Monitor) LastOutput() string {

monitor_test.go

@@ -1,6 +1,7 @@
 package main_test
 
 import (
+	"errors"
 	"reflect"
 	"testing"
 	"time"
@@ -8,18 +9,19 @@ import (
 	m "git.iamthefij.com/iamthefij/minitor-go"
 )
 
-// TestMonitorIsValid tests the Monitor.IsValid()
+func TestMonitorValidate(t *testing.T) {
-func TestMonitorIsValid(t *testing.T) {
+	t.Parallel()
+
 	cases := []struct {
 		monitor  m.Monitor
-		expected bool
+		expected error
 		name     string
 	}{
-		{m.Monitor{AlertAfter: 1, Command: []string{"echo", "test"}, AlertDown: []string{"log"}}, true, "Command only"},
+		{m.Monitor{AlertAfter: 1, Command: []string{"echo", "test"}, AlertDown: []string{"log"}}, nil, "Command only"},
-		{m.Monitor{AlertAfter: 1, ShellCommand: "echo test", AlertDown: []string{"log"}}, true, "CommandShell only"},
+		{m.Monitor{AlertAfter: 1, ShellCommand: "echo test", AlertDown: []string{"log"}}, nil, "CommandShell only"},
-		{m.Monitor{AlertAfter: 1, Command: []string{"echo", "test"}}, false, "No AlertDown"},
+		{m.Monitor{AlertAfter: 1, Command: []string{"echo", "test"}}, m.ErrInvalidMonitor, "No AlertDown"},
-		{m.Monitor{AlertAfter: 1, AlertDown: []string{"log"}}, false, "No commands"},
+		{m.Monitor{AlertAfter: 1, AlertDown: []string{"log"}}, m.ErrInvalidMonitor, "No commands"},
-		{m.Monitor{AlertAfter: -1, Command: []string{"echo", "test"}, AlertDown: []string{"log"}}, false, "Invalid alert threshold, -1"},
+		{m.Monitor{AlertAfter: -1, Command: []string{"echo", "test"}, AlertDown: []string{"log"}}, m.ErrInvalidMonitor, "Invalid alert threshold, -1"},
 	}
 
 	for _, c := range cases {
@@ -28,8 +30,11 @@ func TestMonitorIsValid(t *testing.T) {
 		t.Run(c.name, func(t *testing.T) {
 			t.Parallel()
 
-			actual := c.monitor.IsValid()
+			actual := c.monitor.Validate()
-			if actual != c.expected {
+			hasErr := (actual != nil)
+			expectErr := (c.expected != nil)
+
+			if hasErr != expectErr || !errors.Is(actual, c.expected) {
 				t.Errorf("IsValid(%v), expected=%t actual=%t", c.name, c.expected, actual)
 			}
 		})