Failed to update config file on windows #210
Comments
|
Ouch, sorry for this problem. Maybe we can fix renameio to work on Windows too though. /cc @stapelberg |
|
I’d love to receive a fix for this in renameio, but have no idea about windows myself :) |
|
Atomic file replacement in windows seems almost impossible unless with hardcoded routines |
|
@matthiasng I'm not sure how to do it, but is there any chance you could hack your renameio, replacing |
|
We can write a temporary fix for glab by eliminating the https://github.com/google/renameio/blob/353f8196982447d8b12c64f69530e657331e3dbc/writefile.go#L29 with: if runtime.GOOS != "windows" {
if err := t.Chmod(perm); err != nil {
return err
}
} |
|
I've submitted google/renameio#21 which was tested with a trivial test program importing my forked version from |
|
Following your PR in renameio it seems that this library will completely drop windows support, which in my opinion is right desigion because there is no reliable way to gurantee a atomic rename on windows. I understand that with #170 the config loading and saving is getting to replaces by viper, am i right ? Therefore, i think that a simple workaround for windows is good enought at the moment. |
|
FYI example at google/renameio#21 (comment) - the switch statement could just be dropped in place of the only use of renameio.WriteFile that currently exists in glab trunk. I'm thinking the need of importing/linking Since I feel somewhat responsibel for creating this problem I'm willing to spend some more time on fixing things up. Please let me know if this is something you want and in which direction you'd like to see the implementation go. (Also just simply switching to ioutil.WriteFile would still be an improvement over the previous code that was replaced with renameio.WriteFile. Which might be the most straight forward solution to avoid spending time on this regression if there are other more urgent things to adress to get a new release out the door.) |
|
I think renameio only fixes the sympton and not the root cause. There are several variables in the configuration, and for each of them glab reads and write the configration separately. I think reading and writing the entire file only once for all variable should be good enough (not in scope of this issue but an implicite upvote for #170). I have also never seen a cli tool that uses atomic write operations for configfiles Therefore, I think that it is not worth investing a lot of time because a simple ioutil. WriteFile should be enough. |
As reported apparently renameio does not work on windows and there's no obvious way to even make an atomic file overwrite on windows at all, thus renameio is looking at dropping Windows support. This change simply switches to ioutil.WriteFile which is not atomic but the code should still be nicer than the previous one we replaced. While at it also cut down on the readability part of the files permissions as the config file could contain sensitive information (like the token). Fixes profclems#210
If you’re okay with emptying out your config file when you run out of disk space, Personally, I think it’s better to avoid these cases, so I recommend using renameio.WriteFile where available. |
As reported renameio doesn't work on Windows and apparently there's no way to do atomic file replace on Windows. We thus create a helper function in a separate file that uses renameio but say it should not be built on windows, then hook in a windows- specific implementation of that function which uses ioutil.WriteFile which is not safe but gets the job done. While at it also tighten up the permissions on the config file as it might contain sensitive information (like the token). Fixes profclems#210
As reported renameio doesn't work on Windows and apparently there's no way to do atomic file replace on Windows. We thus create a helper function in a separate file that uses renameio but say it should not be built on windows, then hook in a windows- specific implementation of that function which uses ioutil.WriteFile which is not safe but gets the job done. While at it also tighten up the permissions on the config file as it might contain sensitive information (like the token). Fixes #210
Running
glab config -t ...failes withFailed to update config file: chmod C:\Users\...\AppData\Local\Temp\..env337738213: not supported by windowsDescription
glab/internal/config/config.go
Line 162 in 3ff88b6
renameio.WriteFile uses File.Chmod which is not supported on windows.
Expected Behavior
glab configcommand can write the config fileActual Behavior
glab configfailes when writing the config filePossible Fix
Remove renameio and add some error handling to the open-write go code.
I think that should be enough to guarantee (99.99... %) not to destroy the config file.
@andhe please correct me if i'm wrong.
Alternative, fix google/renameio#17
Steps to Reproduce
glab config -t 123Your Environment
The text was updated successfully, but these errors were encountered: