store gh config in %APPDATA% on windows os #2080
Conversation
There was a problem hiding this comment.
I'm fully in support of using the user's AppData folder on Windows to store configuration and authentication credentials.
However, we cannot merge something like this without also putting in some migration code in place that would ensure that for existing users, the config from under ~/.config/gh/ will still be respected and migrated to a new location. Otherwise, when we ship this in a future update, existing Windows users will be forced to re-authenticate and will have lost their previous configuration.
I'm not asking you to necessarily add the migration code to this PR, but if you could just address the line comments for start, that would be great. Thank you!
internal/config/config_file.go
Outdated
|
|
||
| func userHomeDir() string { | ||
| if runtime.GOOS == "windows" { | ||
| home := os.Getenv("HOMEDRIVE") + os.Getenv("HOMEPATH") |
There was a problem hiding this comment.
Did you intentionally avoid using homedir.Dir() here?
There was a problem hiding this comment.
I didn't intentional avoid using homedir.Dir. I have since removed this in the latest commit
internal/config/config_file.go
Outdated
| dirPath := userHomeDir() | ||
|
|
||
| if runtime.GOOS == "windows" { | ||
| dirPath += "\\AppData\\Local\\Github Cli" |
There was a problem hiding this comment.
Protip: you can use filepath.Join() here
filepath.Join(dirPath, "AppData", "Local", "GitHub CLI")However:
- Shouldn't we be reading from the
%AppData%environment variable for this? - Should we consider using
Roaminginstead ofLocalso that the user's gh config gets synced between their computers? CLIshould be spelled uppercase.
removed unnecessary code to get home dir
…into appdata_configPath
There was a problem hiding this comment.
@RozzaysRed Thank you for the updates!
This looks good. We'll take it over from here 🙇
| dir, _ := homedir.Expand("~/.config/gh") | ||
| dirPath := "" | ||
| if runtime.GOOS == "windows" { | ||
| dirPath = filepath.Join(os.Getenv("APPDATA"), "Github CLI") |
There was a problem hiding this comment.
| dirPath = filepath.Join(os.Getenv("APPDATA"), "Github CLI") | |
| dirPath = filepath.Join(os.Getenv("APPDATA"), "GitHub CLI") |
|
I'll be taking this over with a goal of adding migration code for existing Windows users. |
|
Going to close this in favor of #3671 |
Instead of storing gh config in the Home path. The gh config will now be stored in "%APPDATA%\Local\Github CLI" for Windows OS
Address #1944