Fixes for ShellExecutioner

docs/executioners/shell_executioner.md

@@ -15,7 +15,7 @@ file. The following configuration options are available:
 - `command`: This is the shell command you want to execute.For example,
   `echo "Data received: $GOVERSEER_DATA"`.
 - `shell`: (Optional) This specifies the shell to use for executing the command.
-  Defaults to `/bin/sh` if not provided.
+  Defaults to `/bin/sh -ec` if not provided.
 - `work_dir`: (Optional) This specifies the directory where the executioner
   stores the data file. Defaults to the `/tmp` if not provided.
 - `persist_data`: (Optional) This determines whether the command and data will
@@ -34,8 +34,9 @@ watcher:
 executioner:
   type: shell
   config:
-    command: echo "Data received: $GOVERSEER_DATA"
-    shell: /bin/bash
+    shell: /bin/bash -euo pipefail -c
+    command: |
+      echo "Data received: $GOVERSEER_DATA"
 ```
 
 **Note:**

internal/goverseer/executioner/shell_executioner/shell_executioner.go

@@ -7,6 +7,7 @@ import (
 	"io"
 	"os"
 	"os/exec"
+	"strings"
 
 	"github.com/charmbracelet/log"
 	"github.com/simplifi/goverseer/internal/goverseer/config"
@@ -18,7 +19,7 @@ const (
 	DataEnvVarName = "GOVERSEER_DATA"
 
 	// DefaultShell is the default shell to use when executing a command
-	DefaultShell = "/bin/sh"
+	DefaultShell = "/bin/sh -ec"
 
 	// DefaultWorkDir is the default value for the work directory
 	DefaultWorkDir = "/tmp"
@@ -34,16 +35,16 @@ type Config struct {
 	Command string
 
 	// Shell is the shell to use when executing the command
+	// Options can also be passed to the shell here
 	Shell string
 
 	// WorkDir is the directory in which the ShellExecutioner will store
-	// the command to run and the data to pass into the command
+	// the data to pass into the command
 	WorkDir string
 
-	// PersistWorkDir determines whether the command and data will persist after
-	// completion
+	// PersistWorkDir determines whether the data will persist after completion
 	// This can be useful to enable when troubleshooting configured commands but
-	// should generally remain disabled otherwise
+	// should generally remain disabled
 	PersistData bool
 }
 
@@ -146,8 +147,26 @@ func New(cfg config.Config) (*ShellExecutioner, error) {
 	}, nil
 }
 
+func (e *ShellExecutioner) enableOutputStreaming(cmd *exec.Cmd) error {
+	// Stream stdout of the command to the logger
+	stdOut, err := cmd.StdoutPipe()
+	if err != nil {
+		return fmt.Errorf("error creating stdout pipe: %w", err)
+	}
+	go e.streamOutput(stdOut, log.InfoLevel)
+
+	// Stream stderr of the command to the logger
+	stdErr, err := cmd.StderrPipe()
+	if err != nil {
+		return fmt.Errorf("error creating stderr pipe: %w", err)
+	}
+	go e.streamOutput(stdErr, log.ErrorLevel)
+
+	return nil
+}
+
 // streamOutput streams the output of a pipe to the logger
-func (e *ShellExecutioner) streamOutput(pipe io.ReadCloser) {
+func (e *ShellExecutioner) streamOutput(pipe io.ReadCloser, logLevel log.Level) {
 	scanner := bufio.NewScanner(pipe)
 	for {
 		select {
@@ -156,7 +175,7 @@ func (e *ShellExecutioner) streamOutput(pipe io.ReadCloser) {
 			return
 		default:
 			if scanner.Scan() {
-				log.Info("command", "output", scanner.Text())
+				log.Log(logLevel, "command", "output", scanner.Text())
 			} else {
 				if err := scanner.Err(); err != nil {
 					// Avoid logging errors if the context was canceled mid-scan
@@ -172,67 +191,56 @@ func (e *ShellExecutioner) streamOutput(pipe io.ReadCloser) {
 	}
 }
 
-// writeToWorkDir writes the data to a file in the temporary work directory
+// writeTempData writes the data to a file in the temporary work directory
 // It returns the path to the file and an error if the data could not be written
-func (e *ShellExecutioner) writeToWorkDir(execWorkDir, name string, data interface{}) (string, error) {
-	filePath := fmt.Sprintf("%s/%s", execWorkDir, name)
-	if err := os.WriteFile(filePath, []byte(data.(string)), 0644); err != nil {
-		return "", fmt.Errorf("error writing file to work dir: %w", err)
+func (e *ShellExecutioner) writeTempData(data interface{}) (string, error) {
+	tempDataFile, err := os.CreateTemp(e.WorkDir, "goverseer")
+	if err != nil {
+		return "", fmt.Errorf("error creating temp file: %w", err)
+	}
+	defer tempDataFile.Close()
+
+	if _, err := tempDataFile.WriteString(data.(string)); err != nil {
+		return "", fmt.Errorf("error writing data to temp file: %w", err)
 	}
-	log.Info("wrote file to work dir", "path", filePath)
-	return filePath, nil
+
+	log.Info("wrote data to work dir", "path", tempDataFile.Name())
+	return tempDataFile.Name(), nil
 }
 
 // Execute runs the command with the given data
 // It returns an error if the command could not be started or if the command
 // returned an error.
 // The data is written to a temp file and the path is passed to the command via
 // the DataEnvVarName environment variable.
-// The command is started in the configured shell.
+// The command is run in the configured shell.
 func (e *ShellExecutioner) Execute(data interface{}) error {
-	var execWorkDir, dataPath, commandPath string
+	var tempDataPath string
 	var err error
 
-	// Create a temp directory to store the command and data
-	if execWorkDir, err = os.MkdirTemp(e.WorkDir, "goverseer"); err != nil {
-		return fmt.Errorf("error creating work dir: %w", err)
+	// Write the data passed in from the watcher to a temp file in the work dir
+	if tempDataPath, err = e.writeTempData(data); err != nil {
+		return fmt.Errorf("error writing data: %w", err)
 	}
 
 	if e.PersistData {
-		log.Warn("persisting data", "path", execWorkDir)
+		log.Warn("persisting data", "path", tempDataPath)
 	} else {
-		defer os.RemoveAll(execWorkDir)
+		defer os.Remove(tempDataPath)
 	}
 
-	// Write the data to a file in the work directory
-	if dataPath, err = e.writeToWorkDir(execWorkDir, "data", data); err != nil {
-		return fmt.Errorf("error writing data to work dir: %w", err)
-	}
+	// Build the command to run
+	// Split the Shell so we can pass the args to exec.Command the way it expects
+	// Pass the path to the data file via the DataEnvVarName environment variable
+	shellParts := strings.Split(e.Shell, " ")
+	cmd := exec.CommandContext(e.ctx, shellParts[0], append(shellParts[1:], e.Command)...)
+	cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", DataEnvVarName, tempDataPath))
 
-	// Write the command to a file in the work directory
-	if commandPath, err = e.writeToWorkDir(execWorkDir, "command", e.Command); err != nil {
-		return fmt.Errorf("error writing command to work dir: %w", err)
+	// Stream command output to the logger
+	if err := e.enableOutputStreaming(cmd); err != nil {
+		return fmt.Errorf("error enabling output streaming: %w", err)
 	}
 
-	// Build the command
-	cmd := exec.CommandContext(e.ctx, e.Shell, commandPath)
-	cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", DataEnvVarName, dataPath))
-
-	// Handle output from command
-	combinedOutput, err := cmd.StdoutPipe()
-	if err != nil {
-		return fmt.Errorf("error creating output pipe: %w", err)
-	}
-	defer combinedOutput.Close()
-
-	// Redirect stderr to stdout
-	cmd.Stderr = cmd.Stdout
-
-	// Stream combined output to the logger
-	go func() {
-		e.streamOutput(combinedOutput)
-	}()
-
 	// Start the command running
 	// This does not block and depends on the caller to call cmd.Wait()
 	if err := cmd.Start(); err != nil {

internal/goverseer/executioner/shell_executioner/shell_executioner_test.go

@@ -33,6 +33,15 @@ func TestParseConfig(t *testing.T) {
 	assert.Equal(t, "/bin/bash", parsedConfig.Shell,
 		"Shell should be set to the value in the config")
 
+	parsedConfig, err = ParseConfig(map[string]interface{}{
+		"command": "echo 123",
+		"shell":   "/bin/bash -euo pipefail -c",
+	})
+	assert.NoError(t, err,
+		"Parsing a config with shell options not return an error")
+	assert.Equal(t, "/bin/bash -euo pipefail -c", parsedConfig.Shell,
+		"Shell should be set to the value in the config")
+
 	parsedConfig, err = ParseConfig(map[string]interface{}{
 		"command": "echo 123",
 		"shell":   nil,
@@ -178,8 +187,13 @@ func TestShellExecutioner_Execute(t *testing.T) {
 	assert.NoError(t, err,
 		"Executing a valid command multiple times should not return an error")
 
-	// Test data persistance
+	// Test shell options
+	executioner.Shell = "/bin/bash -euo pipefail -c"
+	err = executioner.Execute("test_shell_opts")
+	assert.NoError(t, err,
+		"Executing a command with shell options should not return an error")
 
+	// Test data persistance
 	testWorkDir := t.TempDir()
 	executioner.PersistData = true
 	executioner.WorkDir = testWorkDir

internal/goverseer/watcher/time_watcher/time_watcher.go

@@ -67,7 +67,6 @@ func New(cfg config.Config) (*TimeWatcher, error) {
 }
 
 // Watch ticks at regular intervals, sending the time to the changes channel
-// The changes channel is where the path to the file is sent when it changes
 func (w *TimeWatcher) Watch(change chan interface{}) {
 	log.Info("starting watcher")
 	for {
@@ -76,7 +75,7 @@ func (w *TimeWatcher) Watch(change chan interface{}) {
 			return
 		case value := <-time.After(time.Duration(w.PollSeconds) * time.Second):
 			log.Info("time watcher tick", "value", value)
-			change <- value
+			change <- value.String()
 		}
 	}
 }

internal/goverseer/watcher/time_watcher/time_watcher_test.go

@@ -76,6 +76,8 @@ func TestTimeWatcher_Watch(t *testing.T) {
 	select {
 	case value := <-changes:
 		assert.NotEmpty(t, value)
+		// assert that the value is a string
+		assert.IsType(t, "", value)
 	case <-time.After(2 * time.Second):
 		assert.Fail(t, "Timed out waiting for file change")
 	}