Skip to content

Commit 79a7633

Browse files
vdyevdye
committed
cmd: add configurability to 'Run()'
Change the function signature of 'Run()' to accept command arguments in a '[]string' (rather than variadic args) and add a set of 'cmd.Setting' variadic args to optionally configure the command execution. This configurability will be necessary for cutting over things like 'git.go' (which, for example, sometimes specifies a custom stdin buffer) to use the 'CommandExecutor'. To maintain the old function signatures, also add convenience wrappers 'RunQuiet()' and 'RunStdout()'. Signed-off-by: Victoria Dye <[email protected]>
1 parent 034bafe commit 79a7633

File tree

7 files changed

+136
-31
lines changed

7 files changed

+136
-31
lines changed

‎internal/cmd/command.go

Lines changed: 53 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,17 @@ package cmd
22

33
import (
44
"context"
5+
"io"
6+
"os"
57
"os/exec"
68

79
"github.com/github/git-bundle-server/internal/log"
810
)
911

1012
type CommandExecutor interface {
11-
Run(ctx context.Context, command string, args ...string) (int, error)
13+
RunStdout(ctx context.Context, command string, args ...string) (int, error)
14+
RunQuiet(ctx context.Context, command string, args ...string) (int, error)
15+
Run(ctx context.Context, command string, args []string, settings ...Setting) (int, error)
1216
}
1317

1418
type commandExecutor struct {
@@ -21,15 +25,40 @@ func NewCommandExecutor(l log.TraceLogger) CommandExecutor {
2125
}
2226
}
2327

