Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign up[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 <github@gone.nl> (cherry picked from commit 22a291f) Signed-off-by: Brian Goff <cpuguy83@gmail.com>
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 <cpuguy83@gmail.com> (cherry picked from commit d021730) Signed-off-by: Brian Goff <cpuguy83@gmail.com>
| package configfile | ||
|
|
||
| func copyFilePermissions(src, dst string) { | ||
| // TODO implement for Windows |
silvin-lubecki
Jul 15, 2020
Contributor
Question: what would be the implementation for windows?
Question: what would be the implementation for windows?
thaJeztah
Jul 16, 2020
Member
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
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
simonferquel
Jul 16, 2020
Contributor
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).
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).
thaJeztah
Jul 16, 2020
Member
Thx! Perhaps I should update the comment and remove "todo"
Thx! Perhaps I should update the comment and remove "todo"
simonferquel
Jul 16, 2020
Contributor
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.
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.
thaJeztah
Jul 16, 2020
Member
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
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
|
LGTM |
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.