cron: refactor into interfaces

Refactor 'internal/cron.go' into a 'CronScheduler' interface, and
'cmd/git-bundle-server/cron.go' (moved to 'cmd/utils/cron.go') into a
'CronHelper'  interface. The former converts standalone functions previously
in the file into struct methods, while also moving much of the job
addition/update logic from 'cmd/git-bundle-server/cron.go' into a new
'AddJob()'. The 'CronHelper' has one method - 'SetCronSchedule()' - which
calls 'AddJob()' to set the daily 'update-all' schedule (used by the 'init'
and 'start' commands).

Both of these structs utilize the existing helper structures ('FileSystem',
'CommandExecutor', and 'UserProvider') to handle all system interaction. As
a result, they can both be unit tested in a future update.

Signed-off-by: Victoria Dye <[email protected]>

Author: vdye

cmd/git-bundle-server/cron.go

@@ -83,7 +83,8 @@ func (i *initCmd) Run(ctx context.Context, args []string) error {
 		return i.logger.Errorf(ctx, "failed to write bundle list: %w", listErr)
 	}
 
-	SetCronSchedule()
+	cron := utils.GetDependency[utils.CronHelper](ctx, i.container)
+	cron.SetCronSchedule(ctx)
 
 	return nil
 }

cmd/git-bundle-server/init.go

@@ -51,7 +51,8 @@ func (s *startCmd) Run(ctx context.Context, args []string) error {
 	}
 
 	// Make sure we have the global schedule running.
-	SetCronSchedule()
+	cron := utils.GetDependency[utils.CronHelper](ctx, s.container)
+	cron.SetCronSchedule(ctx)
 
 	return nil
 }

cmd/git-bundle-server/start.go

@@ -32,6 +32,21 @@ func BuildGitBundleServerContainer(logger log.TraceLogger) *DependencyContainer
 	registerDependency(container, func(ctx context.Context) bundles.BundleProvider {
 		return bundles.NewBundleProvider(logger)
 	})
