diff --git a/docs/executioners/shell_executioner.md b/docs/executioners/shell_executioner.md index b8d6646..956c77f 100644 --- a/docs/executioners/shell_executioner.md +++ b/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:** diff --git a/internal/goverseer/executioner/shell_executioner/shell_executioner.go b/internal/goverseer/executioner/shell_executioner/shell_executioner.go index d39db9d..c6ffc54 100644 --- a/internal/goverseer/executioner/shell_executioner/shell_executioner.go +++ b/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,15 +191,21 @@ 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 @@ -188,51 +213,34 @@ func (e *ShellExecutioner) writeToWorkDir(execWorkDir, name string, data interfa // 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 { diff --git a/internal/goverseer/executioner/shell_executioner/shell_executioner_test.go b/internal/goverseer/executioner/shell_executioner/shell_executioner_test.go index b8d630f..5034670 100644 --- a/internal/goverseer/executioner/shell_executioner/shell_executioner_test.go +++ b/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 diff --git a/internal/goverseer/watcher/time_watcher/time_watcher.go b/internal/goverseer/watcher/time_watcher/time_watcher.go index 63950d5..5416c88 100644 --- a/internal/goverseer/watcher/time_watcher/time_watcher.go +++ b/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() } } } diff --git a/internal/goverseer/watcher/time_watcher/time_watcher_test.go b/internal/goverseer/watcher/time_watcher/time_watcher_test.go index c0ccd0d..80e51f4 100644 --- a/internal/goverseer/watcher/time_watcher/time_watcher_test.go +++ b/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") }