Improve argument parsing & implement the `web-server` command #9

vdye · 2023-01-14T00:33:55Z

Overview

This pull request is made up of two main changes (the second depends on the first):

Create a unified interface for specifying and parsing arguments, including subcommands and positional arguments (patches 1-7, up to bundle-server: update commands to use arg parser)
- Because git-bundle-server web-server uses both flags & subcommands, it seemed like a good opportunity to improve argument parsing across the repository 😁
- The first two commits perform the necessary updates to the command interface type (changing the name of the Subcommand interface & adding the description() with information pulled mostly from README.md)
- The next three commits implement the arg parser: first basic flag parsing, then subcommand parsing, then positional argument parsing.
- The last two commits update main.go and each subcommand to use the arg parser
Implement the git-bundle-server web-server (start|stop) command for MacOS and Linux
- This subcommand uses launchd or systemd (depending on the OS) to start or stop the git-bundle-web-server.
- The first three commits implement "wrapper" providers for common functionality to-be-used when configuring the daemons. They use a particular design pattern (public interface, private type) to ensure they are mockable, which will be critical when unit testing the daemon providers.
- The next three commits implement the launchd Create(), Start(), and Stop() functions and unit tests those functions (using the mocked interfaces from the prior commits).
- The next three commits implement the systemd functions, including unit testing.
- The next commit adds a wrapper to git-bundle-web-server to intercept SIGTERM and SIGINT signals to trigger a graceful shutdown & cleanup of resources. SIGKILL will still ungracefully shut down, however.
- The last commit implements the git-bundle-server web-server (start|stop) command. It generates the configuration needed by the daemon provider (finds the path to git-bundle-web-server by first searching on the PATH, then in the same directory as the running git-bundle-server executable), then creates and invokes the appropriate daemon provider.

Testing

I did some basic manual testing on both MacOS (local machine) and Linux (Ubuntu VM), and the services started & stopped successfully. Proper integration tests are needed to verify edge-case behavior and ensure we don't regress over time (it's on the project's TODO list 😉).

Leftover bits

TOCTOU protections (particularly for launchd/launchctl)
Log warning whether a service is already running/stopped when running web-server (start|stop)
Avoiding writing the service configuration file when the on-disk contents match the to-be-written contents

derrickstolee

I read through all of the commits, occasionally looking at the code in my IDE.

As a Go newbie myself, I was mostly looking for things that caught my eye as an interesting pattern and trying to think about it as something that will apply to the other code paths untouched by this PR. In every case, I found your choices here to be solid and ready for moving forward.

You've also done a great job establishing our first unit tests and a framework for creating such tests in the future. After this, there is room for parallel work in shoring up our test coverage of existing functionality.

cmd/git-bundle-server/main.go

derrickstolee · 2023-01-17T15:31:07Z

internal/daemon/launchd.go

+func NewLaunchdProvider(
+	u common.UserProvider,
+	c common.CommandExecutor,
+	fs common.FileSystem,


Good dependency injection!

derrickstolee · 2023-01-17T15:40:24Z

internal/daemon/launchd_test.go

+	"github.com/stretchr/testify/mock"
+)
+
+var launchdCreateTests = []struct {


I really like this pattern. I found it easier to read in my IDE.

derrickstolee · 2023-01-17T15:47:05Z

internal/daemon/launchd_test.go

+	for _, tt := range launchdCreateTests {
+		forceArg := tt.force.toBoolList()
+		for _, force := range forceArg {
+			t.Run(fmt.Sprintf("%s (force='%t')", tt.title, force), func(t *testing.T) {


This pattern of t.Run() to launch "subtests" is new to me. But I like this pattern for mock-checks, with expansions to more involved tests when the generic structure can't describe the situation.

derrickstolee · 2023-01-17T19:39:49Z

cmd/git-bundle-web-server/main.go

+	c := make(chan os.Signal, 1)
+	signal.Notify(c, os.Interrupt, syscall.SIGINT, syscall.SIGTERM)
+	go func() {
+		<-c


Neat first use of channels in this project, I think.

derrickstolee · 2023-01-17T19:41:01Z

cmd/git-bundle-server/command.go

 		Update{},
 		UpdateAll{},
+		NewWebServerCommand(),
 	}


I expect that this will be the new pattern for this list when we get around to unit-testing each command. It's a good pattern.

ldennington

Looks great! While this was a big change, I can't say how much I appreciate the thought and effort that went into breaking it into small pieces. It made the review extremely nice both in terms of being able to understand what was going on and feeling as though I was consistently and measurably making progress as I went through it.

cmd/git-bundle-server/update.go

ldennington · 2023-01-17T19:41:14Z

internal/common/filesystem.go

+	}
+}
+
+func (f *fileSystem) WriteFile(filename string, content []byte) error {


Creating the parent directories/adding default permissions here is nice!

ldennington · 2023-01-17T20:14:14Z

internal/daemon/systemd.go

 )

-type systemd struct{}
+const serviceTemplate string = `[Unit]


Oh wow, so much simpler than the plist.

ldennington · 2023-01-17T20:26:50Z

cmd/git-bundle-server/command.go

 		Update{},
 		UpdateAll{},
+		NewWebServerCommand(),
 	}


derrickstolee

Change the 'Subcommand' interface's 'subcommand()' to 'Name()' to clarify what that the function is intended to return the name of the subcommand (as it would be used on the command line). Additionally, by capitalizing the function name, make it public to other packages; this will be necessary in later patches when the interface is moved to a new package. Signed-off-by: Victoria Dye <[email protected]>

To set up for future patches in which we'll improve the command-line reporting for 'git-bundle-server' commands, add a public 'Description()' function to require each command to include a string description of its action. Signed-off-by: Victoria Dye <[email protected]>

Make the 'run()' function public (by capitalizing it as 'Run()') so that, when 'Subcommand' is moved into an external package, it can be implemented by the commands in 'git-bundle-server'. Signed-off-by: Victoria Dye <[email protected]>

Add a basic argument parser for arbitrary series of string arguments. For now, it is little more than a wrapper of 'flag.FlagSet' (a builtin structure that handles parsing of flags preceded by a '-'). In future patches, the arg parser will be extended to handle subcommands and positional arguments to simplify 'git-bundle-server' command setup. Signed-off-by: Victoria Dye <[email protected]>

Move the 'Subcommand' interface from the 'main' package of 'git-bundle-server' to the internal 'argparse' library so that it can be used by the parser as a general representation of an invokable command/subcommand. Additionally, keep the 'all()' function in the 'main' package by moving it to 'main.go'. Signed-off-by: Victoria Dye <[email protected]>

Add a 'genericSubcommand' type and "constructor" 'NewSubcommand()' to allow for specifying subcommands that do not directly implement the 'Subcommand' interface, but instead are specified with a static name, description, and run function. Signed-off-by: Victoria Dye <[email protected]>

Add handling for subcommands in the custom arg parser. To specify a subcommand, a user can call 'parser.Subcommand' and provide an 'argparse.Subcommand' instance, whose 'Name()' is used to identify its use on the command line. When the specified subcommand is identifies, its 'Run()' function can be called in 'InvokeSubcommand()'. This function, when called, is provided the remaining arguments *after* the subcommand, to allow the subcommand function to perform its own argument parsing. To use the most accurate terminology in usage printouts, include a function ('SetIsTopLevel()') to toggle whether the subcommands listed in the printout are called "Commands" or "Subcommands". Signed-off-by: Victoria Dye <[email protected]>

Add the ability to specify single- or multi-item positional arguments, represented as 'string's and '[]string's, respectively. These arguments are processes in order of specification, and multi-item positional args are only allowed as the *last* positional argument specified. Signed-off-by: Victoria Dye <[email protected]>

Update 'main.go' to use the custom argument parser in the 'argparse' package, specifying each command as a "subcommand" and invoking it with 'InvokeSubcommand()'. By using the argument parser, the '-h' or '--help' flags will automatically print usage information, and improperly formed invocations (invalid command, no command specified, unused flags, etc.) will trigger an informative error and exit with the usage string. This change substantially simplifies 'main.go' and improves user experience with the CLI. Signed-off-by: Victoria Dye <[email protected]>

Update the 'git-bundle-server' commands to use argument parsers to parse their flags and positional arguments (none currently use subcommands). Signed-off-by: Victoria Dye <[email protected]>

Create 'DaemonProvider' interface for creating and managing daemon processes, to eventually be used by the 'web-server' subcommand to manage a web server daemon. Because daemon process management is dependent on the operating system/tools available, initially create two types to implement the interface: one for 'launchd' (MacOS) and one for 'systemd' (Linux). The functions, for now, all return "not implemented" errors but will be implemented in later patches. Finally, add a 'NewDaemonProvider' to create the appropriate daemon provider implementation based on the operating system. Signed-off-by: Victoria Dye <[email protected]>

Add 'CommandExecutor' interfaced type for executing arbitrary commands ('git', 'launchctl', etc.). Signed-off-by: Victoria Dye <[email protected]>

Add interfaced 'FileSystem' type for handling basic filesystem operations (existence checks, read/write, etc.). For now, just implement 'FileExists()' and 'WriteFile()'. Note that 'WriteFile()' differs from 'os.WriteFile()' - it creates the parent directories of the file (if necessary), and sets directory and file permissions based on hardcoded defaults (755 and 644, respectively). Signed-off-by: Victoria Dye <[email protected]>

Like 'FileSystem' and 'CommandExecutor', 'UserProvider' wraps functionality related to the system users in a mockable interface. At the moment, its only function returns the current user, eventually to be used for web server daemon process setup. Signed-off-by: Victoria Dye <[email protected]>

Implement and test the 'Create()' function for the 'launchd' implementation of 'DaemonProvider'. To create the service, this function first generates the 'plist' [1] defining service metadata (the name, program to execute, etc), then writes the file and loads - but does not start - the service. The exact behavior is dependent on the 'force' flag. If it is 'false', 'Create()' will not overwrite the existing service; if 'true', it unloads the old service (if applicable), writes the new file, and reloads. Finally, add unit tests covering various scenarios that may occur when creating the 'launchd' service. Add mocks and other common structures & functions to a dedicated 'shared_test.go' file, as they will be useful when testing other daemon provider implementations in later patches. [1] https://www.unix.com/man-page/osx/5/launchd.plist/ Signed-off-by: Victoria Dye <[email protected]>

Implement and test the 'Start()' function for the 'launchd' daemon process manager. The 'Start()' function assumes that the specified service has already been boostrapped (with 'Create()'), and simply runs 'launchctl kickstart' to start the service. If the service is already running, 'Start()' is effectively a no-op (as 'launchctl kickstart' does not restart the already-running process). Signed-off-by: Victoria Dye <[email protected]>

Implement and test the 'Stop()' function for the 'launchd' daemon manager. 'Stop()' shuts down the specified service (if it is running) with 'launchctl kill SIGINT'. If the command succeeds *or* fails indicating the service is not bootstrapped, exit without an error; otherwise, report the appropriate error. Signed-off-by: Victoria Dye <[email protected]>

Implement and test the 'Create()' function for creating a 'systemd' daemon service. The service unit created is user-scoped (hence the '--user' option in all 'systemctl' commands) and, like the equivalent 'launchd' implementation, does not overwrite an existing service file unless the 'force' arg is 'true'. Signed-off-by: Victoria Dye <[email protected]>

Implement and test the 'Start()' function for a systemd-managed daemon service. As in the 'launchd' daemon provider, the 'Start()' function does not check whether the service is already running; if it is, the operation is a no-op. Signed-off-by: Victoria Dye <[email protected]>

Implement and test the 'Stop()' function for 'systemd'-managed daemon processes. If the service is already stopped or doesn't exist, this function is a no-op. Signed-off-by: Victoria Dye <[email protected]>

Rather than have a SIGINT or SIGTERM cause an immediate exit to the 'git-bundle-web-server' process, have it trigger a 'Shutdown()' on the hosted web server. Note that SIGKILL still forcibly stops the server, in case the shutdown gets stuck. Signed-off-by: Victoria Dye <[email protected]>

The 'web-server' subcommand is used to start/stop a daemon process running 'git-bundle-web-server', selecting 'launchd' or 'systemd' to manage the process depending on the operating system (Windows support is not yet implemented). Signed-off-by: Victoria Dye <[email protected]>

vdye · 2023-01-18T19:35:01Z

@derrickstolee @ldennington just pushed changes to de-duplicate some subcommand-related code, summary of changes is:

Rename Command back to Subcommand, move to argparse package, make all functions public.
Make argParser.Subcommand() take a Subcommand instance as an argument, rather than individual name/description/run function args.
Add a genericSubcommand to handle subcommands specified with static name/description/run function (e.g. web-server start and web-server stop).

derrickstolee

I took a look at the new parsing structure changes. Looking good. Thanks for taking the time to establish a solid foundation for future work!

vdye requested review from derrickstolee and ldennington January 14, 2023 00:33

vdye self-assigned this Jan 14, 2023

derrickstolee approved these changes Jan 17, 2023

View reviewed changes

ldennington approved these changes Jan 17, 2023

View reviewed changes

vdye force-pushed the vdye/web-server-daemon branch from 25ece8b to 6f165d5 Compare January 18, 2023 17:56

derrickstolee approved these changes Jan 18, 2023

View reviewed changes

vdye added 22 commits January 18, 2023 10:43

bundle-server: update commands to use arg parser

2d4657d

Update the 'git-bundle-server' commands to use argument parsers to parse their flags and positional arguments (none currently use subcommands). Signed-off-by: Victoria Dye <[email protected]>

common: add interface for executing external programs

a442b65

Add 'CommandExecutor' interfaced type for executing arbitrary commands ('git', 'launchctl', etc.). Signed-off-by: Victoria Dye <[email protected]>

systemd: implement 'Stop()' function

7ed7e1c

Implement and test the 'Stop()' function for 'systemd'-managed daemon processes. If the service is already stopped or doesn't exist, this function is a no-op. Signed-off-by: Victoria Dye <[email protected]>

vdye force-pushed the vdye/web-server-daemon branch from 6f165d5 to ca23190 Compare January 18, 2023 19:31

derrickstolee approved these changes Jan 18, 2023

View reviewed changes

vdye merged commit f64602c into main Jan 18, 2023

vdye deleted the vdye/web-server-daemon branch January 18, 2023 19:58

derrickstolee mentioned this pull request Jan 31, 2023

Unit test GetRepositories() #11

Merged

Improve argument parsing & implement the web-server command #9

Improve argument parsing & implement the web-server command #9

Uh oh!

Conversation

vdye commented Jan 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Testing

Leftover bits

Uh oh!

derrickstolee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldennington left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derrickstolee left a comment

Choose a reason for hiding this comment

Uh oh!

vdye commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derrickstolee left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Improve argument parsing & implement the `web-server` command #9

Improve argument parsing & implement the `web-server` command #9

vdye commented Jan 14, 2023 •

edited

Loading

vdye commented Jan 18, 2023 •

edited

Loading