Skip to content
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

ContainerCollection.run sets tag to "latest" if not set. #2656

Conversation

dan-hipschman
Copy link

This fixes issue #2545.

The issue is when you call client.containers.run("some-image") and some-image:latest doesn't exist locally. run will call ImageCollection.pull without passing a tag:

try:
container = self.create(image=image, command=command,
detach=detach, **kwargs)
except ImageNotFound:
self.client.images.pull(image, platform=platform)
container = self.create(image=image, command=command,
detach=detach, **kwargs)

Since the tag doesn't exist, pull will pull all tags from the repository:

def pull(self, repository, tag=None, **kwargs):
"""
Pull an image of the given name and return it. Similar to the
``docker pull`` command.
If no tag is specified, all tags from that repository will be
pulled.

This behavior of run seems unintuitive since it will only run the latest image and the rest are not needed.

This patch only modifies run. I felt like it might be preferable for pull to mimic docker pull, and only pull all tags if the user explicitly asks for that with a keyword arg like all=True. However, the current behavior of pull is documented, so that would be a major breaking change.

I also considered whether ContainerCollection.create should append the latest tag if there was none, but its current behavior is also documented. Hence, only updating run seems like the safest approach.

Signed-off-by: Dan Hipschman <dan.hipschman@opendoor.com>
@aiordache
Copy link
Collaborator

Thank you for this PR but I think a better way to fix this is in #2671 as this issue is a side effect of missing a default for tag in the pull method.
If you don't mind I will close this in favor of what I proposed.

@aiordache aiordache closed this Sep 15, 2020
@dan-hipschman
Copy link
Author

Thank you for this PR but I think a better way to fix this is in #2671 as this issue is a side effect of missing a default for tag in the pull method.
If you don't mind I will close this in favor of what I proposed.

I prefer your approach of making the default of pull() to pull only the latest tag. The only reason I didn't do that is that the "pull all" behavior is documented, and so I thought it might break some existing code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants