-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
add guideline for missing image when running breeze #35813
Conversation
breeze ci-image pull or breeze ci-image build |
Actually this is not the right solution. The problem is that image should be automatically build if it is missing, there should never be the need to pull it manually. Likely this might be a side effect of some change where not having an image triggers this problem. It might be result of some recent refactors I've done, but user should never need to pull the image of Breeze manually, user should not even have to know the image name, it's merely an implementation detaill Was there any special you've done (cleaning docker etc.? @Lee-W - can you come up withe a scenario where this could happen ? I might try to reproduce it, but if you more ore less know how you got there, it could help to fix it. |
Yes, |
Ok. I think (maybe you can check) just adding check_and_rebuild_image_if_needed() when the start command run should solve the problem. It I lis for sure done in shell command and default one. Getting to plane now but should be easy to find and fix :) somewhere in 'developer_commands' |
Sounds good. Let me check where i can add it |
I found the problem and fixed it as part of #35830 The problem is that the check only checked if the image should be upgraded (not whether it was pulled in the first place). I have also found another problem that prevented the image to be automatically build correctly (version suffix was not set to dev0 and there were conflicting requirements). Closing in favour or my PR :). Thanks for raising the problem :D |
Wow, it's a huge PR. thanks for helping out |
Actually, that fix was separated out to a slightly smaller one #35862 I could split it out even further if reviewers think it makes sense (I thin that would inflate the number of PRs artifficiall but if others think the PR is still too big to review, I am happy to split it out. |
The part that fixes the problem you had is here:
But it's done together with renaming of e "image_check" property in the object - that's why it's easier to keep it together with the rename. |
Yep, I'm saying that just to praise the good work, not to request to split the PR. It just might take sometime for me to take a deeper look |
I am splitting it anyway to make it easier to review in small chunks. The current one is #35875 |
I just got the following error...
Then I ran Now I'm getting some yarn error :( |
@RNHTTR -> Post on slack the whole output of what you have done. (and whether it was fresh install). Generally speaking you should always: a) rebase to latest main first I recommend to follow a) and b) - and make sure to read what breeze advice you get. If you follow it and still got error - please post me the output of the command you run after adding Unfirtunately seeing just a |
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.