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 https://github.com/SeleniumHQ/docker-selenium/issues/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
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
Checklist
- [ x ] I have read the contributing document.
- [ x ] My change requires a change to the documentation.
- [ - ] I have updated the documentation accordingly.
- [ - ] I have added tests to cover my changes.
- [ - ] All new and existing tests passed.
PR Type
Enhancement
Description
-
Introduce multi-browser Selenium node Dockerfile
- Installs Chrome, Edge, and Firefox in one container
- Adds browser-specific WebDrivers and language packs
- Supports both AMD64 and ARM64 architectures
-
Add browser cleanup scripts and supervisor configs
- Automated cleanup for Chrome, Edge, and Firefox processes/files
-
Implement browser binary wrappers for Chrome and Edge
- Handles language and argument environment variables
-
Provide utility scripts for Firefox installation and language packs
Changes walkthrough 📝
| Relevant files |
|---|
| Enhancement | 9 files
DockerfileAdd Dockerfile for multi-browser Selenium node image |
+287/-0 |
chrome-cleanup.shAdd Chrome process and temp file cleanup script |
+36/-0 |
edge-cleanup.shAdd Edge process and temp file cleanup script |
+36/-0 |
firefox-cleanup.shAdd Firefox process cleanup script |
+24/-0 |
get_lang_package.shAdd script to download Firefox language packs |
+41/-0 |
install-firefox-apt.shAdd script to set up Firefox APT repository |
+17/-0 |
install-firefox-package.shAdd script for Firefox package installation logic |
+47/-0 |
wrap_chrome_binaryAdd Chrome binary wrapper for argument handling |
+36/-0 |
wrap_edge_binaryAdd Edge binary wrapper for argument handling |
+36/-0 |
|
| Configuration changes | 3 files
chrome-cleanup.confAdd supervisor config for Chrome cleanup daemon |
+21/-0 |
edge-cleanup.confAdd supervisor config for Edge cleanup daemon |
+21/-0 |
firefox-cleanup.confAdd supervisor config for Firefox cleanup daemon |
+21/-0 |
|
| Miscellaneous | 1 files
wrap_chrome_binary copyDuplicate Chrome binary wrapper script (possible artifact) |
+36/-0 |
|
Need help?
Type /help how to ... in the comments thread for any questions about Qodo Merge usage.Check out the documentation for more information.
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
|
| ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪ |
| 🧪 No relevant tests |
| 🔒 No security concerns identified |
⚡ Recommended focus areas for review
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:
| Category | Suggestion | Impact |
| Possible issue |
✅ Use unique program names
Suggestion Impact:The commit renamed the program from 'browserleftoverscleanup' to 'chromeleftoverscleanup' to make it unique, and also updated the log file names to be browser-specific. This addresses the core issue identified in the suggestion.
code diff:
-[program:browserleftoverscleanup]
+[program:chromeleftoverscleanup]
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
@@ -11,8 +11,8 @@
;Logs
redirect_stderr=false
-stdout_logfile=/var/log/supervisor/browser-leftover-cleanup-stdout.log
-stderr_logfile=/var/log/supervisor/browser-leftover-cleanup-stderr.log
+stdout_logfile=/var/log/supervisor/chrome-leftover-cleanup-stdout.log
+stderr_logfile=/var/log/supervisor/chrome-leftover-cleanup-stderr.log
The program name should be unique across all supervisord configuration files.
Since there are three identical program names (browserleftoverscleanup) in
different conf files, this will cause conflicts when supervisord loads them.
NodeMultibrowser/chrome-cleanup.conf [5-10]
-[program:browserleftoverscleanup]
+[program:chrome_leftoverscleanup]
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
[Suggestion has been applied]
Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies that using the same program name browserleftoverscleanup in multiple supervisord configuration files (chrome-cleanup.conf, edge-cleanup.conf, firefox-cleanup.conf) will cause conflicts, likely preventing some cleanup tasks from running. This is a significant correctness issue.
| High
|
|
| |
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..
docker-selenium copied to clipboard
SeleniumHQ
![qodo-code-review[bot] avatar](https://avatars.githubusercontent.com/in/484649?v=4 "Published by a pub.dev verified publisher"){kind=small}
