Conversation
vsbogd
left a comment
There was a problem hiding this comment.
This PR is aligned with the current Omega design.
There are some changes which would better stay in the plugin's code:
Dockerfileandrequirements.txtin order to install plugin's dependenciesentrypoint.shin order to promote environment variable changes
But this has to be changed in a more general task to make plugins more modular.
| # Scrub environment: only allowlisted vars survive. | ||
| SAFE_VARS="HOME USER PATH HOSTNAME TERM LANG LC_ALL \ | ||
| PYTHONDONTWRITEBYTECODE PYTHONUNBUFFERED \ | ||
| PLAYWRIGHT_BROWSERS_PATH \ |
There was a problem hiding this comment.
@jazzbox35 , am I right that this line is needed to promote the environment variable in a case when user uses custom PLAYWRIGHT_BROWSERS_PATH value, for example he mounts the host's directory into the Docker?
There was a problem hiding this comment.
@vsbogd As far as I can tell from AI and their documentation, I think that environment variable is needed to mount an external browser. It looks like that env variable is used in installation and also in execution. I got a crash initially when I was missing it.
Regarding your comments above about Dockerfile, requirements.txt, and entrypoint.sh: I think you mean that ideally these would be inside the plugin's code? However we can't do that yet?
Thanks!
There was a problem hiding this comment.
Also noteworthy: I had to bump up some of the space allocations in the docker run command. When I tried to do screen print, it apparently was running out of RAM and storage. The extended size seemed to work.
In general adding a browser adds some bloat.
There was a problem hiding this comment.
Regarding your comments above about Dockerfile, requirements.txt, and entrypoint.sh: I think you mean that ideally these would be inside the plugin's code? However we can't do that yet?
Yes, my opinion we should design it properly first and then implement it for all of the plugins. May be we need to mark all such places by some TODO: move into playwright plugin but I am not sure.
There was a problem hiding this comment.
I mean marking it clearly and uniformly could help to find all these places in the future.
Description
Major changes planned:
Create new "playwright" modules inside the /plugins directory for new browser skills.
Include the Chromium browser in the docker build.
How Has This Been Tested?
This is still under development and being tested. This is for early discussion only before ready for review.
There is a lot of Codex code yet to be reviewed.
Checklist