Allow container name property from sparse list #2634
Conversation
When listing containers with `sparse=True` the container's name property does not work because there is no 'Name' attribue on the object returned by the Docker API. Instead the name is in the 'Names' attribute. Signed-off-by: Stuart Thomson <stuartwthomson@live.co.uk>
|
Wondering if any maintainers have a chance to look at this? @ulyssessouza @aiordache (not sure if you are the right people to ping, apologies if not!) |
docker/models/containers.py
Outdated
| @@ -28,6 +28,8 @@ def name(self): | |||
| """ | |||
| if self.attrs.get('Name') is not None: | |||
| return self.attrs['Name'].lstrip('/') | |||
| if self.attrs.get('Names') is not None: | |||
| return self.attrs['Names'][0].lstrip('/') | |||
There was a problem hiding this comment.
According to @thaJeztah and https://github.com/docker/cli/blob/389fa742ff0ebfeda5d94735ac0ca09eac8ecc4f/cli/command/formatter/container.go#L127-L138
The name to be chosen is the one with only one slash (/)
There was a problem hiding this comment.
Maybe I'm misunderstanding something, but it seems like that link actually implies we should return a comma-separated string of the all of the names (with slash prefix removed)?
The confusing bit about that code for me though is the stripNamePrefix() call - I assume that's there to remove the leading slash, but then the code searches for a slash anyway.
There was a problem hiding this comment.
No. Note the break in line https://github.com/docker/cli/blob/389fa742ff0ebfeda5d94735ac0ca09eac8ecc4f/cli/command/formatter/container.go#L133.
About the stripNamePrefix, as the name says, strips only the prefix slash.
We are interested in getting the one that satisfies the condition in https://github.com/docker/cli/blob/389fa742ff0ebfeda5d94735ac0ca09eac8ecc4f/cli/command/formatter/container.go#L131
There was a problem hiding this comment.
Thanks for the clarification @ulyssessouza . Happy to implement the same behaviour here. Out of interest, do you know why we there is a special behaviour for container names not containing a forward slash?
Maybe this is more of a question for @thaJeztah ?
There was a problem hiding this comment.
so a container can have multiple names if it uses the legacy --link option (only when using the default "bridge" network; for any other network, --link is handled at the DNS level).
The additional names would only be used by containers connected to the container through that link. Given that the legacy --link option is effectively deprecated (or at least discouraged) I would just ignore the additional names (unless there's a strong reason to add them).
Note that the container information in the "list" endpoints will always have partial / limited information about the container; I'm not super familiar with the python SDK, but if the expectation is to get a "full" container from the /containers/json ("docker ps") response, I'm wondering if that needs a different type (and to get the full container, do a inspect using the container's ID.
Overall, using the container's ID could be better (as it's immutable, whereas the name can be changed (e.g. docker container rename)
There was a problem hiding this comment.
Thanks for the background @thaJeztah . I am happy to ignore the additional names, although it's not entirely clear to me how to tell the difference between them. The code linked by @ulyssessouza suggests to me that those names maybe have something to do with the presence or absence of slashes in the name? The logic seems to be that Names should contain the first name that has a slash in it (not including a slash in the first position). Am I to suppose that this is how we find the non-legacy name of the container?
Re partial container information it seems to be the decision of the maintainers to control if a container is inspected or not via the sparse flag here. Either way the same class is instantiated, just not necessarily all of the fields exist. The reason I opened this PR is because I was trying to get the names of containers when using 'list' without inspecting but it wasn't actually populated.
Signed-off-by: Stuart Thomson <stuartwthomson@live.co.uk>
|
Wondering if there was any update on this? I addressed the review comments. Thanks! |
|
@ulyssessouza @aiordache |
The 'name' property for a container is currently assumes there is a 'Name' attribute on the object returned by the API call. This works when the container was created with an 'inspect' API call, but if it comes from a list containers call the name is not found. The docker docs indicate that containers listed this way have no 'Name' attribute, and instead have 'Names'. As far as I can tell there can only ever be one of these, so taking the first element of the list seems fine.