Fix bug where incorrect response is returned [carry 3845] #3901
Conversation
| si := details.ServerInfo() | ||
| if ost, ok := curr.Annotations["ostype"]; ok && si.OSType != "" && ost != si.OSType { |
There was a problem hiding this comment.
In cases where we were not able to connect with the daemon, we set Experimental to "true", but as we don't know the daemon's OSType, we can't set that value, so in that case we should ignore these;
Lines 342 to 343 in 7240f70
We can probably optimise and detect that we didn't have a valid ping, and skip all of these checks, but leaving that for a follow-up.
|
Looks like I have one test to fix / verify; |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3901 +/- ##
=======================================
Coverage 59.22% 59.22%
=======================================
Files 287 287
Lines 24723 24723
=======================================
Hits 14641 14641
Misses 9197 9197
Partials 885 885 |
thaJeztah
force-pushed
the
carry_3845
branch
7 times, most recently
from
07c5b89
to
a60a9a1
Compare
|
Hm.. right, so the "unit" test is actually using a docker client, which tries to connect to a daemon (which isn't there); Then the check if buildkit must be used is based on the (non-existing) ping result; if that's "windows", it's okay, but it it's non-windows, then it should print an error. As (at this stage) we don't know if the daemon is windows or linux, we probably shouldn't produce an error (which is what this PR does), but the test expects that error to happen. |
When server is unreachable and docker checkpoint (or any command that
needs to check the server type) is run, incorrect error was returned.
When checking if the daemon had the right OS, we compared the OSType
from the clients ServerInfo(). In situations where the client cannot
connect to the daemon, a "stub" Info is used for this, in which we
assume the daemon has experimental enabled, and is running the latest
API version.
However, we cannot fill in the correct OSType, so this field is empty
in this situation.
This patch only compares the OSType if the field is non-empty, otherwise
assumes the platform matches.
before this:
docker -H unix:///no/such/socket.sock checkpoint create test test
docker checkpoint create is only supported on a Docker daemon running on linux, but the Docker daemon is running on
with this patch:
docker -H unix:///no/such/socket.sock checkpoint create test test
Cannot connect to the Docker daemon at unix:///no/such/socket.sock. Is the docker daemon running?
Co-authored-by: Adyanth Hosavalike <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
|
Thx! |
| @@ -419,10 +419,11 @@ func areSubcommandsSupported(cmd *cobra.Command, details versionDetails) error { | |||
| if cmdVersion, ok := curr.Annotations["version"]; ok && versions.LessThan(details.CurrentVersion(), cmdVersion) { | |||
| return fmt.Errorf("%s requires API version %s, but the Docker daemon API version is %s", cmd.CommandPath(), cmdVersion, details.CurrentVersion()) | |||
| } | |||
| if ost, ok := curr.Annotations["ostype"]; ok && ost != details.ServerInfo().OSType { | |||
| return fmt.Errorf("%s is only supported on a Docker daemon running on %s, but the Docker daemon is running on %s", cmd.CommandPath(), ost, details.ServerInfo().OSType) | |||
| si := details.ServerInfo() | |||
When server is unreachable and docker checkpoint (or any command that needs to check the server type) is run, incorrect error was returned.
When checking if the daemon had the right OS, we compared the OSType from the clients ServerInfo(). In situations where the client cannot connect to the daemon, a "stub" Info is used for this, in which we assume the daemon has experimental enabled, and is running the latest API version.
However, we cannot fill in the correct OSType, so this field is empty in this situation.
This patch only compares the OSType if the field is non-empty, otherwise assumes the platform matches.
before this:
with this patch:
- A picture of a cute animal (not mandatory but encouraged)