Docker Swarm Inspect Container Info Support - #459
Conversation
Signed-off-by: Gurkan Erdogdu <gerdogdu@managecat.com>
|
Sorry, but swarm support will be delayed to 3.1.0. Plus it would require integration and functional tests. Change of the field name smells like regression for non swarm containers. |
|
hi gerdogdu , swam related api I can't see in master branch |
| private String name; | ||
|
|
||
| @JsonProperty("Id") | ||
| @JsonProperty("ID") |
There was a problem hiding this comment.
That would break backward compatibility if this field was right and contains data for docker API before.
There was a problem hiding this comment.
There was a problem hiding this comment.
It may happen that different calls has different internals in the same Node object, then it should be splitted.
If Id exists in docker api, then you can't just remove it.
There was a problem hiding this comment.
@suryagaddipati A node as returned by the /events API, for instance, when Swarm is used.
then it may be that swarm changed API. You should try get responces from 2 different swarm versions.
There was a problem hiding this comment.
Swarm Node always had the same api response, there is no difference between versions.
Can you link me to where swarm node api changed? I can't seem to find it, perhaps I missed it.
There was a problem hiding this comment.
https://docs.docker.com/swarm/swarm-api/
"Node": {
"Id": "ODAI:IC6Q:MSBL:TPB5:HIEE:6IKC:VCAM:QRNH:PRGX:ERZT:OK46:PMFX",
"Ip": "0.0.0.0",
Smells like Node is reused... will verify and think on creating ITs.
There was a problem hiding this comment.
hi @KostyaSha those docs seem out of date
https://github.com/docker/swarm/blob/master/scheduler/node/node.go#L11-L12
the case did indeed seems to have been changed by this commit, like you suspected.
probably a good idea to change Node class to have both fields and return the one that is not null. what do you think?
There was a problem hiding this comment.
@KostyaSha i created an issue on docker swarm docker-archive/classicswarm#2378
to clarify the breaking changes.
There was a problem hiding this comment.
So i would suggest to make new SwarmNode class as it seems that it fully other Node objects and current docker Node class shouldn't be changed.
|
hi @KostyaSha thank you for your quick response. What can I help do to get this merged? Looks like this PR has been open for over 4 months( which like 4 yrs in dockerland :)) . |
|
tests |
|
And review my comment about IDs, changing something just for swarm shouldn't break docker api. |
|
#383 related. |
Current coverage is 23.39% (diff: 0.00%)
|
|
#711 is what i expected :( but only now got time. |
|
AFAIR everything is in master... |
Fixes #458 .
When using Docker Swarm, containers/{id}/json does not contains Node information. This reqeust adds Node related information to the InspectContainerResponse.