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

Set image default tag on pull #2671

Merged
merged 2 commits into from Oct 16, 2020
Merged

Conversation

aiordache
Copy link
Collaborator

Set default tag to latest on pull.
Resolves #2545

@thaJeztah
Copy link
Member

Looks like a duplicate of #2656

@thaJeztah
Copy link
Member

With this change, will it still be possible to achieve the "pull all tags" behavior? (i.e. to implement "docker pull --all-tags ..." ?

@aiordache
Copy link
Collaborator Author

With this change, will it still be possible to achieve the "pull all tags" behavior? (i.e. to implement "docker pull --all-tags ..." ?

Oops, don't think so. Let me try something else.

@aiordache aiordache force-pushed the default_tag branch 2 times, most recently from a42be80 to e89a080 Compare September 15, 2020 17:54
@@ -395,7 +395,7 @@ def load(self, data):

return [self.get(i) for i in images]

def pull(self, repository, tag=None, **kwargs):
def pull(self, repository, tag='latest', **kwargs):
"""
Pull an image of the given name and return it. Similar to the
``docker pull`` command.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording of the docstring should be updated to make it more clear. E.g.:

If no tag is specified, all tags from that repository will be pulled.

should be

If tag is None, all tags from that repository will be pulled.

@aiordache aiordache force-pushed the default_tag branch 2 times, most recently from b67e580 to 1d09f93 Compare September 15, 2020 18:30
@@ -342,14 +342,14 @@ def prune_images(self, filters=None):
params['filters'] = utils.convert_filters(filters)
return self._result(self._post(url, params=params), True)

def pull(self, repository, tag=None, stream=False, auth_config=None,
def pull(self, repository, tag='latest', stream=False, auth_config=None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking out loud: would it work to add an all_tags parameter, and based on that either keep None as None, or set None -> latest ?

Train of thought is that if you want to pull all tags, no tag is allowed to be set, so it feels a bit more natural to having a separate option (the original "no tag means all tags" is a bit awkward, and, well, bad choices in the past)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thaJeztah I added an all_tags parameter to the pull methods. Is the logic ok? If we set it, we ignore any tag set with tag or repository parameter.

Signed-off-by: aiordache <anca.iordache@docker.com>

Returns:
(:py:class:`Image` or list): The image that has been pulled.
If no ``tag`` was specified, the method will return a list
If ``tag`` is None, the method will return a list

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment needs updating. Should it be something like this?

The image that has been pulled. If all_tags is True, then the method will return a list of :py:class:Image objects belonging to this repository.

If no tag is specified, all tags from that repository will be
pulled.
If ``all_tags`` is set, the ``tag`` parameter is ignored and all image
tags will be pulled.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though it's intuitive, it's probably good to add explicit documentation that if tag is None or empty, then it will be set to "latest". Also in the api.image module.

Signed-off-by: aiordache <anca.iordache@docker.com>
Copy link
Collaborator

@ulyssessouza ulyssessouza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ulyssessouza
Copy link
Collaborator

@thaJeztah PTAL

@aiordache aiordache merged commit daa9f17 into docker:master Oct 16, 2020
@aiordache aiordache linked an issue Nov 17, 2020 that may be closed by this pull request
@aiordache aiordache added this to the 4.4.0 milestone Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants