[19.03] handle close error on save #2631
Conversation
When running `docker login` or `docker logout`, the CLI updates the configuration file by creating a temporary file, to replace the old one (if exists). When using `sudo`, this caused the file to be created as `root`, making it inaccessible to the current user. This patch updates the CLI to fetch permissions and ownership of the existing configuration file, and applies those permissions to the new file, so that it has the same permissions as the existing file (if any). Currently, only done for "Unix-y" systems (Mac, Linux), but can be implemented for Windows in future if there's a need. Signed-off-by: Sebastiaan van Stijn <[email protected]> (cherry picked from commit 22a291f) Signed-off-by: Brian Goff <[email protected]>
I'm not sure if this fixes anything, however I have seen some weird behavior on Windows where temp config files are left around and there doesn't seem to be any errors reported. Signed-off-by: Brian Goff <[email protected]> (cherry picked from commit d021730) Signed-off-by: Brian Goff <[email protected]>
| package configfile | ||
|
|
||
| func copyFilePermissions(src, dst string) { | ||
| // TODO implement for Windows |
There was a problem hiding this comment.
Question: what would be the implementation for windows?
There was a problem hiding this comment.
Not sure how to copy permissions and/or ownership on Windows. Perhaps @simonferquel would know.
Also not sure if the problem that this was fixing happens on Windows as well
There was a problem hiding this comment.
Hmm, I am not sure about the correct "Windows" way to do that. But anyway I don't think it applies (when you create a file on Windows, by default you inherit ACLs from the parent directory).
There was a problem hiding this comment.
Thx! Perhaps I should update the comment and remove "todo"
There was a problem hiding this comment.
also renaming the function would be welcome, because on the windows implementation it does not do what it claims to do (in the general case it does not copy the permissions, which is fine in this specific case, but could be confusing in the future). Maybe renaming it to replicateConfigFilePermissions or something like this would be preferable.
There was a problem hiding this comment.
Don't think it's needed to include "configfile" in the name, as it's already in this package, and in itself, it's not specific to configfiles.
I'll have a look for changing the comment (and possibly name) on master
Backports:
The first because it seemed at least relevant and the fact that it was missing caused a merge conflict (simple to fix, but still).
Can remove if needed.