search

TritonDataCenter / sdc-docker

Docker Engine for Triton
Mozilla Public License 2.0
183 stars 49 forks source link

DOCKER-458: handle different API versions explicitly #44

Closed misterdjules closed 9 years ago

misterdjules commented 9 years ago

This change addresses @trentm's code review comments about handling different API versions more explicitly regarding the Memory value passed by the docker client when creating a container.

We could be "more liberal in what we accept" by also accepting container.Memory even when the client's API version is >= 1.18, and by accepting container.HostConfig.Memory even when the client's API version is < 1.18, but we would use them as fallbacks.

Also, not necessarily a question for this PR but maybe to avoid problems further down the road, I'm not a big fan of checking the client's API version in code that is not strictly at the API/HTTP params handling layer. It seems that it's how different client API versions are currently handled, so I did it that way.

Would it make sense to create models (for containers and other objects that are used in the API's input/output) that are "version agnostic" to use at the "model" layer, and then parse/send them using the correct format depending on the client API version at the API/HTTP layer?

@trentm @joshwilsdon @twhiteman Thoughts?

trentm commented 9 years ago

LGTM.

Also, not necessarily a question for this PR but maybe to avoid problems further down the road, I'm not a big fan of checking the client's API version in code that is not strictly at the API/HTTP params handling layer. It seems that it's how different client API versions are currently handled, so I did it that way.

Would it make sense to create models (for containers and other objects that are used in the API's input/output) that are "version agnostic" to use at the "model" layer, and then parse/send them using the correct format depending on the client API version at the API/HTTP layer?

Yes, I think that would be nice. Agreed that having clientApiVersion used in the backend layer sucks. Would you like to start a separate ticket for that?

misterdjules commented 9 years ago

@trentm Thanks, landed in 4086be723ba6e89a22e9d60fa8d239eab4edb1bc and created a ticket about handling clientApiVersion at the HTTP/API layer only :+1:

misterdjules commented 9 years ago

The follow-up ticket mentioned above is at https://smartos.org/bugview/DOCKER-482.