-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Auto install pnpm for UI e2e tests in breeze command #58845
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
Auto install pnpm for UI e2e tests in breeze command #58845
Conversation
)" This reverts commit 416c73e.
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.
Looks good! Thanks Rahul!
I am not against having this in breeze as check and run it but it would be a lot stable and easier to maintain if we install this before even we get here. What this check can do is exit and warn us or anyone checking the logs.
result = run_command(
["npm", "install", "-g", "pnpm"],
no_output_dump_on_exception=True,
capture_output=True,
text=True,
check=False,
)
We can install pnpm around here maybe where we already installing additional deps on OS level in docker. Then install_pnpm.sh added to dockerfile which with bash we chan easily do the check if it is installed. If it is in CI image or Prod according to use case, it will be there already in the image and this can fail if we have something wrong with the installation where similar warning as one above method which says install or check script
https://github.com/apache/airflow/tree/main/scripts/docker
Open to discussion, just wanted to share my take on it
|
If we only want to install for this specific case maybe we can maintain installation here which could be better in the contex of |
* Revert "Fix memory leak in remote logging connection cache (apache#56695)" This reverts commit 416c73e. * enable e2e ui test to install pnpm if not installed
* Revert "Fix memory leak in remote logging connection cache (apache#56695)" This reverts commit 416c73e. * enable e2e ui test to install pnpm if not installed
* Revert "Fix memory leak in remote logging connection cache (apache#56695)" This reverts commit 416c73e. * enable e2e ui test to install pnpm if not installed
As highlighted by @potiuk, we should install pnpm if not already installed this PR addresses that.
^ 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.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.