From 4062fecb547058dded7008b076f78234ca7a4e84 Mon Sep 17 00:00:00 2001 From: Nathan Hammond Date: Mon, 6 Jun 2022 04:38:31 +0000 Subject: [PATCH] Prevent side effects when looking for config files. (#1328) As noted in https://github.com/vercel/turborepo/pull/1156#discussion_r863341102, `xdg.ConfigFile` has side effects. One of the consequences of the side effects is that sometimes, when simply wanting to check for existence, we would error out entirely because the path wasn't writable. This splits the process of creating and reading a config file path, which means that the error won't be encountered unless a user is explicitly trying to create a configuration file. (Which *should* be an error.) Fixes #1270 --- cli/internal/config/config_file.go | 40 ++++++++++++++++++++--- cli/internal/config/config_file_test.go | 42 ++++++++++++++++++++++++- cli/internal/login/login_test.go | 2 +- 3 files changed, 78 insertions(+), 6 deletions(-) diff --git a/cli/internal/config/config_file.go b/cli/internal/config/config_file.go index 6165eaaef23d0..8cc7025fdf39c 100644 --- a/cli/internal/config/config_file.go +++ b/cli/internal/config/config_file.go @@ -63,7 +63,21 @@ func WriteRepoConfigFile(fsys afero.Fs, repoRoot fs.AbsolutePath, toWrite *Turbo return writeConfigFile(fsys, path, toWrite) } -func userConfigPath(fsys afero.Fs) (fs.AbsolutePath, error) { +func getUserConfigPath(fsys afero.Fs) (fs.AbsolutePath, error) { + path, err := xdg.SearchConfigFile(filepath.Join("turborepo", "config.json")) + // Not finding an existing config file is not an error. + // We simply bail with no path, and no error. + if err != nil { + return "", nil + } + absPath, err := fs.CheckedToAbsolutePath(path) + if err != nil { + return "", err + } + return absPath, nil +} + +func createUserConfigPath(fsys afero.Fs) (fs.AbsolutePath, error) { path, err := xdg.ConfigFile(filepath.Join("turborepo", "config.json")) if err != nil { return "", err @@ -79,7 +93,7 @@ func userConfigPath(fsys afero.Fs) (fs.AbsolutePath, error) { // configuration file. This is for values that are not shared with a team, such // as credentials. func WriteUserConfigFile(fsys afero.Fs, config *TurborepoConfig) error { - path, err := userConfigPath(fsys) + path, err := createUserConfigPath(fsys) if err != nil { return err } @@ -108,10 +122,19 @@ func readConfigFile(fsys afero.Fs, path fs.AbsolutePath, defaults func() *Turbor // ReadUserConfigFile reads a user config file func ReadUserConfigFile(fsys afero.Fs) (*TurborepoConfig, error) { - path, err := userConfigPath(fsys) + path, err := getUserConfigPath(fsys) + + // Check the error first, that means we got a hit, but failed on path conversion. if err != nil { return nil, err } + + // Otherwise, we just didn't find anything, which isn't an error. + if path == "" { + return nil, nil + } + + // Found something! return readConfigFile(fsys, path, defaultUserConfig) } @@ -123,9 +146,18 @@ func ReadRepoConfigFile(fsys afero.Fs, repoRoot fs.AbsolutePath) (*TurborepoConf // DeleteUserConfigFile deletes a user config file func DeleteUserConfigFile(fsys afero.Fs) error { - path, err := userConfigPath(fsys) + path, err := getUserConfigPath(fsys) + + // Check the error first, that means we got a hit, but failed on path conversion. if err != nil { return err } + + // Otherwise, we just didn't find anything, which isn't an error. + if path == "" { + return nil + } + + // Found a config file! return fs.RemoveFile(fsys, path) } diff --git a/cli/internal/config/config_file_test.go b/cli/internal/config/config_file_test.go index 3380bfd450e1e..e0180e3d70aa3 100644 --- a/cli/internal/config/config_file_test.go +++ b/cli/internal/config/config_file_test.go @@ -9,6 +9,46 @@ import ( "github.com/vercel/turborepo/cli/internal/fs" ) +func Test_UserConfigPath(t *testing.T) { + fsys := afero.NewMemMapFs() + + t.Run("get doesn't create a path", func(t *testing.T) { + // XDG is not filesystem aware. Clean up first. + path, _ := createUserConfigPath(fsys) + err := os.Remove(path.Dir().ToString()) + if err != nil { + t.Errorf("failed to clean up first: %v", err) + } + + getConfigPath, getConfigPathErr := getUserConfigPath(fsys) + if getConfigPathErr != nil { + t.Errorf("failed to run getUserConfigPath: %v", getConfigPathErr) + } + + // The main thing we want to do is make sure that we don't have side effects. + // We know where it would attempt to create a directory already. + if getConfigPath == "" { + getConfigPath = path + } + + getConfigDir := getConfigPath.Dir() + getCheck, _ := os.Stat(getConfigDir.ToString()) + if getCheck != nil { + t.Error("getUserConfigPath() had side effects.") + } + + createConfigPath, createErr := createUserConfigPath(fsys) + if createErr != nil { + t.Errorf("createUserConfigPath() errored: %v.", createErr) + } + createConfigDir := createConfigPath.Dir() + createCheck, _ := os.Stat(createConfigDir.ToString()) + if createCheck == nil { + t.Error("createUserConfigPath() did not create the path.") + } + }) +} + func TestReadRepoConfigWhenMissing(t *testing.T) { fsys := afero.NewMemMapFs() cwd, err := fs.GetCwd() @@ -102,7 +142,7 @@ func TestReadUserConfigWhenMissing(t *testing.T) { } func TestWriteUserConfig(t *testing.T) { - fsys := afero.NewMemMapFs() + fsys := afero.NewOsFs() initial := defaultUserConfig() initial.Token = "my-token" initial.ApiUrl = "https://api.vercel.com" // should be overridden diff --git a/cli/internal/login/login_test.go b/cli/internal/login/login_test.go index d905283751e4a..6c07c22994a2a 100644 --- a/cli/internal/login/login_test.go +++ b/cli/internal/login/login_test.go @@ -105,7 +105,7 @@ func (tr *testResult) getTestLogin() login { } func newTest(t *testing.T, redirectedURL string) *testResult { - fsys := afero.NewMemMapFs() + fsys := afero.NewOsFs() stepCh := make(chan struct{}, 1) cwd, err := fs.GetCwd() if err != nil {