Skip to content

Commit

Permalink
Prevent side effects when looking for config files. (#1328)
Browse files Browse the repository at this point in the history
As noted in #1156 (comment), `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
  • Loading branch information
nathanhammond authored Jun 6, 2022
1 parent 3eded01 commit 4062fec
Show file tree
Hide file tree
Showing 3 changed files with 78 additions and 6 deletions.
40 changes: 36 additions & 4 deletions cli/internal/config/config_file.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
Expand Down Expand Up @@ -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)
}

Expand All @@ -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)
}
42 changes: 41 additions & 1 deletion cli/internal/config/config_file_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion cli/internal/login/login_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 4062fec

Please sign in to comment.