Auto install pnpm for UI e2e tests in breeze command#58845
Auto install pnpm for UI e2e tests in breeze command#58845potiuk merged 11 commits intoapache:mainfrom
Conversation
)" This reverts commit 416c73e.
vatsrahul1001
requested review from
amoghrajesh,
ashb,
bugraoz93,
ephraimbuddy,
gopidesupavan,
jason810496,
jedcunningham,
jscheffl,
kaxil and
potiuk
as code owners
boring-cyborg
bot
added
area:dev-tools
backport-to-v3-1-test
vatsrahul1001
removed
the
backport-to-v3-1-test
There was a problem hiding this comment.
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
4 participants
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.