From 248a0b96d529bb65b68b9b8f90946673030afed1 Mon Sep 17 00:00:00 2001 From: Josh Deprez Date: Tue, 17 Sep 2024 15:45:36 +1000 Subject: [PATCH 1/2] Rename internal/utils to internal/osutil --- clicommand/agent_start.go | 4 ++-- cliconfig/file.go | 4 ++-- cliconfig/loader.go | 6 +++--- internal/job/checkout.go | 16 ++++++++-------- internal/job/executor.go | 12 ++++++------ internal/job/hook/hook.go | 4 ++-- internal/job/plugin.go | 8 ++++---- internal/osutil/doc.go | 2 ++ internal/{utils => osutil}/file.go | 2 +- internal/{utils => osutil}/path.go | 2 +- internal/{utils => osutil}/path_test.go | 2 +- internal/{utils => osutil}/path_windows_test.go | 2 +- internal/utils/doc.go | 4 ---- 13 files changed, 33 insertions(+), 35 deletions(-) create mode 100644 internal/osutil/doc.go rename internal/{utils => osutil}/file.go (98%) rename internal/{utils => osutil}/path.go (99%) rename internal/{utils => osutil}/path_test.go (98%) rename internal/{utils => osutil}/path_windows_test.go (97%) delete mode 100644 internal/utils/doc.go diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index 4821e2070f..70fcaec883 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -30,7 +30,7 @@ import ( "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/agent/v3/internal/job/hook" "github.com/buildkite/agent/v3/internal/job/shell" - "github.com/buildkite/agent/v3/internal/utils" + "github.com/buildkite/agent/v3/internal/osutil" "github.com/buildkite/agent/v3/logger" "github.com/buildkite/agent/v3/metrics" "github.com/buildkite/agent/v3/process" @@ -1133,7 +1133,7 @@ var AgentStartCommand = cli.Command{ // confirm the BuildPath is exists. The bootstrap is going to write to it when a job executes, // so we may as well check that'll work now and fail early if it's a problem - if !utils.FileExists(agentConf.BuildPath) { + if !osutil.FileExists(agentConf.BuildPath) { l.Info("Build Path doesn't exist, creating it (%s)", agentConf.BuildPath) // Actual file permissions will be reduced by umask, and won't be 0777 unless the user has manually changed the umask to 000 if err := os.MkdirAll(agentConf.BuildPath, 0o777); err != nil { diff --git a/cliconfig/file.go b/cliconfig/file.go index aa067985cc..b37c691077 100644 --- a/cliconfig/file.go +++ b/cliconfig/file.go @@ -7,7 +7,7 @@ import ( "os" "strings" - "github.com/buildkite/agent/v3/internal/utils" + "github.com/buildkite/agent/v3/internal/osutil" ) type File struct { @@ -60,7 +60,7 @@ func (f *File) Load() error { } func (f File) AbsolutePath() (string, error) { - return utils.NormalizeFilePath(f.Path) + return osutil.NormalizeFilePath(f.Path) } func (f File) Exists() bool { diff --git a/cliconfig/loader.go b/cliconfig/loader.go index 7c2fe54f22..1f5b47e5e7 100644 --- a/cliconfig/loader.go +++ b/cliconfig/loader.go @@ -12,7 +12,7 @@ import ( "strings" "time" - "github.com/buildkite/agent/v3/internal/utils" + "github.com/buildkite/agent/v3/internal/osutil" "github.com/buildkite/agent/v3/logger" "github.com/oleiade/reflections" "github.com/urfave/cli" @@ -372,7 +372,7 @@ func (l Loader) normalizeField(fieldName string, normalization string) error { // Normalize the field to be a filepath if valueAsString, ok := value.(string); ok { - normalizedPath, err := utils.NormalizeFilePath(valueAsString) + normalizedPath, err := osutil.NormalizeFilePath(valueAsString) if err != nil { return err } @@ -392,7 +392,7 @@ func (l Loader) normalizeField(fieldName string, normalization string) error { // Normalize the field to be a command if valueAsString, ok := value.(string); ok { - normalizedCommandPath, err := utils.NormalizeCommand(valueAsString) + normalizedCommandPath, err := osutil.NormalizeCommand(valueAsString) if err != nil { return err } diff --git a/internal/job/checkout.go b/internal/job/checkout.go index 4720fe2c84..e5caa25f80 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -11,7 +11,7 @@ import ( "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/agent/v3/internal/job/shell" - "github.com/buildkite/agent/v3/internal/utils" + "github.com/buildkite/agent/v3/internal/osutil" "github.com/buildkite/agent/v3/tracetools" "github.com/buildkite/roko" ) @@ -80,7 +80,7 @@ func (e *Executor) removeCheckoutDir() error { func (e *Executor) createCheckoutDir() error { checkoutPath, _ := e.shell.Env.Get("BUILDKITE_BUILD_CHECKOUT_PATH") - if !utils.FileExists(checkoutPath) { + if !osutil.FileExists(checkoutPath) { e.shell.Commentf("Creating \"%s\"", checkoutPath) // Actual file permissions will be reduced by umask, and won't be 0777 unless the user has manually changed the umask to 000 if err := os.MkdirAll(checkoutPath, 0777); err != nil { @@ -258,7 +258,7 @@ func (e *Executor) CheckoutPhase(ctx context.Context) error { } func hasGitSubmodules(sh *shell.Shell) bool { - return utils.FileExists(filepath.Join(sh.Getwd(), ".gitmodules")) + return osutil.FileExists(filepath.Join(sh.Getwd(), ".gitmodules")) } func hasGitCommit(ctx context.Context, sh *shell.Shell, gitDir string, commit string) bool { @@ -283,7 +283,7 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string) (stri isMainRepository := repository == e.Repository // Create the mirrors path if it doesn't exist - if baseDir := filepath.Dir(mirrorDir); !utils.FileExists(baseDir) { + if baseDir := filepath.Dir(mirrorDir); !osutil.FileExists(baseDir) { e.shell.Commentf("Creating \"%s\"", baseDir) // Actual file permissions will be reduced by umask, and won't be 0777 unless the user has manually changed the umask to 000 if err := os.MkdirAll(baseDir, 0777); err != nil { @@ -309,7 +309,7 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string) (stri defer mirrorCloneLock.Unlock() // If we don't have a mirror, we need to clone it - if !utils.FileExists(mirrorDir) { + if !osutil.FileExists(mirrorDir) { e.shell.Commentf("Cloning a mirror of the repository to %q", mirrorDir) flags := "--mirror " + e.GitCloneMirrorFlags if err := gitClone(ctx, e.shell, flags, repository, mirrorDir); err != nil { @@ -455,7 +455,7 @@ func (e *Executor) getOrUpdateMirrorDir(ctx context.Context, repository string) e.shell.Commentf("Skipping update and using existing mirror for repository %s at %s.", repository, mirrorDir) // Check if specified mirrorDir exists, otherwise the clone will fail. - if !utils.FileExists(mirrorDir) { + if !osutil.FileExists(mirrorDir) { // Fall back to a clean clone, rather than failing the clone and therefore the build e.shell.Commentf("No existing mirror found for repository %s at %s.", repository, mirrorDir) mirrorDir = "" @@ -507,7 +507,7 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error { // Does the git directory exist? existingGitDir := filepath.Join(e.shell.Getwd(), ".git") - if utils.FileExists(existingGitDir) { + if osutil.FileExists(existingGitDir) { // Update the origin of the repository so we can gracefully handle // repository renames if _, err := e.updateRemoteURL(ctx, "", e.Repository); err != nil { @@ -647,7 +647,7 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error { // Tests use a local temp path for the repository, real repositories don't. Handle both. var repositoryPath string - if !utils.FileExists(repository) { + if !osutil.FileExists(repository) { repositoryPath = filepath.Join(e.ExecutorConfig.GitMirrorsPath, dirForRepository(repository)) } else { repositoryPath = repository diff --git a/internal/job/executor.go b/internal/job/executor.go index b1a9350871..280bb02abd 100644 --- a/internal/job/executor.go +++ b/internal/job/executor.go @@ -26,11 +26,11 @@ import ( "github.com/buildkite/agent/v3/internal/file" "github.com/buildkite/agent/v3/internal/job/hook" "github.com/buildkite/agent/v3/internal/job/shell" + "github.com/buildkite/agent/v3/internal/osutil" "github.com/buildkite/agent/v3/internal/redact" "github.com/buildkite/agent/v3/internal/replacer" "github.com/buildkite/agent/v3/internal/shellscript" "github.com/buildkite/agent/v3/internal/tempfile" - "github.com/buildkite/agent/v3/internal/utils" "github.com/buildkite/agent/v3/kubernetes" "github.com/buildkite/agent/v3/process" "github.com/buildkite/agent/v3/tracetools" @@ -338,7 +338,7 @@ func (e *Executor) executeHook(ctx context.Context, hookCfg HookConfig) error { } hookName += " " + hookCfg.Name - if !utils.FileExists(hookCfg.Path) { + if !osutil.FileExists(hookCfg.Path) { if e.Debug { e.shell.Commentf("Skipping %s hook, no script at \"%s\"", hookName, hookCfg.Path) } @@ -420,7 +420,7 @@ func logOpenedHookInfo(l shell.Logger, debug bool, hookName, path string) { } else { l.Errorf("The %s hook failed to run the %s process has the hook file open", hookName, procPath) } - case utils.FileExists("/dev/fd"): + case osutil.FileExists("/dev/fd"): isOpened, err := file.IsOpened(l, debug, path) if err == nil { if isOpened { @@ -715,7 +715,7 @@ func dirForRepository(repository string) string { // Given a repository, it will add the host to the set of SSH known_hosts on the machine func addRepositoryHostToSSHKnownHosts(ctx context.Context, sh *shell.Shell, repository string) { - if utils.FileExists(repository) { + if osutil.FileExists(repository) { return } @@ -965,7 +965,7 @@ func (e *Executor) defaultCommandPhase(ctx context.Context) error { scriptFileName := strings.Replace(e.Command, "\n", "", -1) pathToCommand, err := filepath.Abs(filepath.Join(e.shell.Getwd(), scriptFileName)) - commandIsScript := err == nil && utils.FileExists(pathToCommand) + commandIsScript := err == nil && osutil.FileExists(pathToCommand) span.AddAttributes(map[string]string{"hook.command": pathToCommand}) // If the command isn't a script, then it's something we need @@ -1039,7 +1039,7 @@ func (e *Executor) defaultCommandPhase(ctx context.Context) error { // shouldn't be vulnerable to this! if e.ExecutorConfig.CommandEval { // Make script executable - if err = utils.ChmodExecutable(pathToCommand); err != nil { + if err = osutil.ChmodExecutable(pathToCommand); err != nil { e.shell.Warningf("Error marking script %q as executable: %v", pathToCommand, err) return err } diff --git a/internal/job/hook/hook.go b/internal/job/hook/hook.go index 15c4a051e7..2c4131b897 100644 --- a/internal/job/hook/hook.go +++ b/internal/job/hook/hook.go @@ -10,7 +10,7 @@ import ( "runtime" "github.com/buildkite/agent/v3/internal/job/shell" - "github.com/buildkite/agent/v3/internal/utils" + "github.com/buildkite/agent/v3/internal/osutil" ) // Find returns the absolute path to the best matching hook file in a path, or @@ -23,7 +23,7 @@ func Find(hookDir string, name string) (string, error) { } } // otherwise chech for th default shell script - if p := filepath.Join(hookDir, name); utils.FileExists(p) { + if p := filepath.Join(hookDir, name); osutil.FileExists(p) { return p, nil } // Don't wrap os.ErrNotExist without checking callers handle it. diff --git a/internal/job/plugin.go b/internal/job/plugin.go index 793bb818ed..957a83c4f5 100644 --- a/internal/job/plugin.go +++ b/internal/job/plugin.go @@ -13,7 +13,7 @@ import ( "github.com/buildkite/agent/v3/agent/plugin" "github.com/buildkite/agent/v3/internal/job/hook" - "github.com/buildkite/agent/v3/internal/utils" + "github.com/buildkite/agent/v3/internal/osutil" "github.com/buildkite/roko" ) @@ -162,7 +162,7 @@ func (e *Executor) VendoredPluginPhase(ctx context.Context) error { return fmt.Errorf("Failed to resolve vendored plugin path for plugin %s: %w", p.Name(), err) } - if !utils.FileExists(pluginLocation) { + if !osutil.FileExists(pluginLocation) { return fmt.Errorf("Vendored plugin path %s doesn't exist", p.Location) } @@ -324,7 +324,7 @@ func (e *Executor) checkoutPlugin(ctx context.Context, p *plugin.Plugin) (*plugi // that means a potentially slow and unnecessary clone on every build step. Sigh. I think the // tradeoff is favourable for just blowing away an existing clone if we want least-hassle // guarantee that the user will get the latest version of their plugin branch/tag/whatever. - if e.ExecutorConfig.PluginsAlwaysCloneFresh && utils.FileExists(pluginDirectory) { + if e.ExecutorConfig.PluginsAlwaysCloneFresh && osutil.FileExists(pluginDirectory) { e.shell.Commentf("BUILDKITE_PLUGINS_ALWAYS_CLONE_FRESH is true; removing previous checkout of plugin %s", p.Label()) err = os.RemoveAll(pluginDirectory) if err != nil { @@ -333,7 +333,7 @@ func (e *Executor) checkoutPlugin(ctx context.Context, p *plugin.Plugin) (*plugi } } - if utils.FileExists(pluginGitDirectory) { + if osutil.FileExists(pluginGitDirectory) { // It'd be nice to show the current commit of the plugin, so // let's figure that out. headCommit, err := gitRevParseInWorkingDirectory(ctx, e.shell, pluginDirectory, "--short=7", "HEAD") diff --git a/internal/osutil/doc.go b/internal/osutil/doc.go new file mode 100644 index 0000000000..d07902ac13 --- /dev/null +++ b/internal/osutil/doc.go @@ -0,0 +1,2 @@ +// Package osutil provides some OS-level helper functions. +package osutil diff --git a/internal/utils/file.go b/internal/osutil/file.go similarity index 98% rename from internal/utils/file.go rename to internal/osutil/file.go index 2bf5083006..d4c2c91d67 100644 --- a/internal/utils/file.go +++ b/internal/osutil/file.go @@ -1,4 +1,4 @@ -package utils +package osutil import ( "fmt" diff --git a/internal/utils/path.go b/internal/osutil/path.go similarity index 99% rename from internal/utils/path.go rename to internal/osutil/path.go index f6d8b86875..89a6502f0f 100644 --- a/internal/utils/path.go +++ b/internal/osutil/path.go @@ -1,4 +1,4 @@ -package utils +package osutil import ( "errors" diff --git a/internal/utils/path_test.go b/internal/osutil/path_test.go similarity index 98% rename from internal/utils/path_test.go rename to internal/osutil/path_test.go index 7d1742095f..7439cbaeef 100644 --- a/internal/utils/path_test.go +++ b/internal/osutil/path_test.go @@ -1,4 +1,4 @@ -package utils +package osutil import ( "os" diff --git a/internal/utils/path_windows_test.go b/internal/osutil/path_windows_test.go similarity index 97% rename from internal/utils/path_windows_test.go rename to internal/osutil/path_windows_test.go index d8fa5efffb..3cdf315701 100644 --- a/internal/utils/path_windows_test.go +++ b/internal/osutil/path_windows_test.go @@ -1,7 +1,7 @@ //go:build windows // +build windows -package utils +package osutil import ( "os" diff --git a/internal/utils/doc.go b/internal/utils/doc.go deleted file mode 100644 index f7ff347238..0000000000 --- a/internal/utils/doc.go +++ /dev/null @@ -1,4 +0,0 @@ -// Package utils provides some file- and filepath-helper functions. -// -// TODO: `utils` is too vague. Find a better name for this package. -package utils From 6a6dfa00e540a6727bbb35dcd85b0972c3633c2a Mon Sep 17 00:00:00 2001 From: Josh Deprez Date: Tue, 17 Sep 2024 15:45:36 +1000 Subject: [PATCH 2/2] Add osutil.UserHomeDir --- clicommand/agent_start.go | 2 +- internal/job/knownhosts.go | 3 +- internal/osutil/homedir.go | 12 ++++++++ internal/osutil/homedir_test.go | 50 +++++++++++++++++++++++++++++++++ 4 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 internal/osutil/homedir.go create mode 100644 internal/osutil/homedir_test.go diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index 70fcaec883..157bb2a22c 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -1383,7 +1383,7 @@ func agentLifecycleHook(hookName string, log logger.Logger, cfg AgentStartConfig } func defaultSocketsPath() string { - home, err := os.UserHomeDir() + home, err := osutil.UserHomeDir() if err != nil { return filepath.Join(os.TempDir(), "buildkite-sockets") } diff --git a/internal/job/knownhosts.go b/internal/job/knownhosts.go index 03ab7e4de7..4b6f1c5043 100644 --- a/internal/job/knownhosts.go +++ b/internal/job/knownhosts.go @@ -10,6 +10,7 @@ import ( "time" "github.com/buildkite/agent/v3/internal/job/shell" + "github.com/buildkite/agent/v3/internal/osutil" "golang.org/x/crypto/ssh/knownhosts" ) @@ -19,7 +20,7 @@ type knownHosts struct { } func findKnownHosts(sh *shell.Shell) (*knownHosts, error) { - userHomePath, err := os.UserHomeDir() + userHomePath, err := osutil.UserHomeDir() if err != nil { return nil, fmt.Errorf("Could not find the current users home directory (%s)", err) } diff --git a/internal/osutil/homedir.go b/internal/osutil/homedir.go new file mode 100644 index 0000000000..e56cae9739 --- /dev/null +++ b/internal/osutil/homedir.go @@ -0,0 +1,12 @@ +package osutil + +import "os" + +// UserHomeDir is similar to os.UserHomeDir, but prefers $HOME when available +// over other options (such as USERPROFILE on Windows). +func UserHomeDir() (string, error) { + if h := os.Getenv("HOME"); h != "" { + return h, nil + } + return os.UserHomeDir() +} diff --git a/internal/osutil/homedir_test.go b/internal/osutil/homedir_test.go new file mode 100644 index 0000000000..7fa2f9450c --- /dev/null +++ b/internal/osutil/homedir_test.go @@ -0,0 +1,50 @@ +package osutil + +import ( + "os" + "runtime" + "testing" +) + +func TestUserHomeDir(t *testing.T) { + // not parallel because it messes with env vars + origHome := os.Getenv("HOME") + origUserProfile := os.Getenv("USERPROFILE") + t.Cleanup(func() { + os.Setenv("HOME", origHome) + os.Setenv("USERPROFILE", origUserProfile) + }) + + type testCase struct { + home, userProfile, want string + } + + tests := []testCase{ + { + // Prefer $HOME on all platforms + home: "home", + userProfile: "userProfile", + want: "home", + }, + } + if runtime.GOOS == "windows" { + // Windows can use %USERPROFILE% as a treat when $HOME is unavailable + tests = append(tests, testCase{ + home: "", + userProfile: "userProfile", + want: "userProfile", + }) + } + + for _, test := range tests { + os.Setenv("HOME", test.home) + os.Setenv("USERPROFILE", test.userProfile) + got, err := UserHomeDir() + if err != nil { + t.Errorf("HOME=%q USERPROFILE=%q UserHomeDir() error = %v", test.home, test.userProfile, err) + } + if got != test.want { + t.Errorf("HOME=%q USERPROFILE=%q UserHomeDir() = %q, want %q", test.home, test.userProfile, got, test.want) + } + } +}