Add afero to Config, Refactor Config file access #1156

gsoltis · 2022-04-29T14:45:19Z

Add afero.Fs to Config
Use fs.AbsolutePath and afero.Fs for reading and writing repo-specific config and user-specific config
Unwind some parametrization for login, now that we can use afero.Fs for tests
Implement a few filesystem manipulation functions in terms of fs.AbsolutePath and afero.Fs

Possible next step: move config_file.go to its own package.

vercel · 2022-04-29T14:45:22Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
turbo-site	✅ Ready (Inspect)	Visit Preview	May 4, 2022 at 5:42PM (UTC)

…Fs and fs.AbsolutePath

nathanhammond

This is an important step toward using afero everywhere, I can't wait to get it landed.

nathanhammond · 2022-05-03T02:09:38Z

cli/cmd/turbo/main.go

@@ -51,6 +52,7 @@ func main() {

 	ui := ui.BuildColoredUi(colorMode)
 	c := cli.NewCLI("turbo", turboVersion)
+	fsys := afero.NewOsFs()


This returns afero.Fs, but in places where we will end up needing it we'll need IOFS interfaces and afero.Lstater which is not implemented by afero.Fs. I'd pitch diving in and doing fsys := &afero.OsFs{} for that reason and then wrapping it in iofsys := afero.IOFS{Fs: fsys}.

nathanhammond · 2022-05-03T02:13:21Z

cli/internal/config/config_file.go

 // writeConfigFile writes config file at a path
-func writeConfigFile(path string, config *TurborepoConfig) error {
+func writeConfigFile(fsys afero.Fs, path fs.AbsolutePath, config *TurborepoConfig) error {


fs.AbsolutePath 🎉

This looks like it clobbers whatever is there? Should we make this a mergeConfigFile that does a get-set? (Most particularly for ApiUrl and LoginUrl, things that may be hand-tweaked.)

nathanhammond · 2022-05-03T02:31:22Z

cli/internal/config/config_file.go

 	jsonErr := json.Unmarshal(b, config)
 	if jsonErr != nil {
-		return config, jsonErr
+		return nil, jsonErr
 	}
 	if config.ApiUrl == "https://api.vercel.com" {


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.

nathanhammond · 2022-05-03T03:06:10Z

cli/internal/config/config_file.go

-// WriteUserConfigFile writes a user config file. This may contain a token and so should
-// not be saved within the repository to avoid committing sensitive data
-func WriteUserConfigFile(config *TurborepoConfig) error {
+func userConfigPath(fsys afero.Fs) (fs.AbsolutePath, error) {
 	path, err := xdg.ConfigFile(filepath.Join("turborepo", "config.json"))


xdg.ConfigFile creates the directory as a side effect: https://github.com/adrg/xdg/blob/v0.4.0/internal/pathutil/pathutil.go#L51

And the Exists call directly calls os.Stat or os.Lstat (depending upon the implementation)

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.

(adrg/xdg#35)

Ah, unfortunate. Looks like upstream is open to a patch though, which is good.

nathanhammond · 2022-05-03T03:57:30Z

cli/internal/config/config_file.go

 }

 // DeleteUserConfigFile deletes a user config file
-func DeleteUserConfigFile() error {
-	return WriteUserConfigFile(&TurborepoConfig{})
+func DeleteUserConfigFile(fsys afero.Fs) error {


This is a significant improvement, but I'm not sure that we ever actually want this behavior? The call site is logout so I feel like we should remove the config for logging-in only.

I did a little digging, and the only value we use from the user config file is Token. I think what we ought to do is stop reusing the same struct to represent both configs.

At the risk of pushing a significant change down the road, this area of the codebase might be best handled post-cobra migration when we bring in viper, which is built on cobra, to help us resolve our configuration situation a bit better.

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.

cli/internal/fs/fs.go

nathanhammond · 2022-05-03T05:15:21Z

cli/internal/fs/path.go

+}
+
+// WriteFile writes the given bytes to the specified file
+func WriteFile(fs afero.Fs, filename AbsolutePath, toWrite []byte, mode os.FileMode) error {


Can we switch all of these to be afero.IOFS? At that point we can have a common interface of return fs.MethodName...

Yes, since IOFS wraps Fs, but IOFS is read-only because io/fs.FS is read-only. So the functions that do writing won't exactly fit the pattern.

I started doing this, but ran into some issues. afero.ReadFile has different behavior from myIOFS.ReadFile() regarding the leading slash, due to the call to ValidPath here.

It looks like Go's io/fs.FS does not allow absolute paths, and further digging shows that io/fs.FS expects non-absolute, platform-independent paths (e.g. ToSlash() has been called). So it is not a candidate for a direct pass-through from our current implementation of AbsolutePath, which is a platform-dependent absolute path.

I can think of a few options:

Store fs.AbsolutePath in a manner consistent with io/fs.FS. I think this would be fine w/ 100% coverage of our filesystem operations, which is the goal anyways

do conversions when using an instance of io/fs.FS. This might be ok, but it makes me nervous.

I don't think it's practical to avoid io/fs.FS? At least not without changes to doublestar? It looks like it's a little bit of an accident that our current usage of doublestar isn't triggering this, it's based on all of the instances of io/fs.FS that we've passed in so far also implement io/fs.ReadDir, which short-circuits the call to Open(), which would fail.

This was the one that I was hoping would work out and ... lol.

Seems like the best ... path ... forward here will be to make this sort of change after we've got path types through the entire system. We've got a large number of path types that we need; I'mma get a PR up.

nathanhammond · 2022-05-03T05:23:23Z

cli/internal/login/login.go

+				ui:       c.UI,
+				logger:   c.Config.Logger,
+				fsys:     c.Config.Fs,
+				repoRoot: c.Config.Cwd,


That's a super-confusing remapping. We should probably not call it cwd if it's actually the repo root.

Heh. It's actually more confusing than it looks. We have potentially 3 paths in play:

The actual working directory of the turbo process

The directory that contains turbo.json -> the "root" of the monorepo

The directory that contains the .git folder

None of them are guaranteed to be the same. We most often care about the second in the list. The .git location is mostly handled by scm, but we need to be careful when using git for hashing. And of course the actual working directory is implied whenever we forget to use an absolute path. cwd is therefore likely the worst name for the field, and is an artifact from the fact that the flag name is --cwd, with the assumption that the working directory should be the directory that contains turbo.json.

I'm going to suggest:

cwd -> turboRoot across the codebase, with a comment on Config explaining what it is (directory containing turbo.json)

do it in a separate PR

nathanhammond · 2022-05-03T05:26:51Z

cli/internal/login/link.go

@@ -57,6 +60,8 @@ func getCmd(config *config.Config, ui cli.Ui) *cobra.Command {
 			link := &link{
 				ui:                  ui,
 				logger:              config.Logger,
+				fsys:                config.Fs,
+				cwd:                 config.Cwd,


Maybe we change the config to split into two fields and populate them with the same value during the transition?

(These things are not the same, even if we're treating them as the same now.)

nathanhammond · 2022-05-03T05:32:04Z

cli/internal/login/unlink.go

@@ -36,7 +36,7 @@ Usage: turbo unlink

 // Run executes tasks in the monorepo
 func (c *UnlinkCommand) Run(args []string) int {
-	if err := config.WriteRepoConfigFile(&config.TurborepoConfig{}); err != nil {
+	if err := config.WriteRepoConfigFile(c.Config.Fs, c.Config.Cwd, &config.TurborepoConfig{}); err != nil {


This is an example of why this property is super-confusing.

nathanhammond

Worth mentioning, I'm really really trying to avoid letting perfect be the enemy of good here. This is strictly an improvement and I don't see anything that should truly stop this from landing.

None of the open comments have anything to do with the implementation at present; they're primarily about additional refactoring options. That should not block this clear improvement from landing so that we're able to make continuous incremental improvements.

gsoltis · 2022-05-04T16:01:08Z

Resolving merge conflicts dismissed reviews...

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

Add afero.Fs to config, rewrite config file access in terms of afero.…

5a89151

…Fs and fs.AbsolutePath

gsoltis force-pushed the gsoltis/hoist_afero branch from 3419aec to 5a89151 Compare May 1, 2022 18:18

gsoltis changed the base branch from gsoltis/sso_verify_status to main May 1, 2022 18:19

vercel bot deployed to Preview May 1, 2022 18:19 View deployment

gsoltis marked this pull request as ready for review May 1, 2022 18:21

gsoltis requested review from jaredpalmer, gaspar09 and nathanhammond as code owners May 1, 2022 18:21

nathanhammond suggested changes May 3, 2022

View reviewed changes

Merge branch 'main' into gsoltis/hoist_afero

673093d

vercel bot deployed to Preview May 3, 2022 18:15 View deployment

nathanhammond added the pr: fixship A PR which is approved with notes and does not require re-review. label May 4, 2022

nathanhammond previously approved these changes May 4, 2022

View reviewed changes

Merge branch 'main' into gsoltis/hoist_afero

d617746

vercel bot deployed to Preview May 4, 2022 03:37 View deployment

Merge branch 'main' into gsoltis/hoist_afero

18e9706

gsoltis dismissed nathanhammond’s stale review via 18e9706 May 4, 2022 16:00

gsoltis requested a review from nathanhammond May 4, 2022 16:00

vercel bot deployed to Preview May 4, 2022 16:01 View deployment

Merge branch 'main' into gsoltis/hoist_afero

799e507

gsoltis added pr: automerge Kodiak will merge these automatically after checks pass and removed pr: fixship A PR which is approved with notes and does not require re-review. labels May 4, 2022

vercel bot deployed to Preview May 4, 2022 17:42 View deployment

nathanhammond approved these changes May 4, 2022

View reviewed changes

kodiakhq bot merged commit 5997590 into main May 4, 2022

kodiakhq bot deleted the gsoltis/hoist_afero branch May 4, 2022 17:54

ooflorent mentioned this pull request May 20, 2022

Error while reading user config file #1270

Closed

nathanhammond mentioned this pull request Jun 3, 2022

Prevent side effects when looking for config files. #1328

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add afero to Config, Refactor Config file access #1156

Add afero to Config, Refactor Config file access #1156

gsoltis commented Apr 29, 2022

vercel bot commented Apr 29, 2022 •

edited

Loading

nathanhammond left a comment

nathanhammond May 3, 2022

nathanhammond May 3, 2022

nathanhammond May 3, 2022

nathanhammond May 3, 2022

nathanhammond May 3, 2022

gsoltis May 3, 2022

nathanhammond May 3, 2022

gsoltis May 3, 2022

nathanhammond May 3, 2022

gsoltis May 3, 2022

gsoltis May 3, 2022

nathanhammond May 4, 2022

nathanhammond May 3, 2022

gsoltis May 3, 2022

nathanhammond May 3, 2022

nathanhammond May 3, 2022

nathanhammond left a comment

gsoltis commented May 4, 2022

Add afero to Config, Refactor Config file access #1156

Add afero to Config, Refactor Config file access #1156

Conversation

gsoltis commented Apr 29, 2022

vercel bot commented Apr 29, 2022 • edited Loading

nathanhammond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanhammond left a comment

Choose a reason for hiding this comment

gsoltis commented May 4, 2022

vercel bot commented Apr 29, 2022 •

edited

Loading