Pass through port binding dict

[rca]

Upon upgrading docker and docker-py my code stopped working because I was passing the port bindings as a properly formatted list of dict objects. This patch re-allows passing the ports in the already-correct format.

Please let me know if this is a good fix and/or if it needs some tweaks.

Thanks!

[mpetazzoni]

Could you fix the style error and squash these two commits together? Thanks!

[denibertovic]

I'm not too happy about having 2 ways to use ports. Am I the only one? I think we broke backwards compatibility with that feature on purpose as to have a more Pythonic API.

[rca]

@mpetazzoni Sure, no problem, but can you elaborate on the style error? Also, going to ensure this is the right direction first.

@denibertovic If this was done intentionally, then no problem. I just went to the README and see that the format is well-documented, so my fault for not reading! :)

I agree with having one known way of doing things; close at your leisure.

[rca]

Actually, what if the else statement is changed so that it will fail if the binding param is not a string or int. A dict should not be stringified and used as the port; I spent a good amount of time figuring out why my port bindings stopped working.

[denibertovic]

Personally I don't like doing defensive coding like that in a dynamic language. If one passes incorrect parameters the API will tell us. But maybe @mpetazzoni and @shin- can weigh in.

[rca]

Is silently doing the wrong thing better?

I realize that the library is under heavy dev, so backwards incompatible changes are to be expected (and the docs clearly describe the parameter format), but just to illustrate, I ended up with a stringified dict as the HostPort setting, which is clearly wrong. And Docker upstream, as far as I can tell, silently ignored the port value and simply assigned a dynamic port.

Anyway, not trying to be a pain here, just describing the issue I ran into.

Thanks!

[rca]

@denibertovic What are your thoughts on something like this? I suppose you'd still consider this defensive coding, but do we agree that HostPort must be numeric?

[denibertovic]

Yes we agree that we should not let wrong values just fly like that, i'm just trying to limit the number of times we need to do stuff with type and isistance :)

I think it's maybe worth looking into why the API didn't raise an error if it got malformed values from the client.

This does seem better, although this can also raise a ValueError (ie int('some string')) not just a TypeError.

Can you paste the example that you used that produced the initial error. What got sent to the API (and the result was a random port assigned right?)

[rca]

Yes, it can raise a ValueError and TypeError, but I'm not processing that in the actual code, just using it in the test case. I added a test that asserts the ValueError, just in case.

Here's the binding that brought us here today. Feed this into def _convert_port_binding(binding):

{'HostPort': '49159', 'HostIp': '0.0.0.0'}

And get this back:

{'HostPort': "{'HostPort': '49159', 'HostIp': '0.0.0.0'}", 'HostIp': ''}

[rca]

@denibertovic And to answer your question, yes, port randomly assigned and IP ignored.

[denibertovic]

@shin- what do you tnink?

[shin-]

Not too sure about this, let me play with it a bit first!

Ideally, if we can accomodate both ways of doing things (i.e. passing arguments in the official CLI format + passing arguments in the python friendly format) I'd like us to do that.

[rca]

Unfortunately I've been force updating but the initial patch checked to see
if the param was a dict and had the right keys.

I can resurrect the patch, but it will be Monday before I can do so.

Let me know.

On Friday, January 10, 2014, Joffrey F wrote:

Not too sure about this, let me play with it a bit first!

Ideally, if we can accomodate both ways of doing things (i.e. passing
arguments in the official CLI format + passing arguments in the python
friendly format) I'd like us to do that.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/135#issuecomment-32036414
.