-
Notifications
You must be signed in to change notification settings - Fork 16.2k
Add --mount-ui-dist flag for Breeze start-airflow command #59778
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
base: main
Are you sure you want to change the base?
Conversation
amoghrajesh
left a comment
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.
Nice!
|
Very cool idea! But it can be done way simpler I think. You can likely directly mount the |
| console.print("[bright_blue]UI assets compiled successfully") | ||
|
|
||
|
|
||
| def copy_mounted_ui_dist(mounted_ui_dist_source: Path, dist_prefix: str, host_source_prefix: str): |
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.
Hmm. I thinl rather than doing this copy, we could simply mount the folder directly where it is expected to be? I don't think we need to use the temporary folder to copy it from there inside?
Or am I wrong :) ?
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.
Also this could be enhanced with edge3 and fab provider assets done the same way after #56456 is merged and fab assets are also dynamically generated.
gopidesupavan
left a comment
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.
nice addition :)
jason810496
left a comment
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.
Hmm. I thinl rather than doing this copy, we could simply mount the folder directly where it is expected to be? I don't think we need to use the temporary folder to copy it from there inside?
Yes, very agree with that! It's way more better to mount it directly to library path.
Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
breeze setup regenerate-command-images --force
e296418 to
814b3bf
Compare
jason810496
left a comment
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 all for the review! I just refactored as directly mount to correct path instead of copying from tmp path.
related: #57219
Why
After Auto-compile UI assets on Breeze start-airflow command #57219, we could
breeze start-airflow --use-airflow version <pr-number> / <owner/repo:branch>and access the Frontend successfully. We make it happen by:node.jsnpmpnpmpnpm build/usr/python/lib/python3.10/site-packages/airflow/ui/distHowever, if we just want to test the Airflow Core change from someone's PR (e.g. validate API Server TaskInstance Log reading path, check Scheduler behavior but still want to access Frontend for observability, etc), we still need to do the all the steps above just to access Frontend, which is waste of time for test iteration.
Instead, we could just mount the pre-built UI dist from host to Breeze container to save the time, in most of the time, the core change will not impact API Server <-> Frontend behavior. (e.g. When I testing Fix rendering of template fields with start from trigger#55068 recently, there isn't anything related with Frontend, but I still need to access it to check the TaskInstance status. )
What
Add
--mount-ui-distflag forstart-airflowcommand to mount UI dist directly from host to Breeze container without building the whole frontend again to save time.Verfication
--mount-ui-dist, the middle of it should be navy blue color instead of pure black if we are using3.1.5--mount-ui-distOutput of
breeze start-airflow --backend postgres --mount-sources providers-and-tests --use-airflow-version apache/airflow:main --mount-ui-dist