24-
func (c *commandExecutor) Run(ctx context.Context, command string, args ...string) (int, error) {
28+
func (c *commandExecutor) buildCmd(ctx context.Context, command string, args ...string) (*exec.Cmd, error) {
2529
exe, err := exec.LookPath(command)
2630
if err != nil {
27-
return -1, c.logger.Errorf(ctx, "failed to find '%s' on the path: %w", command, err)
31+
return nil, c.logger.Errorf(ctx, "failed to find '%s' on the path: %w", command, err)
2832
}
2933

3034
cmd := exec.Command(exe, args...)
3135

32-
err = cmd.Start()
36+
return cmd, nil
37+
}
38+
39+
func (c *commandExecutor) applyOptions(ctx context.Context, cmd *exec.Cmd, settings []Setting) {
40+
for _, setting := range settings {
41+
switch setting.Key {
42+
case stdinKey:
43+
cmd.Stdin = setting.Value.(io.Reader)
44+
case stdoutKey:
45+
cmd.Stdout = setting.Value.(io.Writer)
46+
case stderrKey:
47+
cmd.Stderr = setting.Value.(io.Writer)
48+
case envKey:
49+
env, ok := setting.Value.([]string)
50+
if !ok {
51+
panic("incorrect env setting type")
52+
}
53+
cmd.Env = append(cmd.Env, env...)
54+
default:
55+
panic("invalid cmdSettingKey")
56+
}
57+
}
58+
}
59+
60+
func (c *commandExecutor) runCmd(ctx context.Context, cmd *exec.Cmd) (int, error) {
61+
err := cmd.Start()
3362
if err != nil {
3463
return -1, c.logger.Errorf(ctx, "command failed to start: %w", err)
3564
}
@@ -42,6 +71,25 @@ func (c *commandExecutor) Run(ctx context.Context, command string, args ...strin
4271
if err == nil || isExitError {
4372
return cmd.ProcessState.ExitCode(), nil
4473
} else {
45-
return -1, c.logger.Error(ctx, err)
74+
return -1, err
4675
}
4776
}
77+
78+
func (c *commandExecutor) RunStdout(ctx context.Context, command string, args ...string) (int, error) {
79+
return c.Run(ctx, command, args, Stdout(os.Stdout), Stderr(os.Stderr))
80+
}
81+
82+
func (c *commandExecutor) RunQuiet(ctx context.Context, command string, args ...string) (int, error) {
83+
return c.Run(ctx, command, args)
84+
}
85+
86+
func (c *commandExecutor) Run(ctx context.Context, command string, args []string, settings ...Setting) (int, error) {
87+
cmd, err := c.buildCmd(ctx, command, args...)
88+
if err != nil {
89+
return -1, err
90+
}
91+
92+
c.applyOptions(ctx, cmd, settings)
93+
94+
return c.runCmd(ctx, cmd)
95+
}

‎internal/cmd/settings.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
package cmd
2+
3+
import (
4+
"io"
5+
6+
"github.com/github/git-bundle-server/internal/utils"
7+
)
8+
9+
type settingType int
10+
11+
const (
12+
stdinKey settingType = iota
13+
stdoutKey
14+
stderrKey
15+
envKey
16+
)
17+
18+
type Setting utils.KeyValue[settingType, any]
19+
20+
func Stdin(stdin io.Reader) Setting {
21+
return Setting{
22+
stdinKey,
23+
stdin,
24+
}
25+
}
26+
27+
func Stdout(stdout io.Writer) Setting {
28+
return Setting{
29+
stdoutKey,
30+
stdout,
31+
}
32+
}
33+
34+
func Stderr(stderr io.Writer) Setting {
35+
return Setting{
36+
stderrKey,
37+
stderr,
38+
}
39+
}
40+
41+
func Env(env []string) Setting {
42+
return Setting{
43+
envKey,
44+
env,
45+
}
46+
}

‎internal/daemon/launchd.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ func NewLaunchdProvider(
115115

116116
func (l *launchd) isBootstrapped(ctx context.Context, serviceTarget string) (bool, error) {
117117
// run 'launchctl print' on given service target to see if it exists
118-
exitCode, err := l.cmdExec.Run(ctx, "launchctl", "print", serviceTarget)
118+
exitCode, err := l.cmdExec.RunQuiet(ctx, "launchctl", "print", serviceTarget)
119119
if err != nil {
120120
return false, l.logger.Error(ctx, err)
121121
}
@@ -132,7 +132,7 @@ func (l *launchd) isBootstrapped(ctx context.Context, serviceTarget string) (boo
132132

133133
func (l *launchd) bootstrapFile(ctx context.Context, domain string, filename string) error {
134134
// run 'launchctl bootstrap' on given domain & file
135-
exitCode, err := l.cmdExec.Run(ctx, "launchctl", "bootstrap", domain, filename)
135+
exitCode, err := l.cmdExec.RunQuiet(ctx, "launchctl", "bootstrap", domain, filename)
136136
if err != nil {
137137
return l.logger.Error(ctx, err)
138138
}
@@ -146,7 +146,7 @@ func (l *launchd) bootstrapFile(ctx context.Context, domain string, filename str
146146

147147
func (l *launchd) bootout(ctx context.Context, serviceTarget string) (bool, error) {
148148
// run 'launchctl bootout' on given service target
149-
exitCode, err := l.cmdExec.Run(ctx, "launchctl", "bootout", serviceTarget)
149+
exitCode, err := l.cmdExec.RunQuiet(ctx, "launchctl", "bootout", serviceTarget)
150150
if err != nil {
151151
return false, l.logger.Error(ctx, err)
152152
}
@@ -239,7 +239,7 @@ func (l *launchd) Start(ctx context.Context, label string) error {
239239

240240
domainTarget := fmt.Sprintf(domainFormat, user.Uid)
241241
serviceTarget := fmt.Sprintf("%s/%s", domainTarget, label)
242-
exitCode, err := l.cmdExec.Run(ctx, "launchctl", "kickstart", serviceTarget)
242+
exitCode, err := l.cmdExec.RunQuiet(ctx, "launchctl", "kickstart", serviceTarget)
243243
if err != nil {
244244
return l.logger.Error(ctx, err)
245245
}
@@ -259,7 +259,7 @@ func (l *launchd) Stop(ctx context.Context, label string) error {
259259

260260
domainTarget := fmt.Sprintf(domainFormat, user.Uid)
261261
serviceTarget := fmt.Sprintf("%s/%s", domainTarget, label)
262-
exitCode, err := l.cmdExec.Run(ctx, "launchctl", "kill", "SIGINT", serviceTarget)
262+
exitCode, err := l.cmdExec.RunQuiet(ctx, "launchctl", "kill", "SIGINT", serviceTarget)
263263
if err != nil {
264264
return l.logger.Error(ctx, err)
265265
}

‎internal/daemon/launchd_test.go

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -250,21 +250,21 @@ func TestLaunchd_Create(t *testing.T) {
250250
t.Run(fmt.Sprintf("%s (force='%t')", tt.title, force), func(t *testing.T) {
251251
// Mock responses
252252
for _, retVal := range tt.launchctlPrint {
253-
testCommandExecutor.On("Run",
253+
testCommandExecutor.On("RunQuiet",
254254
ctx,
255255
"launchctl",
256256
mock.MatchedBy(func(args []string) bool { return args[0] == "print" }),
257257
).Return(retVal.First, retVal.Second).Once()
258258
}
259259
for _, retVal := range tt.launchctlBootstrap {
260-
testCommandExecutor.On("Run",
260+
testCommandExecutor.On("RunQuiet",
261261
ctx,
262262
"launchctl",
263263
mock.MatchedBy(func(args []string) bool { return args[0] == "bootstrap" }),
264264
).Return(retVal.First, retVal.Second).Once()
265265
}
266266
for _, retVal := range tt.launchctlBootout {
267-
testCommandExecutor.On("Run",
267+
testCommandExecutor.On("RunQuiet",
268268
ctx,
269269
"launchctl",
270270
mock.MatchedBy(func(args []string) bool { return args[0] == "bootout" }),
@@ -307,12 +307,12 @@ func TestLaunchd_Create(t *testing.T) {
307307
var actualFileBytes []byte
308308

309309
// Mock responses for successful fresh write
310-
testCommandExecutor.On("Run",
310+
testCommandExecutor.On("RunQuiet",
311311
ctx,
312312
"launchctl",
313313
mock.MatchedBy(func(args []string) bool { return args[0] == "print" }),
314314
).Return(daemon.LaunchdServiceNotFoundErrorCode, nil).Once()
315-
testCommandExecutor.On("Run",
315+
testCommandExecutor.On("RunQuiet",
316316
ctx,
317317
"launchctl",
318318
mock.MatchedBy(func(args []string) bool { return args[0] == "bootstrap" }),
@@ -378,7 +378,7 @@ func TestLaunchd_Start(t *testing.T) {
378378

379379
// Test #1: launchctl succeeds
380380
t.Run("Calls correct launchctl command", func(t *testing.T) {
381-
testCommandExecutor.On("Run",
381+
testCommandExecutor.On("RunQuiet",
382382
ctx,
383383
"launchctl",
384384
[]string{"kickstart", fmt.Sprintf("user/123/%s", basicDaemonConfig.Label)},
@@ -394,7 +394,7 @@ func TestLaunchd_Start(t *testing.T) {
394394

395395
// Test #2: launchctl fails
396396
t.Run("Returns error when launchctl fails", func(t *testing.T) {
397-
testCommandExecutor.On("Run",
397+
testCommandExecutor.On("RunQuiet",
398398
ctx,
399399
mock.AnythingOfType("string"),
400400
mock.AnythingOfType("[]string"),
@@ -464,7 +464,7 @@ func TestLaunchd_Stop(t *testing.T) {
464464
t.Run(tt.title, func(t *testing.T) {
465465
// Mock responses
466466
if tt.launchctlKill != nil {
467-
testCommandExecutor.On("Run",
467+
testCommandExecutor.On("RunQuiet",
468468
ctx,
469469
"launchctl",
470470
[]string{"kill", "SIGINT", fmt.Sprintf("user/123/%s", tt.label)},
@@ -561,7 +561,7 @@ func TestLaunchd_Remove(t *testing.T) {
561561

562562
// Mock responses
563563
if tt.launchctlBootout != nil {
564-
testCommandExecutor.On("Run",
564+
testCommandExecutor.On("RunQuiet",
565565
ctx,
566566
"launchctl",
567567
[]string{"bootout", fmt.Sprintf("user/123/%s", tt.label)},

‎internal/daemon/systemd.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ func NewSystemdProvider(
4545
}
4646

4747
func (s *systemd) reloadDaemon(ctx context.Context) error {
48-
exitCode, err := s.cmdExec.Run(ctx, "systemctl", "--user", "daemon-reload")
48+
exitCode, err := s.cmdExec.RunQuiet(ctx, "systemctl", "--user", "daemon-reload")
4949
if err != nil {
5050
return s.logger.Error(ctx, err)
5151
}
@@ -105,7 +105,7 @@ func (s *systemd) Create(ctx context.Context, config *DaemonConfig, force bool)
105105

106106
func (s *systemd) Start(ctx context.Context, label string) error {
107107
// TODO: warn user if already running
108-
exitCode, err := s.cmdExec.Run(ctx, "systemctl", "--user", "start", label)
108+
exitCode, err := s.cmdExec.RunQuiet(ctx, "systemctl", "--user", "start", label)
109109
if err != nil {
110110
return s.logger.Error(ctx, err)
111111
}
@@ -119,7 +119,7 @@ func (s *systemd) Start(ctx context.Context, label string) error {
119119

120120
func (s *systemd) Stop(ctx context.Context, label string) error {
121121
// TODO: warn user if already stopped
122-
exitCode, err := s.cmdExec.Run(ctx, "systemctl", "--user", "stop", label)
122+
exitCode, err := s.cmdExec.RunQuiet(ctx, "systemctl", "--user", "stop", label)
123123
if err != nil {
124124
return s.logger.Error(ctx, err)
125125
}

‎internal/daemon/systemd_test.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ func TestSystemd_Create(t *testing.T) {
160160
).Return(retVal).Once()
161161
}
162162
for _, retVal := range tt.systemctlDaemonReload {
163-
testCommandExecutor.On("Run",
163+
testCommandExecutor.On("RunQuiet",
164164
ctx,
165165
"systemctl",
166166
[]string{"--user", "daemon-reload"},
@@ -192,7 +192,7 @@ func TestSystemd_Create(t *testing.T) {
192192
var actualFileBytes []byte
193193

194194
// Mock responses for successful fresh write
195-
testCommandExecutor.On("Run",
195+
testCommandExecutor.On("RunQuiet",
196196
ctx,
197197
"systemctl",
198198
[]string{"--user", "daemon-reload"},
@@ -256,7 +256,7 @@ func TestSystemd_Start(t *testing.T) {
256256

257257
// Test #1: systemctl succeeds
258258
t.Run("Calls correct systemctl command", func(t *testing.T) {
259-
testCommandExecutor.On("Run",
259+
testCommandExecutor.On("RunQuiet",
260260
ctx,
261261
"systemctl",
262262
[]string{"--user", "start", basicDaemonConfig.Label},
@@ -272,7 +272,7 @@ func TestSystemd_Start(t *testing.T) {
272272

273273
// Test #2: systemctl fails
274274
t.Run("Returns error when systemctl fails", func(t *testing.T) {
275-
testCommandExecutor.On("Run",
275+
testCommandExecutor.On("RunQuiet",
276276
ctx,
277277
mock.AnythingOfType("string"),
278278
mock.AnythingOfType("[]string"),
@@ -303,7 +303,7 @@ func TestSystemd_Stop(t *testing.T) {
303303

304304
// Test #1: systemctl succeeds
305305
t.Run("Calls correct systemctl command", func(t *testing.T) {
306-
testCommandExecutor.On("Run",
306+
testCommandExecutor.On("RunQuiet",
307307
ctx,
308308
"systemctl",
309309
[]string{"--user", "stop", basicDaemonConfig.Label},
@@ -319,7 +319,7 @@ func TestSystemd_Stop(t *testing.T) {
319319

320320
// Test #2: systemctl fails with uncaught error
321321
t.Run("Returns error when systemctl fails", func(t *testing.T) {
322-
testCommandExecutor.On("Run",
322+
testCommandExecutor.On("RunQuiet",
323323
ctx,
324324
mock.AnythingOfType("string"),
325325
mock.AnythingOfType("[]string"),
@@ -335,7 +335,7 @@ func TestSystemd_Stop(t *testing.T) {
335335

336336
// Test #3: service unit not found still succeeds
337337
t.Run("Succeeds if service unit not installed", func(t *testing.T) {
338-
testCommandExecutor.On("Run",
338+
testCommandExecutor.On("RunQuiet",
339339
ctx,
340340
mock.AnythingOfType("string"),
341341
mock.AnythingOfType("[]string"),
@@ -413,7 +413,7 @@ func TestSystemd_Remove(t *testing.T) {
413413
).Return(tt.deleteFile.First, tt.deleteFile.Second).Once()
414414
}
415415
if tt.systemctlDaemonReload != nil {
416-
testCommandExecutor.On("Run",
416+
testCommandExecutor.On("RunQuiet",
417417
ctx,
418418
"systemctl",
419419
[]string{"--user", "daemon-reload"},

‎internal/testhelpers/mocks.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"os/user"
77
"runtime"
88

9+
"github.com/github/git-bundle-server/internal/cmd"
910
"github.com/stretchr/testify/mock"
1011
)
1112

@@ -121,11 +122,21 @@ type MockCommandExecutor struct {
121122
mock.Mock
122123
}
123124

124-
func (m *MockCommandExecutor) Run(ctx context.Context, command string, args ...string) (int, error) {
125+
func (m *MockCommandExecutor) RunStdout(ctx context.Context, command string, args ...string) (int, error) {
125126
fnArgs := m.Called(ctx, command, args)
126127
return fnArgs.Int(0), fnArgs.Error(1)
127128
}
128129

130+
func (m *MockCommandExecutor) RunQuiet(ctx context.Context, command string, args ...string) (int, error) {
131+
fnArgs := m.Called(ctx, command, args)
132+
return fnArgs.Int(0), fnArgs.Error(1)
133+
}
134+
135+
func (m *MockCommandExecutor) Run(ctx context.Context, command string, args []string, settings ...cmd.Setting) (int, error) {
136+
fnArgs := m.Called(ctx, command, args, settings)
137+
return fnArgs.Int(0), fnArgs.Error(1)
138+
}
139+
129140
type MockFileSystem struct {
130141
mock.Mock
131142
}

0 commit comments

Comments
 (0)