Add afero to Config, Refactor Config file access #1156
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,10 +2,12 @@ package config | |
|
|
||
| import ( | ||
| "encoding/json" | ||
| "errors" | ||
| "os" | ||
| "path/filepath" | ||
|
|
||
| "github.com/adrg/xdg" | ||
| "github.com/spf13/afero" | ||
| "github.com/vercel/turborepo/cli/internal/fs" | ||
| ) | ||
|
|
||
|
|
@@ -23,13 +25,27 @@ type TurborepoConfig struct { | |
| TeamSlug string `json:"teamSlug,omitempty" envconfig:"team"` | ||
| } | ||
|
|
||
| func defaultUserConfig() *TurborepoConfig { | ||
| return &TurborepoConfig{ | ||
| ApiUrl: "https://vercel.com/api", | ||
| LoginUrl: "https://vercel.com", | ||
| } | ||
| } | ||
|
|
||
| func defaultRepoConfig() *TurborepoConfig { | ||
| return &TurborepoConfig{ | ||
| ApiUrl: "https://vercel.com/api", | ||
| LoginUrl: "https://vercel.com", | ||
| } | ||
| } | ||
|
|
||
| // writeConfigFile writes config file at a path | ||
| func writeConfigFile(fsys afero.Fs, path fs.AbsolutePath, config *TurborepoConfig) error { | ||
|
There was a problem hiding this comment.
This looks like it clobbers whatever is there? Should we make this a |
||
| jsonBytes, marshallError := json.Marshal(config) | ||
| if marshallError != nil { | ||
| return marshallError | ||
| } | ||
| writeFilErr := fs.WriteFile(fsys, path, jsonBytes, 0644) | ||
| if writeFilErr != nil { | ||
| return writeFilErr | ||
| } | ||
|
|
@@ -38,38 +54,51 @@ func writeConfigFile(path string, config *TurborepoConfig) error { | |
|
|
||
| // WriteRepoConfigFile is used to write the portion of the config file that is saved | ||
| // within the repository itself. | ||
| func WriteRepoConfigFile(fsys afero.Fs, repoRoot fs.AbsolutePath, toWrite *TurborepoConfig) error { | ||
| path := repoRoot.Join(".turbo", "config.json") | ||
| err := fs.EnsureDirFS(fsys, path) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return writeConfigFile(fsys, path, toWrite) | ||
| } | ||
|
|
||
| func userConfigPath(fsys afero.Fs) (fs.AbsolutePath, error) { | ||
| path, err := xdg.ConfigFile(filepath.Join("turborepo", "config.json")) | ||
|
There was a problem hiding this comment.
And the It isn't fully-required as it isn't a code path we'd necessarily care to push through a custom FS, but it'd be nice to be able to use a consistent filesystem abstraction. There was a problem hiding this comment. Ah, unfortunate. Looks like upstream is open to a patch though, which is good. |
||
| if err != nil { | ||
| return "", err | ||
| } | ||
| absPath, err := fs.CheckedToAbsolutePath(path) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return absPath, nil | ||
| } | ||
|
|
||
| // WriteUserConfigFile writes the given configuration to a user-specific | ||
| // 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) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return writeConfigFile(fsys, path, config) | ||
| } | ||
|
|
||
| // readConfigFile reads a config file at a path | ||
| func readConfigFile(fsys afero.Fs, path fs.AbsolutePath, defaults func() *TurborepoConfig) (*TurborepoConfig, error) { | ||
| b, err := fs.ReadFile(fsys, path) | ||
| if err != nil { | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| return nil, nil | ||
| } | ||
| return nil, err | ||
| } | ||
| config := defaults() | ||
| jsonErr := json.Unmarshal(b, config) | ||
| if jsonErr != nil { | ||
| return nil, jsonErr | ||
| } | ||
| if config.ApiUrl == "https://api.vercel.com" { | ||
|
There was a problem hiding this comment. An optional nice-to-have cleanup for this PR would be to go ahead and rewrite this to the correct value on disk or spit out a log message. |
||
| config.ApiUrl = "https://vercel.com/api" | ||
|
|
@@ -78,21 +107,25 @@ func ReadConfigFile(path string) (*TurborepoConfig, error) { | |
| } | ||
|
|
||
| // ReadUserConfigFile reads a user config file | ||
| func ReadUserConfigFile(fsys afero.Fs) (*TurborepoConfig, error) { | ||
| path, err := userConfigPath(fsys) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return readConfigFile(fsys, path, defaultUserConfig) | ||
| } | ||
|
|
||
| // ReadRepoConfigFile reads the user-specific configuration values | ||
| func ReadRepoConfigFile(fsys afero.Fs, repoRoot fs.AbsolutePath) (*TurborepoConfig, error) { | ||
| path := repoRoot.Join(".turbo", "config.json") | ||
| return readConfigFile(fsys, path, defaultRepoConfig) | ||
| } | ||
|
|
||
| // DeleteUserConfigFile deletes a user config file | ||
| func DeleteUserConfigFile(fsys afero.Fs) error { | ||
|
There was a problem hiding this comment. This is a significant improvement, but I'm not sure that we ever actually want this behavior? The call site is There was a problem hiding this comment. I did a little digging, and the only value we use from the user config file is At the risk of pushing a significant change down the road, this area of the codebase might be best handled post- I'm not opposed to splitting out the struct now, but I'm a bit hesitant to tackle any more intelligent configuration merging or rewriting, given that I'm hoping most of this code is not [too] long for this world. |
||
| path, err := userConfigPath(fsys) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return fs.RemoveFile(fsys, path) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| package config | ||
gsoltis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "os" | ||
| "testing" | ||
|
|
||
| "github.com/spf13/afero" | ||
| "github.com/vercel/turborepo/cli/internal/fs" | ||
| ) | ||
|
|
||
| func TestReadRepoConfigWhenMissing(t *testing.T) { | ||
| fsys := afero.NewMemMapFs() | ||
| cwd, err := fs.GetCwd() | ||
| if err != nil { | ||
| t.Fatalf("getting cwd: %v", err) | ||
| } | ||
|
|
||
| config, err := ReadRepoConfigFile(fsys, cwd) | ||
|
There was a problem hiding this comment. So, because of If we ever did a multi-layer FS this sort of weirdness could be an issue. |
||
| if err != nil { | ||
| t.Errorf("got error reading non-existent config file: %v, want <nil>", err) | ||
| } | ||
| if config != nil { | ||
| t.Errorf("got config value %v, wanted <nil>", config) | ||
| } | ||
| } | ||
|
|
||
| func writePartialInitialConfig(t *testing.T, fsys afero.Fs, repoRoot fs.AbsolutePath) *TurborepoConfig { | ||
| path := repoRoot.Join(".turbo", "config.json") | ||
| initial := &TurborepoConfig{ | ||
| TeamSlug: "my-team", | ||
| } | ||
| toWrite, err := json.Marshal(initial) | ||
| if err != nil { | ||
| t.Fatalf("marshal config: %v", err) | ||
| } | ||
| err = fs.WriteFile(fsys, path, toWrite, os.ModePerm) | ||
| if err != nil { | ||
| t.Fatalf("writing config file: %v", err) | ||
| } | ||
| return initial | ||
| } | ||
|
|
||
| func TestRepoConfigIncludesDefaults(t *testing.T) { | ||
| fsys := afero.NewMemMapFs() | ||
| cwd, err := fs.GetCwd() | ||
| if err != nil { | ||
| t.Fatalf("getting cwd: %v", err) | ||
| } | ||
|
|
||
| initial := writePartialInitialConfig(t, fsys, cwd) | ||
|
|
||
| config, err := ReadRepoConfigFile(fsys, cwd) | ||
| if err != nil { | ||
| t.Errorf("ReadRepoConfigFile err got %v, want <nil>", err) | ||
| } | ||
| defaultConfig := defaultRepoConfig() | ||
| if config.ApiUrl != defaultConfig.ApiUrl { | ||
| t.Errorf("api url got %v, want %v", config.ApiUrl, defaultConfig.ApiUrl) | ||
| } | ||
| if config.TeamSlug != initial.TeamSlug { | ||
| t.Errorf("team slug got %v, want %v", config.TeamSlug, initial.TeamSlug) | ||
| } | ||
| } | ||
|
|
||
| func TestWriteRepoConfig(t *testing.T) { | ||
| fsys := afero.NewMemMapFs() | ||
| cwd, err := fs.GetCwd() | ||
| if err != nil { | ||
| t.Fatalf("getting cwd: %v", err) | ||
| } | ||
|
|
||
| initial := &TurborepoConfig{} | ||
| initial.TeamSlug = "my-team" | ||
| err = WriteRepoConfigFile(fsys, cwd, initial) | ||
| if err != nil { | ||
| t.Errorf("WriteRepoConfigFile got %v, want <nil>", err) | ||
| } | ||
|
|
||
| config, err := ReadRepoConfigFile(fsys, cwd) | ||
| if err != nil { | ||
| t.Errorf("ReadRepoConfig err got %v, want <nil>", err) | ||
| } | ||
| if config.TeamSlug != initial.TeamSlug { | ||
| t.Errorf("TeamSlug got %v want %v", config.TeamSlug, initial.TeamSlug) | ||
| } | ||
| defaultConfig := defaultRepoConfig() | ||
| if config.ApiUrl != defaultConfig.ApiUrl { | ||
| t.Errorf("ApiUrl got %v, want %v", config.ApiUrl, defaultConfig.ApiUrl) | ||
| } | ||
| } | ||
|
|
||
| func TestReadUserConfigWhenMissing(t *testing.T) { | ||
| fsys := afero.NewMemMapFs() | ||
| config, err := ReadUserConfigFile(fsys) | ||
| if err != nil { | ||
| t.Errorf("ReadUserConfig err got %v, want <nil>", err) | ||
| } | ||
| if config != nil { | ||
| t.Errorf("ReadUserConfig on non-existent file got %v, want <nil>", config) | ||
| } | ||
| } | ||
|
|
||
| func TestWriteUserConfig(t *testing.T) { | ||
| fsys := afero.NewMemMapFs() | ||
| initial := defaultUserConfig() | ||
| initial.Token = "my-token" | ||
| initial.ApiUrl = "https://api.vercel.com" // should be overridden | ||
| err := WriteUserConfigFile(fsys, initial) | ||
| if err != nil { | ||
| t.Errorf("WriteUserConfigFile err got %v, want <nil>", err) | ||
| } | ||
|
|
||
| config, err := ReadUserConfigFile(fsys) | ||
| if err != nil { | ||
| t.Errorf("ReadUserConfig err got %v, want <nil>", err) | ||
| } | ||
| if config.Token != initial.Token { | ||
| t.Errorf("Token got %v want %v", config.Token, initial.Token) | ||
| } | ||
| // Verify that our legacy ApiUrl was upgraded | ||
| defaultConfig := defaultUserConfig() | ||
| if config.ApiUrl != defaultConfig.ApiUrl { | ||
| t.Errorf("ApiUrl got %v, want %v", config.ApiUrl, defaultConfig.ApiUrl) | ||
| } | ||
|
|
||
| err = DeleteUserConfigFile(fsys) | ||
| if err != nil { | ||
| t.Errorf("DeleteUserConfigFile err got %v, want <nil>", err) | ||
| } | ||
|
|
||
| missing, err := ReadUserConfigFile(fsys) | ||
| if err != nil { | ||
| t.Errorf("ReadUserConfig err got %v, want <nil>", err) | ||
| } | ||
| if missing != nil { | ||
| t.Errorf("reading deleted config got %v, want <nil>", missing) | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This returns
afero.Fs, but in places where we will end up needing it we'll need IOFS interfaces andafero.Lstaterwhich is not implemented byafero.Fs. I'd pitch diving in and doingfsys := &afero.OsFs{}for that reason and then wrapping it iniofsys := afero.IOFS{Fs: fsys}.