web-server: add '--remove' option to 'stop'

vdye · vdye · commit 69518f0ddf98 · 2023-02-02T11:19:49.000-08:00
Signed-off-by: Victoria Dye &lt;vdye@github.com&gt;
diff --git a/cmd/git-bundle-server/web-server.go b/cmd/git-bundle-server/web-server.go
@@ -145,7 +145,8 @@ func (w *webServer) startServer(args []string) error {
 
 func (w *webServer) stopServer(args []string) error {
 	// Parse subcommand arguments
-	parser := argparse.NewArgParser("git-bundle-server web-server stop")
+	parser := argparse.NewArgParser("git-bundle-server web-server stop [--remove]")
+	remove := parser.Bool("remove", false, "Remove the web server daemon configuration from the system after stopping")
 	parser.Parse(args)
 
 	d, err := daemon.NewDaemonProvider(w.user, w.cmdExec, w.fileSystem)
@@ -163,6 +164,13 @@ func (w *webServer) stopServer(args []string) error {
 		return err
 	}
 
+	if *remove {
+		err = d.Remove(config.Label)
+		if err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
 
diff --git a/internal/common/filesystem.go b/internal/common/filesystem.go
@@ -6,11 +6,13 @@ import (
 	"fmt"
 	"os"
 	"path"
+	"syscall"
 )
 
 type FileSystem interface {
 	FileExists(filename string) (bool, error)
 	WriteFile(filename string, content []byte) error
+	DeleteFile(filename string) (bool, error)
 	ReadFileLines(filename string) ([]string, error)
 }
 
@@ -46,6 +48,20 @@ func (f *fileSystem) WriteFile(filename string, content []byte) error {
 	return nil
 }
 
+func (f *fileSystem) DeleteFile(filename string) (bool, error) {
+	err := os.Remove(filename)
+	if err == nil {
+		return true, nil
+	}
+
+	pathErr, ok := err.(*os.PathError)
+	if ok && pathErr.Err == syscall.ENOENT {
+		return false, nil
+	} else {
+		return false, err
+	}
+}
+
 func (f *fileSystem) ReadFileLines(filename string) ([]string, error) {
 	file, err := os.OpenFile(filename, os.O_RDONLY|os.O_CREATE, 0o600)
 	if err != nil {
diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go
@@ -20,6 +20,8 @@ type DaemonProvider interface {
 	Start(label string) error
 
 	Stop(label string) error
+
+	Remove(label string) error
 }
 
 func NewDaemonProvider(
diff --git a/internal/daemon/launchd.go b/internal/daemon/launchd.go
@@ -261,3 +261,24 @@ func (l *launchd) Stop(label string) error {
 
 	return nil
 }
+
+func (l *launchd) Remove(label string) error {
+	user, err := l.user.CurrentUser()
+	if err != nil {
+		return fmt.Errorf("could not get current user for launchd service: %w", err)
+	}
+	filename := filepath.Join(user.HomeDir, "Library", "LaunchAgents", fmt.Sprintf("%s.plist", label))
+	domainTarget := fmt.Sprintf(domainFormat, user.Uid)
+
+	err = l.bootoutFile(domainTarget, filename)
+	if err != nil {
+		return fmt.Errorf("could not unload launchd service: %w", err)
+	}
+
+	_, err = l.fileSystem.DeleteFile(filename)
+	if err != nil {
+		return fmt.Errorf("could not delete launchd plist: %w", err)
+	}
+
+	return nil
+}
diff --git a/internal/daemon/launchd_test.go b/internal/daemon/launchd_test.go
@@ -426,7 +426,7 @@ func TestLaunchd_Stop(t *testing.T) {
 	// Reset the mock structure between tests
 	testCommandExecutor.Mock = mock.Mock{}
 
-	// Test #3: launchctl fails with uncaught error
+	// Test #3: launchctl fails with expected error
 	t.Run("Exits without error if service not found", func(t *testing.T) {
 		testCommandExecutor.On("Run",
 			mock.AnythingOfType("string"),
@@ -438,3 +438,102 @@ func TestLaunchd_Stop(t *testing.T) {
 		mock.AssertExpectationsForObjects(t, testCommandExecutor)
 	})
 }
+
+var launchdRemoveTests = []struct {
+	title string
+
+	// Inputs
+	label string
+
+	// Mocked responses
+	launchctlBootout *Pair[int, error]
+	deleteFile       *Pair[bool, error]
+
+	// Expected values
+	expectErr bool
+}{
+	{
+		"Unloads and deletes plist when service loaded",
+		"com.test.service",
+		PtrTo(NewPair[int, error](0, nil)),     // launchctl bootout
+		PtrTo(NewPair[bool, error](true, nil)), // delete file
+		false,
+	},
+	{
+		"Removes plist when service is missing",
+		"com.test.service",
+		PtrTo(NewPair[int, error](daemon.LaunchdServiceNotFoundErrorCode, nil)), // launchctl bootout
+		PtrTo(NewPair[bool, error](true, nil)),                                  // delete file
+		false,
+	},
+	{
+		"Removal of non-existent service succeeds",
+		"com.test.service",
+		PtrTo(NewPair[int, error](daemon.LaunchdServiceNotFoundErrorCode, nil)), // launchctl bootout
+		PtrTo(NewPair[bool, error](false, nil)),                                 // delete file
+		false,
+	},
+	{
+		"launchctl error code skips file deletion",
+		"com.test.service",
+		PtrTo(NewPair[int, error](-1, nil)), // launchctl bootout
+		nil,                                 // delete file
+		true,
+	},
+	{
+		"Unhandled launchctl error skips file deletion",
+		"com.test.service",
+		PtrTo(NewPair(0, fmt.Errorf("some unhandled error"))), // launchctl bootout
+		nil, // delete file
+		true,
+	},
+}
+
+func TestLaunchd_Remove(t *testing.T) {
+	// Set up mocks
+	testUser := &user.User{
+		Uid:      "123",
+		Username: "testuser",
+		HomeDir:  "/my/test/dir",
+	}
+	testUserProvider := &MockUserProvider{}
+	testUserProvider.On("CurrentUser").Return(testUser, nil)
+
+	testCommandExecutor := &MockCommandExecutor{}
+	testFileSystem := &MockFileSystem{}
+
+	launchd := daemon.NewLaunchdProvider(testUserProvider, testCommandExecutor, testFileSystem)
+
+	for _, tt := range launchdRemoveTests {
+		t.Run(tt.title, func(t *testing.T) {
+			// Setup expected values
+			expectedFilename := filepath.Clean(fmt.Sprintf("/my/test/dir/Library/LaunchAgents/%s.plist", tt.label))
+
+			// Mock responses
+			if tt.launchctlBootout != nil {
+				testCommandExecutor.On("Run",
+					"launchctl",
+					[]string{"bootout", "gui/123", expectedFilename},
+				).Return(tt.launchctlBootout.First, tt.launchctlBootout.Second).Once()
+			}
+			if tt.deleteFile != nil {
+				testFileSystem.On("DeleteFile",
+					expectedFilename,
+				).Return(tt.deleteFile.First, tt.deleteFile.Second).Once()
+			}
+
+			// Call function
+			err := launchd.Remove(tt.label)
+			mock.AssertExpectationsForObjects(t, testCommandExecutor)
+			if tt.expectErr {
+				assert.NotNil(t, err)
+			} else {
+				assert.Nil(t, err)
+			}
+		})
+
+		// Reset the mocks between tests
+		testCommandExecutor.Mock = mock.Mock{}
+		testFileSystem.Mock = mock.Mock{}
+	}
+}
diff --git a/internal/daemon/systemd.go b/internal/daemon/systemd.go
@@ -36,6 +36,19 @@ func NewSystemdProvider(
 	}
 }
 
+func (s *systemd) reloadDaemon() error {
+	exitCode, err := s.cmdExec.Run("systemctl", "--user", "daemon-reload")
+	if err != nil {
+		return err
+	}
+
+	if exitCode != 0 {
+		return fmt.Errorf("'systemctl --user daemon-reload' exited with status %d", exitCode)
+	}
+
+	return nil
+}
+
 func (s *systemd) Create(config *DaemonConfig, force bool) error {
 	user, err := s.user.CurrentUser()
 	if err != nil {
@@ -73,16 +86,12 @@ func (s *systemd) Create(config *DaemonConfig, force bool) error {
 		return fmt.Errorf("unable to write service unit: %w", err)
 	}
 
-	// Reload the user-scoped service units
-	exitCode, err := s.cmdExec.Run("systemctl", "--user", "daemon-reload")
+	// Reload the user-scoped service units after adding
+	err = s.reloadDaemon()
 	if err != nil {
 		return err
 	}
 
-	if exitCode != 0 {
-		return fmt.Errorf("'systemctl --user daemon-reload' exited with status %d", exitCode)
-	}
-
 	return nil
 }
 
@@ -113,3 +122,24 @@ func (s *systemd) Stop(label string) error {
 
 	return nil
 }
+
+func (s *systemd) Remove(label string) error {
+	user, err := s.user.CurrentUser()
+	if err != nil {
+		return fmt.Errorf("could not get current user for launchd service: %w", err)
+	}
+	filename := filepath.Join(user.HomeDir, ".config", "systemd", "user", fmt.Sprintf("%s.service", label))
+
+	_, err = s.fileSystem.DeleteFile(filename)
+	if err != nil {
+		return fmt.Errorf("could not delete service unit: %w", err)
+	}
+
+	// Reload the user-scoped service units after removing
+	err = s.reloadDaemon()
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
diff --git a/internal/daemon/systemd_test.go b/internal/daemon/systemd_test.go
@@ -314,3 +314,88 @@ func TestSystemd_Stop(t *testing.T) {
 		mock.AssertExpectationsForObjects(t, testCommandExecutor)
 	})
 }
+
+var systemdRemoveTests = []struct {
+	title string
+
+	// Inputs
+	label string
+
+	// Mocked responses
+	deleteFile            *Pair[bool, error]
+	systemctlDaemonReload *Pair[int, error]
+
+	// Expected values
+	expectErr bool
+}{
+	{
+		"Unloads and deletes service unit",
+		"com.test.service",
+		PtrTo(NewPair[bool, error](true, nil)), // delete file
+		PtrTo(NewPair[int, error](0, nil)),     // systemctl daemon-reload
+		false,
+	},
+	{
+		"Reloads daemon even if service unit missing",
+		"com.test.service",
+		PtrTo(NewPair[bool, error](false, nil)), // delete file
+		PtrTo(NewPair[int, error](0, nil)),      // systemctl daemon-reload
+		false,
+	},
+	{
+		"Daemon not reloaded if file cannot be deleted",
+		"com.test.service",
+		PtrTo(NewPair(false, fmt.Errorf("unhandled error"))), // delete file
+		nil, // systemctl daemon-reload
+		true,
+	},
+}
+
+func TestSystemd_Remove(t *testing.T) {
+	// Set up mocks
+	testUser := &user.User{
+		Uid:      "123",
+		Username: "testuser",
+		HomeDir:  "/my/test/dir",
+	}
+	testUserProvider := &MockUserProvider{}
+	testUserProvider.On("CurrentUser").Return(testUser, nil)
+
+	testCommandExecutor := &MockCommandExecutor{}
+	testFileSystem := &MockFileSystem{}
+
+	systemd := daemon.NewSystemdProvider(testUserProvider, testCommandExecutor, testFileSystem)
+
+	for _, tt := range systemdRemoveTests {
+		t.Run(tt.title, func(t *testing.T) {
+			// Setup expected values
+			expectedFilename := filepath.Clean(fmt.Sprintf("/my/test/dir/.config/systemd/user/%s.service", tt.label))
+
+			// Mock responses
+			if tt.deleteFile != nil {
+				testFileSystem.On("DeleteFile",
+					expectedFilename,
+				).Return(tt.deleteFile.First, tt.deleteFile.Second).Once()
+			}
+			if tt.systemctlDaemonReload != nil {
+				testCommandExecutor.On("Run",
+					"systemctl",
+					[]string{"--user", "daemon-reload"},
+				).Return(tt.systemctlDaemonReload.First, tt.systemctlDaemonReload.Second).Once()
+			}
+
+			// Call function
+			err := systemd.Remove(tt.label)
+			mock.AssertExpectationsForObjects(t, testCommandExecutor)
+			if tt.expectErr {
+				assert.NotNil(t, err)
+			} else {
+				assert.Nil(t, err)
+			}
+		})
+
+		// Reset the mocks between tests
+		testCommandExecutor.Mock = mock.Mock{}
+		testFileSystem.Mock = mock.Mock{}
+	}
+}
diff --git a/internal/testhelpers/funcs.go b/internal/testhelpers/funcs.go
@@ -0,0 +1,9 @@
+package testhelpers
+
+/********************************************/
+/************* Helper Functions *************/
+/********************************************/
+
+func PtrTo[T any](val T) *T {
+	return &val
+}
diff --git a/internal/testhelpers/mocks.go b/internal/testhelpers/mocks.go
@@ -38,6 +38,11 @@ func (m *MockFileSystem) WriteFile(filename string, content []byte) error {
 	return fnArgs.Error(0)
 }
 
+func (m *MockFileSystem) DeleteFile(filename string) (bool, error) {
+	fnArgs := m.Called(filename)
+	return fnArgs.Bool(0), fnArgs.Error(1)
+}
+
 func (m *MockFileSystem) ReadFileLines(filename string) ([]string, error) {
 	fnArgs := m.Called(filename)
 	return fnArgs.Get(0).([]string), fnArgs.Error(1)

Original file line number	Diff line number	Diff line change
`@@ -20,6 +20,8 @@ type DaemonProvider interface {`
`20`	`20`	`Start(label string) error`
`21`	`21`
`22`	`22`	`Stop(label string) error`
	`23`	`+`
	`24`	`+ Remove(label string) error`
`23`	`25`	`}`
`24`	`26`
`25`	`27`	`func NewDaemonProvider(`