Fix #22240 do not pull all the tags implicitely#22247
Conversation
86bf4ae to
1b18d24
Compare
1b18d24 to
dec8e54
Compare
|
Updated and build fixed (only ping @tonistiigi @stevvooe |
dec8e54 to
c96c9b4
Compare
There was a problem hiding this comment.
This should not be called IsTrusted. This field has no bearing on the trust state of an image. Not sure if we need a PR in engine-api to handle this.
There was a problem hiding this comment.
Hum, it's already in engine-api, and I guess it's for golint, is the attribute is named IsTrusted 👼
There was a problem hiding this comment.
This needs to be fixed. These images are not trusted; they are official.
There was a problem hiding this comment.
This is the comment for the IsTrusted flag, not IsOfficial. The comment referenced the incorrect variable.
There was a problem hiding this comment.
IIRC IsTrusted was only there for backward compatibility, as it was the old name for official images on Docker Hub; can we officially deprecate and remove it?
There was a problem hiding this comment.
@aaronlehmann This was IsTrusted, which changed to IsOfficial in the code, but had to stay is_trusted on the wire. I am not sure about the deprecation status, but let's do so.
There was a problem hiding this comment.
Wow. Then it definitely is misleading. I thought this referred to content trust. You're right that it needs to be changed.
There was a problem hiding this comment.
Actually, I'm wrong: https://docs.docker.com/engine/reference/api/docker_remote_api_v1.19/.
"This API returns both is_trusted and is_automated images. Currently, they are considered identical. In the future, the is_trusted property will be deprecated and replaced by the is_automated property."
Seems to have been deprecated between 1.19 and 1.20. I propose we remove this from engine-api.
|
We should add a test for it so we don't break it in the future. |
|
@tonistiigi yes, I'll work on that next few days 👼 |
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
|
|
c96c9b4 to
0134fd8
Compare
|
@tonistiigi Added a regression test ; it fails on master, and succeed locally. |
|
LGTM, thanks! ping @aaronlehmann @tonistiigi PTAL |
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
0134fd8 to
54ebe42
Compare
|
and ping @cpuguy83 as this is blocking your PR 😇 |
|
|
||
| // ImagePullOptions holds information to pull images. | ||
| type ImagePullOptions struct { | ||
| All bool |
There was a problem hiding this comment.
This would benefit from an explanatory comment.
There was a problem hiding this comment.
@aaronlehmann true. I'll add a comment on this next PR I do on engine-api 😉. It should get in docker/docker before next release but we shouldn't block this PR for it 👼
|
LGTM |
1 similar comment
|
LGTM |
|
LGTM |
This fixes #22240 (
docker run -it busybox /bin/shwill, on master, pull any tag ifbusybox:latestwas not present)in a weird and quick way. I would really prefer the fix in docker-archive-public/docker.engine-api#205 (in one of the option)— it is now cleaner and uses engine-api.Depends on docker-archive-public/docker.engine-api#205 👼
/cc @stevvooe @calavera @tonistiigi
Signed-off-by: Vincent Demeester vincent@sbr.pm