Fix: Make sure default image can be found #698
Conversation
bimtauer
requested review from
katrogan,
kumare3 and
wild-endeavor
as code owners
flytekit/core/context_manager.py
Outdated
| @@ -92,7 +92,7 @@ def find_image(self, name) -> Optional[Image]: | |||
| """ | |||
| Return an image, by name, if it exists. | |||
| """ | |||
| for i in self.images: | |||
| for i in self.images + [self.default_image]: | |||
There was a problem hiding this comment.
Thank you so much. Would you mind adding a simple unit test so that the behavior does not retrogress?
There was a problem hiding this comment.
Added a test and also another line to handle the case of images being None. It seems unnecessary since pretty much everywhere else you initialize ImageConfig with either images=[] or images=[default_image] (also a solution to my problem but maybe more hacky?) but I still thought it might be a good idea since the type hints declare it as Optional.
Codecov Report
@@ Coverage Diff @@
## master #698 +/- ##
=======================================
Coverage 85.70% 85.71%
=======================================
Files 355 356 +1
Lines 29703 29715 +12
Branches 2426 2426
=======================================
+ Hits 25457 25469 +12
Misses 3605 3605
Partials 641 641
Continue to review full report at Codecov.
|
Signed-off-by: Tim Bauer <[email protected]>
Signed-off-by: Tim Bauer <[email protected]>
Signed-off-by: Tim Bauer <[email protected]>
bimtauer
force-pushed
the
fix/include-default-img-in-find-images
branch
from
9aaf53b to
2736440
Compare
Signed-off-by: Tim Bauer <[email protected]>
bimtauer
force-pushed
the
fix/include-default-img-in-find-images
branch
from
cc6955d to
6861cf6
Compare
|
@bimtauer some tests are broken, can you please fix them. we can then merge this PR |
|
Congrats on merging your first pull request! 🎉 |
|
Thanks for restarting the github actions and merging! |
Signed-off-by: Tim Bauer <[email protected]> Signed-off-by: Lisa <[email protected]>
TL;DR
Image name interpolation from
defaultimage currently fails bcfind_imagedoes not compare to to the default_image of theImageConfig.This is confusing bc the docs explicitly mention interpolation from the
defaultimage as an example.Currently when trying to register workflows with multiple images, they fail with
Type
Are all requirements met?
Complete description
The fix is very simple and adds the
default_imageto the list of images searched by thefind_imagemethod.Tracking Issue
NA
Follow-up issue
NA