Fix bug where incorrect response is returned

[adyanth]

- What I did
Check for server reachability when areSubcommandsSupported fails.

- How I did it
By calling the /_ping API using APIClient.Ping() and reporting the error.

- How to verify it
Run docker checkpoint create test test as a user without privilege to access the docker socket.

- Description for the changelog
Fixes #3844 incorrect error message when server unreachable.

- A picture of a cute animal (not mandatory but encouraged)

[thaJeztah]

Thanks for contributing!

I saw this PR, but wanted to check if we could possibly avoid the additional "ping". I think this code is also used for outputting docker help etc, and I know we have some code that falls back to "use defaults if we weren't able to ping" as for those it's not critical if the connection failed.

That said, when executing the command (as the issue this one is fixing), we should indeed show the correct error.

With the above in mind; perhaps we need to look at better disambiguating "we successfully obtained Information from "ping"" from "we couldn't ping, so we're assuming it's available".

I'll try to make some time to have a look at the code flow, but feel free to give me a ping if I don't get back to you ❤️

[adyanth]

Thanks for taking the time to review!

Yes, this code path is not used for help, and the "ping" only happens if we are about to return an error anyway, which would be an incorrect one if it was due to the ping failing. Help does not check areSubcommandsSupported afaik.

Do let me know if you think of something else!

[adyanth]

Hey @thaJeztah, were you able to take a look?

[adyanth]

Hey @thaJeztah just checking back on this one :)

[thaJeztah]

Thanks for being patient! I dug a bit deeper into "why" this didn't work, as I knew we set a default (Experimental enabled) in cases where we weren't able to reach the daemon to obtain that info.

cli/cli/command/cli.go

Lines 342 to 343 in 7240f70

	// Default to true if we fail to connect to daemon
	cli.serverInfo = ServerInfo{HasExperimental: true}

It turned out that the issue was not due to the Experimental check, but the OSType (Linux or Windows), where we weren't checking if that information was empty (which happens if we weren't able to "ping"). I found some other places where this check was incorrect, and opened #3901

I based that PR on your commit, and added you as Co-Author, so that you should be listed as a contributor.

[adyanth]

Yeah, I saw that default to experimental check, which felt out of place!

I see that you have switched from checking if the Ping returned an error to checking if the OSType is empty or not. I did not want to do that because I am unaware of what would happen when the daemon actually returned an empty string due to an error/bug.

Thank you! Do you think we should close this PR in favor of #3901?

[thaJeztah]

Do you think we should close this PR in favor of #3901?

I added a "closes" in the other PR, so this one will automatically be closed if the other one gets merged.

Debugging a test right now, which fails in CI, but didn't fail locally; looks like that test may be incorrect, but trying to find out what is happening exactly 😅

[thaJeztah]

added a "closes" in the other PR, so this one will automatically be closed if the other one gets merged.

Looks like GitHub didn't trigger that part; #3901 was merged, so let me close this one; thanks for contributing!!