+	registerDependency(container, func(ctx context.Context) core.CronScheduler {
+		return core.NewCronScheduler(
+			logger,
+			GetDependency[common.UserProvider](ctx, container),
+			GetDependency[cmd.CommandExecutor](ctx, container),
+			GetDependency[common.FileSystem](ctx, container),
+		)
+	})
+	registerDependency(container, func(ctx context.Context) CronHelper {
+		return NewCronHelper(
+			logger,
+			GetDependency[common.FileSystem](ctx, container),
+			GetDependency[core.CronScheduler](ctx, container),
+		)
+	})
 	registerDependency(container, func(ctx context.Context) daemon.DaemonProvider {
 		t, err := daemon.NewDaemonProvider(
 			logger,

cmd/utils/container-helpers.go

@@ -0,0 +1,45 @@
+package utils
+
+import (
+	"context"
+
+	"github.com/github/git-bundle-server/internal/common"
+	"github.com/github/git-bundle-server/internal/core"
+	"github.com/github/git-bundle-server/internal/log"
+)
+
+type CronHelper interface {
+	SetCronSchedule(ctx context.Context) error
+}
+
+type cronHelper struct {
+	logger     log.TraceLogger
+	fileSystem common.FileSystem
+	scheduler  core.CronScheduler
+}
+
+func NewCronHelper(
+	l log.TraceLogger,
+	fs common.FileSystem,
+	s core.CronScheduler,
+) CronHelper {
+	return &cronHelper{
+		logger:     l,
+		fileSystem: fs,
+		scheduler:  s,
+	}
+}
+
+func (c *cronHelper) SetCronSchedule(ctx context.Context) error {
+	pathToExec, err := c.fileSystem.GetLocalExecutable("git-bundle-server")
+	if err != nil {
+		return c.logger.Errorf(ctx, "failed to get executable: %w", err)
+	}
+
+	err = c.scheduler.AddJob(ctx, core.CronDaily, pathToExec, []string{"update-all"})
+	if err != nil {
+		return c.logger.Errorf(ctx, "failed to set cron schedule: %w", err)
+	}
+
+	return nil
+}

cmd/utils/cron.go

@@ -2,62 +2,120 @@ package core
 
 import (
 	"bytes"
+	"context"
 	"fmt"
-	"os/exec"
+	"strings"
+
+	"github.com/github/git-bundle-server/internal/cmd"
+	"github.com/github/git-bundle-server/internal/common"
+	"github.com/github/git-bundle-server/internal/log"
+	"github.com/github/git-bundle-server/internal/utils"
+)
+
+type cronSchedule string
+
+const (
+	CronDaily  cronSchedule = "0 0 * * *"
+	CronWeekly cronSchedule = "0 0 0 * *"
 )
 
-func GetCrontabCommand(args ...string) (*exec.Cmd, error) {
-	crontab, err := exec.LookPath("crontab")
+type CronScheduler interface {
+	AddJob(ctx context.Context, schedule cronSchedule,
+		exePath string, args []string) error
+}
+
+type cronScheduler struct {
+	logger     log.TraceLogger
+	user       common.UserProvider
+	cmdExec    cmd.CommandExecutor
+	fileSystem common.FileSystem
+}
+
+func NewCronScheduler(
+	l log.TraceLogger,
+	u common.UserProvider,
+	c cmd.CommandExecutor,
+	fs common.FileSystem,
+) CronScheduler {
+	return &cronScheduler{
+		logger:     l,
+		user:       u,
+		cmdExec:    c,
+		fileSystem: fs,
+	}
+}
+
+func (c *cronScheduler) loadExistingSchedule(ctx context.Context) ([]byte, error) {
+	buffer := bytes.Buffer{}
+	exitCode, err := c.cmdExec.Run(ctx, "crontab", []string{"-l"}, cmd.Stdout(&buffer))
 	if err != nil {
-		return nil, fmt.Errorf("failed to find 'crontab' on the path: %w", err)
+		return nil, c.logger.Error(ctx, err)
+	} else if exitCode != 0 {
+		return nil, c.logger.Errorf(ctx, "'crontab' exited with status %d", exitCode)
 	}
 
-	cmd := exec.Command(crontab, args...)
-	return cmd, nil
+	return buffer.Bytes(), nil
 }
 
-func LoadExistingSchedule() ([]byte, error) {
-	cmd, err := GetCrontabCommand("-l")
+func (c *cronScheduler) commitCronSchedule(ctx context.Context, filename string) error {
+	exitCode, err := c.cmdExec.RunQuiet(ctx, "crontab", filename)
 	if err != nil {
-		return nil, err
+		return c.logger.Error(ctx, err)
+	} else if exitCode != 0 {
+		return c.logger.Errorf(ctx, "'crontab' exited with status %d", exitCode)
 	}
 
-	buffer := bytes.Buffer{}
-	cmd.Stdout = &buffer
+	return nil
+}
 
-	errorBuffer := bytes.Buffer{}
-	cmd.Stderr = &errorBuffer
+func (c *cronScheduler) AddJob(ctx context.Context,
+	schedule cronSchedule,
+	exePath string,
+	args []string,
+) error {
+	newSchedule := fmt.Sprintf("%s \"%s\" %s",
+		schedule,
+		exePath,
+		utils.Map(args, func(s string) string { return "\"" + s + "\"" }),
+	)
 
-	err = cmd.Start()
+	scheduleBytes, err := c.loadExistingSchedule(ctx)
 	if err != nil {
-		return nil, fmt.Errorf("crontab failed to start: %w", err)
+		return c.logger.Errorf(ctx, "failed to get existing cron schedule: %w", err)
 	}
 
-	err = cmd.Wait()
-	if err != nil {
-		return nil, fmt.Errorf("crontab returned a failure: %w\nstderr: %s", err, errorBuffer.String())
+	scheduleStr := string(scheduleBytes)
+
+	// TODO: Use comments to indicate a "region" where our schedule
+	// is set, so we can remove the entire region even if we update
+	// the schedule in the future.
+	if strings.Contains(scheduleStr, newSchedule) {
+		// We already have this schedule, so skip modifying
+		// the crontab schedule.
+		return nil
 	}
 
-	return buffer.Bytes(), nil
-}
+	scheduleBytes = append(scheduleBytes, []byte(schedule)...)
 
-func CommitCronSchedule(filename string) error {
-	cmd, err := GetCrontabCommand(filename)
+	user, err := c.user.CurrentUser()
 	if err != nil {
-		return err
+		return c.logger.Error(ctx, err)
 	}
+	scheduleFile := CrontabFile(user)
 
-	errorBuffer := bytes.Buffer{}
-	cmd.Stderr = &errorBuffer
+	err = c.fileSystem.WriteFile(scheduleFile, scheduleBytes)
+	if err != nil {
+		return c.logger.Errorf(ctx, "failed to write new cron schedule to temp file: %w", err)
+	}
 
-	err = cmd.Start()
+	err = c.commitCronSchedule(ctx, scheduleFile)
 	if err != nil {
-		return fmt.Errorf("crontab failed to start: %w", err)
+		return c.logger.Errorf(ctx, "failed to commit new cron schedule: %w", err)
 	}
 
-	err = cmd.Wait()
+	_, err = c.fileSystem.DeleteFile(scheduleFile)
 	if err != nil {
-		return fmt.Errorf("crontab returned a failure: %w\nstderr: %s", err, errorBuffer.String())
+		return c.logger.Errorf(ctx, "failed to clear schedule temp file: %w", err)
 	}
 
 	return nil