Improve readability of Windows connect error#39588
Conversation
|
CI is failing because this branch doesn't have the latest fixes from master (it should pass once rebased) |
thaJeztah
added
platform/windows
status/2-code-review
and removed
status/0-triage
labels
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
left one comment, but if possible, we should try to find what the reason was for the failure; can we somehow detect a "permission denied" and only print the extra information about "need to run privileged" in that case
@vikramhh do you know (or could you check) if that's possible? e.g. would os.IsPermission(err) https://golang.org/pkg/os/#IsPermission work?
client/request.go
Outdated
There was a problem hiding this comment.
Not new (original code already did so), but this should use errors.Wrap so that we preserve the original error;
| err = errors.New("In the default daemon configuration on Windows, the docker client must be run elevated to connect. This error may also indicate that the docker daemon is not running. " + err.Error()) | |
| err = errors.Wrap(err, "in the default daemon configuration on Windows, the docker client must be run elevated to connect. This error may also indicate that the docker daemon is not running. ") |
There was a problem hiding this comment.
I've done the change to wrap the error and used @vikramhh's sample below to check for if the user is privileged so as to direct the error message
zappy-shu
force-pushed
the
DESKTOP-1286-win-admin-error-readability
branch
from
c2bc58c to
ea64a16
Compare
|
Here is how one might do it: https://play.golang.org/p/bBtRZrk4_p I tried both the above and the alternative suggested at golang/go#28804. I suggest using this because it is more simple and robust. @zappy-shu : I noticed the "How to verify it" section above is incomplete.[Please ignore my comment if it is meant to be completed later on]. Also I suggest consistent use of the term "elevated privileges" throughout [or admin mode or elevated permissions, whichever you prefer] instead of mixing these. |
|
Instead of throwing away the opened file handle, I suggest capturing it and calling Close on it (only in case of success). This is the pattern shown in documentation of Open at https://golang.org/src/os/file.go?s=8458:8524#L272 |
zappy-shu
force-pushed
the
DESKTOP-1286-win-admin-error-readability
branch
from
92c759a to
db1c623
Compare
|
@vikramhh apologies for the delay in updating the description. Hopefully that is better. I'm happy to squash the commits as well if that helps |
|
LGTM Thanks you, Nick, for accepting my suggestions. |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
left one comment 😅 but otherwise looks good
GordonTheTurtle
added
the
dco/no
|
Please sign your commits following these rules: $ git clone -b "DESKTOP-1286-win-admin-error-readability" [email protected]:zappy-shu/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842359069840
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
zappy-shu
force-pushed
the
DESKTOP-1286-win-admin-error-readability
branch
from
385810b to
0784fa8
Compare
GordonTheTurtle
removed
the
dco/no
zappy-shu
force-pushed
the
DESKTOP-1286-win-admin-error-readability
branch
from
0784fa8 to
8a38ed5
Compare
Improve the readability of the connection error displayed to the user on Windows when running docker commands fails by checking if the client is privileged. If so then display the actual error wrapped in a generic error "This error may indicate that the docker daemon is not running." If not that display the actual error wrapped in a more specific error: "In the default daemon configuration on Windows, the docker client must be run with elevated privileges to connect." Signed-off-by: Nick Adcock <[email protected]>
zappy-shu
force-pushed
the
DESKTOP-1286-win-admin-error-readability
branch
from
8a38ed5 to
1a5dafb
Compare
|
ping @tiborvass @kolyshkin PTAL |
|
marking this for cherry-picking, because this should be low-risk, and there's often confusion about this on Docker Desktop users (but will need a re-vendor in the CLI after it's been backported) |
7 participants
- What I did
Improve the readability of the connection error displayed to the user on Windows when running docker commands when either the user is not running in admin mode or the daemon is not running.
- How I did it
This is done putting the help message first and putting the error details, which are difficult to
diagnose, after.
- How to verify it
On Windows, run a docker command (e.g.
ps) without the daemon running and again with the daemon running but without elevated permissions. The displayed error message should start withIn the default daemon configuration on Windows, the docker client must be run with elevated privileges to connect.Run again without the daemon running but with elevated permissions. The displayed error message should start with
This error may indicate that the docker daemon is not running.- Description for the changelog
Improve error message shown on Windows when daemon is not running or client does not have elevated permissions
- A picture of a cute animal (not mandatory but encouraged)