Skip to content

Conversation

grahamlyons
Copy link
Contributor

@grahamlyons grahamlyons commented Jun 8, 2017

The from_env method on the docker module passed None as the value for the timeout keyword argument which overrode the default value in the initialiser, taken from constants module.

This sets the default in the initialiser to None and adds logic to set that, in the same way that version is handled.

This sets the default in from_env to the default defined in the constants module.

Addresses #1374

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "readtimeout_calling_container_stop" git@github.com:grahamlyons/docker-py.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

The `from_env` method on the `docker` module passed `None` as the
value for the `timeout` keyword argument which overrode the default
value in the initialiser, taken from `constants` module.

This sets the default in the initialiser to `None` and adds logic
to set that, in the same way that `version` is handled.

Signed-off-by: grahamlyons <graham@grahamlyons.com>
@grahamlyons grahamlyons force-pushed the readtimeout_calling_container_stop branch from e5f87fc to ee75a1c Compare June 8, 2017 13:39
@grahamlyons
Copy link
Contributor Author

Is the "janky" check configured correctly? It doesn't have a link to a Jenkins job and it hasn't completed on the last large handful of pull requests.

@grahamlyons
Copy link
Contributor Author

I'm not sure what the failure on Jenkins relates to; can anybody assist with that?

@shin-
Copy link
Contributor

shin- commented Jun 8, 2017

Please ignore the janky test... It's an obsolete test, but we somehow haven't been able to disable the corresponding webhook.

Copy link
Contributor

@shin- shin- left a comment

Choose a reason for hiding this comment

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

My only issue with this PR is that None has a special meaning for timeouts (requests docs).

We'd like to be able to pass `None` as a value for `timeout` because
it has meaning to the `requests` library
(http://docs.python-requests.org/en/master/user/advanced/#timeouts)

Signed-off-by: grahamlyons <graham@grahamlyons.com>
@grahamlyons grahamlyons force-pushed the readtimeout_calling_container_stop branch from 04a5dcb to ff993dd Compare June 9, 2017 08:52
@grahamlyons
Copy link
Contributor Author

@shin-, is that the kind of change you envisaged? Let me know if not.

@shin-
Copy link
Contributor

shin- commented Jun 14, 2017

Yeah, I think that works! Thank you!

@shin- shin- added this to the 2.4.0 milestone Jun 14, 2017
@shin- shin- merged commit 1eef700 into docker:master Jun 14, 2017
@grahamlyons grahamlyons deleted the readtimeout_calling_container_stop branch June 14, 2017 22:22
@grahamlyons
Copy link
Contributor Author

Awesome. Thanks, @shin-

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.

3 participants