-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Adding my version of a multibrowser node #2822
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
Conversation
CLA assistant check
All committers have signed the CLA.
PR Reviewer Guide 🔍
Here are some key observations to aid the review process:
🎫 Ticket compliance analysis ✅
2795 - PR Code Verified
Compliant requirements:
- Implement a "magic node" with all popular browsers in a single container
- Support both AMD64 and ARM64 architectures
- Include Chrome, Firefox, and Edge browsers in a single container
- Maintain the same functionality as current Node/Standalone containers
Requires further human verification:
- Keep the Dockerfile, build and deploy process simple and maintainable
- Reuse current tests for this new image in CI
Duplicate Program Names
All three cleanup configuration files (chrome, edge, firefox) use the same program name "browserleftoverscleanup" in supervisord config, which could cause conflicts when all are loaded.
[program:browserleftoverscleanup] priority=20 command=bash -c "if [ ${SE_ENABLE_BROWSER_LEFTOVERS_CLEANUP} = "true" ]; then /opt/bin/chrome-cleanup.sh; fi" autostart=%(ENV_SE_ENABLE_BROWSER_LEFTOVERS_CLEANUP)s autorestart=%(ENV_SE_ENABLE_BROWSER_LEFTOVERS_CLEANUP)s stopsignal=INT
Duplicate File
This appears to be an exact duplicate of the wrap_chrome_binary file and should be removed to avoid confusion and maintenance issues.
#!/bin/bash WRAPPER_PATH=$(readlink -f /usr/bin/google-chrome) BASE_PATH="$WRAPPER_PATH-base" mv "$WRAPPER_PATH" "$BASE_PATH" cat >"$WRAPPER_PATH" <<_EOF #!/bin/bash # umask 002 ensures default permissions of files are 664 (rw-rw-r--) and directories are 775 (rwxrwxr-x). umask 002 # Debian/Ubuntu seems to not respect --lang, it instead needs to be a LANGUAGE environment var # See: https://stackoverflow.com/a/41893197/359999 for var in "\$@"; do if [[ \$var == --lang=* ]]; then LANGUAGE=\${var//--lang=} fi done # Set language environment variable export LANGUAGE="\$LANGUAGE" # Capture the filtered environment variables start with "SE_BROWSER_ARGS_" into an array mapfile -t BROWSER_ARGS_ARRAY < <(printenv | grep ^SE_BROWSER_ARGS_) # Iterate over the array for var in "\${BROWSER_ARGS_ARRAY[@]}"; do # Split the variable into name and value IFS='=' read -r name value <<< "\$var" SE_BROWSER_ARGS="\$SE_BROWSER_ARGS \$value" done # Note: exec -a below is a bashism. exec -a "\$0" "$BASE_PATH" --no-sandbox \$SE_BROWSER_ARGS "\$@" _EOF chmod +x "$WRAPPER_PATH"
Browser Configuration Overwrite
The browser configuration at the end of the Dockerfile overwrites previous browser entries since it uses the same files repeatedly, potentially leaving only Firefox configuration accessible.
RUN echo "chrome" > /opt/selenium/browser_name RUN google-chrome --version | awk '{print $3}' | cut -d'.' -f1,2 >> /opt/selenium/browser_version RUN echo "\"goog:chromeOptions\": {\"binary\": \"/usr/bin/google-chrome\"}" >> /opt/selenium/browser_binary_location RUN echo "MicrosoftEdge" > /opt/selenium/browser_name RUN microsoft-edge --version | awk '{print $3}' | cut -d'.' -f1,2 >> /opt/selenium/browser_version RUN echo "\"ms:edgeOptions\": {\"binary\": \"/usr/bin/microsoft-edge\"}" >> /opt/selenium/browser_binary_location RUN echo "firefox" > /opt/selenium/browser_name RUN firefox --version | awk '{print $3}' >> /opt/selenium/browser_version RUN echo "\"moz:firefoxOptions\": {\"binary\": \"/usr/bin/firefox\"}" >> /opt/selenium/browser_binary_location
PR Code Suggestions ✨Explore these optional code suggestions:
|
Hi, thank you for your work and contribution to this area. However, I am still thinking about the approach to build multiple layers to reduce the duplicated code (cloned Dockerfile and scripts). So, I will keep your PR here for a while before making a decision to use this or not.
Absolutely! Feel free to do whatever suits the project best.
It might be a good idea to import only parts from the other builds if at all possible..
Uh oh!
There was an error while loading. Please reload this page.
User description
Description
This is how we go about building images with more than one browser.
Essentially it is just a combination of the different dockerfiles for the separate nodes.
I don't know much about how the tests is setup here, and could also not find a good place for documenting the new node-type. So I leave that to someone that is more involved in the "Selenium way" of doing stuff.
Motivation and Context
This PR is supposed to solve #2795 feature request. Basically this is one way of building a node, that holds more than one type of browser. The total size image is much smaller than building three separate browser nodes.
Types of changes
Checklist
PR Type
Enhancement
Description
Introduce multi-browser Selenium node Dockerfile
Add browser cleanup scripts and supervisor configs
Implement browser binary wrappers for Chrome and Edge
Provide utility scripts for Firefox installation and language packs
Changes walkthrough 📝
9 files
Add Dockerfile for multi-browser Selenium node image
Add Chrome process and temp file cleanup script
Add Edge process and temp file cleanup script
Add Firefox process cleanup script
Add script to download Firefox language packs
Add script to set up Firefox APT repository
Add script for Firefox package installation logic
Add Chrome binary wrapper for argument handling
Add Edge binary wrapper for argument handling
3 files
Add supervisor config for Chrome cleanup daemon
Add supervisor config for Edge cleanup daemon
Add supervisor config for Firefox cleanup daemon
1 files
Duplicate Chrome binary wrapper script (possible artifact)