Refactor to use os.UserHomeDir()#2740
Conversation
mislav
left a comment
There was a problem hiding this comment.
Thanks, this is a good start!
For the sake of backwards compatibility, however, we should keep the dependency on go-homedir as part of the migration path. In the original issue, I stressed this out:
It's important to note that whenever we add new logic about config file storage, we need to ensure a migration path for users who already have configuration files in the old location.
So, whenever we try to read from a config file, we should check both the new location and the old location, and automatically move the file from old to new location if found. This migratory logic shall stay around until the next major release in the future, when we will remove it.
See #2429 (comment) for more context.
|
Ok so I just made the following changes:
|
mislav
left a comment
There was a problem hiding this comment.
This looks good! If you have time, I've requested some changes that will make the logic safer & clearer.
internal/config/config_file.go
Outdated
|
|
||
| // Get the absolute path for a folder or file. Support for the old homedir method | ||
| // Tied to https://github.com/cli/cli/pull/2740 | ||
| func GetAbsolutePath(elem ...string) (string, error) { |
There was a problem hiding this comment.
Could this be named HomeDirPath?
Also, could the name somehow reflect that auto-renaming will happen if the old home location was found? Basically, the idea is that no new callers should ever use this function. It's only to be used in places where we currently relied on mitchellh/go-homedir.
internal/config/config_file.go
Outdated
| fullOldPath := filepath.Join(append([]string{oldHomeDir}, elem...)...) | ||
| fullPath := filepath.Join(append([]string{homeDir}, elem...)...) | ||
|
|
||
| if fullOldPath != fullPath { |
There was a problem hiding this comment.
Could we avoid renaming if elem was empty? This is to have a guard against ever renaming the literal user's home directory, which of course we never want to move/rename.
|
Ok so I changed the following:
|
mislav
left a comment
There was a problem hiding this comment.
Thank you! This looks great.
I've pushed an optimization that avoids ever running go-homedir functionality until the fallback path is activated.
Part of #2470