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

Added useful error message when image not found #6056

Closed
wants to merge 8 commits into from

Conversation

honeyankit
Copy link
Contributor

@honeyankit honeyankit commented Nov 5, 2022

Context

When an image is not found in the private registry, it throws an exception of DockerRegistry2::NotFound and lot of stack trace but this info is not useful for the end user.

updater | ERROR <job_500122728> Error processing myreg/ubuntu (DockerRegistry2::NotFound)
updater | ERROR <job_500122728> 404 Not Found
updater | ERROR <job_500122728> /home/dependabot/dependabot-updater/vendor/ruby/3.1.0/gems/docker_registry2-1.12.0/lib/registry/registry.rb:285:in `rescue in doreq'
updater | ERROR <job_500122728> /home/dependabot/dependabot-updater/vendor/ruby/3.1.0/gems/docker_registry2-1.12.0/lib/registry/registry.rb:269:in `doreq'
updater | ERROR <job_500122728> /home/dependabot/dependabot-updater/vendor/ruby/3.1.0/gems/docker_registry2-1.12.0/lib/registry/registry.rb:26:in `doget'

Error logs: https://github.com/dsp-testing/docker-firewalled-testing/network/updates/500122728

Approach

I'm using a rescue block to catch the "DockerRegistry2::NotFound" exception and throwing it with useful error message when a Dependabot tries to retrieve an image from the registry but it doesn't exist there.

@honeyankit honeyankit self-assigned this Nov 5, 2022
@honeyankit honeyankit requested a review from a team as a code owner November 5, 2022 04:24
@honeyankit honeyankit changed the title Added useful error message when image not found. Added useful error message when image not found Nov 5, 2022
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Thanks for improving error messages! I just added two minor comments.

docker/lib/dependabot/docker/update_checker.rb Outdated Show resolved Hide resolved
docker/lib/dependabot/docker/update_checker.rb Outdated Show resolved Hide resolved
@jeffwidman
Copy link
Member

IMO let's hold off merging this for a couple of days to see if we can land a fix in upstream, as it should be straightforward and that'd be less work than merging this then backing it out when upstream lands a fix. I pinged the maintainer on @honeyankit 's issue, hopefully we get a quick response.

@jeffwidman
Copy link
Member

Pending fix upstream: deitch/docker_registry2#68

@deivid-rodriguez
Copy link
Contributor

For what it's worth, I'm looking into actually deleting all of this code at #6150 since I believe it's problematic.

@jeffwidman
Copy link
Member

IMO we should not merge this because even if if the underlying code doesn't get deleted in #6150, the upstream lib will still get patched in deitch/docker_registry2#68.

So not worth the trouble to add it here, then pulling it back out once upstream fix arrives...

@honeyankit
Copy link
Contributor Author

This PR is not relevant as the fix is coming from upstream: deitch/docker_registry2#68.
So closing this PR.

@honeyankit honeyankit closed this Dec 13, 2022
@jeffwidman jeffwidman deleted the honeyankit/fix-docker-error-message branch March 20, 2023 23:32
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.

4 participants