Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Only pull image on container create if the error matches #5743

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LaurentGoderre
Copy link
Member

@LaurentGoderre LaurentGoderre commented Jan 14, 2025

- What I did

Validate that the image not found error matches the container definition before pulling it. This supports moby/moby#48798

- How I did it

Check the error message from the server before pulling.

- How to verify it

Unit tests are updated to demonstrate it.

- Description for the changelog

Validate that the image not found error matches the container definition before pulling it

- A picture of a cute animal (not mandatory but encouraged)

@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.66%. Comparing base (b462778) to head (6800e80).
Report is 11 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5743      +/-   ##
==========================================
+ Coverage   59.51%   59.66%   +0.15%     
==========================================
  Files         346      340       -6     
  Lines       29376    29127     -249     
==========================================
- Hits        17483    17379     -104     
+ Misses      10923    10784     -139     
+ Partials      970      964       -6     

@LaurentGoderre LaurentGoderre force-pushed the missing-image-validation branch 4 times, most recently from a887cd2 to 20dd3e5 Compare January 14, 2025 15:19
Signed-off-by: Laurent Goderre <laurent.goderre@docker.com>
@LaurentGoderre LaurentGoderre force-pushed the missing-image-validation branch from 20dd3e5 to 6800e80 Compare January 14, 2025 15:29
@thaJeztah
Copy link
Member

Hm... honestly, not sure if we should go down this road again, because string-matching is really brittle (we used to have this, and changing an error message between releases resulted in things failing). e.g. linters will complain that No such: should not be capitalised, and changing it to lower case (no such image) will break logic.

I think ideally, we'd look at some other options, which may be additional, structured, information in the error-response, or (something discussed in the past) even consider moving the pulling to the daemon side (although there will still be some work to do if the volume requires different authentication than the image that's being run)

@LaurentGoderre
Copy link
Member Author

@thaJeztah I was debating changing the error message from the daemon to include a raw JSON representation of the error which would be a lot less brittle but wasn't sure if it was too big of a change

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants