-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
c8d/pull: Return same access denied error as graphdrivers #46656
Conversation
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I hate it 😂
LGTM
@@ -120,6 +123,9 @@ func (i *ImageService) PullImage(ctx context.Context, ref reference.Named, platf | |||
|
|||
img, err := i.client.Pull(ctx, ref.String(), opts...) | |||
if err != nil { | |||
if errors.Is(err, docker.ErrInvalidAuthorization) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will clash with containerd/containerd#9234
maybe we should wrap the returned error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doh! forgot about that one; I guess we need to revert this one when we bump to a containerd vendor with that patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the check will be even uglier, we'll have to check the error message itself and return the right error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we still be able to match the ErrInvalidAuthorization
that it wraps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened: #46662
- What I did
- How I did it
- How to verify it
Fixes
ContainerCollectionTest.test_run_with_image_that_does_not_exist
from docker-pyPartially fixes TestImagePullNonExisting
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)