-
Notifications
You must be signed in to change notification settings - Fork 8
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
FileConfig consistency with dockerpy #171
Conversation
Current coverage is 85.13% (diff: 100%)@@ master #171 diff @@
==========================================
Files 36 36
Lines 1374 1379 +5
Methods 0 0
Messages 0 0
Branches 130 130
==========================================
+ Hits 1169 1174 +5
Misses 166 166
Partials 39 39
|
@@ -77,18 +70,18 @@ def __init__(self, *args, **kwargs): | |||
self.docker_host = "unix://var/run/docker.sock" | |||
|
|||
self.tls_verify = (env.get("DOCKER_TLS_VERIFY", "") != "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOCKER_TLS_VERIFY=0
would evaluate to True
here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOCKER_TLS_VERIFY is not a 1=true 0=false . It's a "defined" vs "not defined" flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. How strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks to me something that future developers might unknowingly change it.
Maybe adding moby/moby#22411 to the code as comment. We don't need to do it here, we can log this as one of the auditing issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really strange, it's quite common to test for definedness, and docker is not the only one doing that. Putting 1 is just a convention. It could be anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevertheless, issue 22411 on docker and its referenced issues show that I am not the only one surprised by this ;)
Looks good to merge. The single comment is orthogonal to this PR, so could be fixed separately. |
Fixes #149