Skip to content
This repository was archived by the owner on Jul 18, 2025. It is now read-only.

Update GetTagFromNamedRef to return latest…#205

Merged
LK4D4 merged 2 commits into
docker-archive-public:masterfrom
vdemeester:reference-add-latest
Apr 26, 2016
Merged

Update GetTagFromNamedRef to return latest…#205
LK4D4 merged 2 commits into
docker-archive-public:masterfrom
vdemeester:reference-add-latest

Conversation

@vdemeester

@vdemeester vdemeester commented Apr 17, 2016

Copy link
Copy Markdown
Contributor

… in case of no tag in the reference 🐰.

Right now, if you pass a reference without a tag will send a request without tag to the daemon and thus the daemon will download all tags for this repository. docker/docker adds the default latest tag if it has not been and if --all is not defined.

I'm not sure this is the right place for it, waiting for your input. This change would break the way --all works, and the 2nd commits adds a more explicit way to specify we want to download all tags (putting it in option). If we decide to go this route, I'll do the PR in docker/docker to update the server side API.

/cc @stevvooe @calavera @MHBauer

This also adds some unit test for this package 🐙.

🐸

Signed-off-by: Vincent Demeester vincent@sbr.pm

testCases := []struct {
ref string
expectedName string
expectedTag string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure we should reinforce the concept of a tag being a digest. Maybe call this expectedTagOrDigest.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah yes 😉

@stevvooe

Copy link
Copy Markdown
Contributor

LGTM

This may be more subtle in the engine, but I think this works.

@vdemeester

Copy link
Copy Markdown
Contributor Author

@stevvooe It would also be possible to not change anything in the engine, just handle the all attribute in engine-api and append or not the default tag — but I feel it makes it clearer by passing all to engine (although it changes the API and might be tricky to handle daemon wise for backward compatibilites..).

@stevvooe

Copy link
Copy Markdown
Contributor

@vdemeester I agree.

The engine-api needs to focus on user intent, rather than expose the subtleties of the docker api.

@HackToday

Copy link
Copy Markdown

So it means change API, and allow to pass all=1 for createimages ? Is that ?

@vdemeester

Copy link
Copy Markdown
Contributor Author

@HackToday there is two solution, one with an API change, one without. I think we'll split it into two, one that fixes the behaviour without the API change, and then proposing the API change.

… in case of no tag in the reference. This also adds some unit test for
this package.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@vdemeester

vdemeester commented Apr 24, 2016

Copy link
Copy Markdown
Contributor Author

Updated to require no Server API changes for now 👼
This is needed to fix moby/moby#22247

This make the `--all` option more explicit, API wise.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@vdemeester

vdemeester commented Apr 26, 2016

Copy link
Copy Markdown
Contributor Author

Updated by also modifying push behavior, was causing trouble to add tag latest automatically… 😅.

/cc @stevvooe @tonistiigi

@calavera

Copy link
Copy Markdown
Contributor

LGTM

1 similar comment
@LK4D4

LK4D4 commented Apr 26, 2016

Copy link
Copy Markdown
Contributor

LGTM

@LK4D4 LK4D4 merged commit b7e5e1e into docker-archive-public:master Apr 26, 2016
@vdemeester vdemeester deleted the reference-add-latest branch April 26, 2016 16:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants