Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixing docstring to match behaviour #2640

Closed
wants to merge 1 commit into from
Closed

Conversation

nadavwe
Copy link

@nadavwe nadavwe commented Aug 11, 2020

i think this might have been missed in #2512.

(on a side note, I would love to see the default being "auto", as for basic usage, I don't really care about the version of docker).

Thanks!

@thaJeztah
Copy link
Member

Thanks! Yes, looks like I missed these 😅

on a side note, I would love to see the default being "auto", as for basic usage, I don't really care about the version of docker

Agreed! I'm not really a python coder myself, so haven't looked into how it would be implemented in Python (but contributions would be welcome).

it could use the same logic (could still be configurable to enable/disable it), as https://github.com/moby/moby/blob/ecdb0b22393bb669325099320d26d18687425e5f/client/client.go#L200-L246

(and moby/moby#39032, which handles the API version negotiation the moment a request will be made)

@thaJeztah
Copy link
Member

Oh! I see you probably made the changes through GitHub's web UI, and didn't add a DCO-sign-off in the commit message (see https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work).

It's possible to amend your commit-message to fix that, but you'll have to do that from the command-line (unfortunately can't be done through GitHub's web UI 😞)

Let me know if you need help doing so, then I can post some instructions if needed.

Signed-off-by: Nadav Wexler <nadavwe@wix.com>
@nadavwe
Copy link
Author

nadavwe commented Aug 11, 2020

fixed the dco.
looks like the auto thing already has some support, no need to do anything...

@thaJeztah
Copy link
Member

Ah, you're right; looks like there's something in place in

elif isinstance(version, six.string_types):
if version.lower() == 'auto':
self._version = self._retrieve_server_version()
else:
self._version = version
else:
raise DockerException(
'Version parameter must be a string or None. Found {0}'.format(
type(version).__name__
)
)
if utils.version_lt(self._version, MINIMUM_DOCKER_API_VERSION):
raise InvalidVersion(
'API versions below {} are no longer supported by this '
'library.'.format(MINIMUM_DOCKER_API_VERSION)
)
def _retrieve_server_version(self):
try:
return self.version(api_version=False)["ApiVersion"]
except KeyError:
raise DockerException(
'Invalid response from docker daemon: key "ApiVersion"'
' is missing.'
)
except Exception as e:
raise DockerException(
'Error while fetching server API version: {0}'.format(e)
)

Looks like it's currently calling the /version endpoint. For recent versions of the daemon, using the /_ping endpoint may be slightly more lightweight for that (https://docs.docker.com/engine/api/v1.40/#operation/SystemPingHead), but not critical I guess.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (not a maintainer)

@nadavwe
Copy link
Author

nadavwe commented Aug 23, 2020

even better! this has been solved in #2650

@nadavwe nadavwe closed this Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants