Skip to content

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

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

parholmdahl
Copy link

@parholmdahl parholmdahl commented May 5, 2025

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

  • Bug fix (non-breaking change which fixes an issue)
  • 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
Dockerfile
Add Dockerfile for multi-browser Selenium node image         
+287/-0 
chrome-cleanup.sh
Add Chrome process and temp file cleanup script                   
+36/-0   
edge-cleanup.sh
Add Edge process and temp file cleanup script                       
+36/-0   
firefox-cleanup.sh
Add Firefox process cleanup script                                             
+24/-0   
get_lang_package.sh
Add script to download Firefox language packs                       
+41/-0   
install-firefox-apt.sh
Add script to set up Firefox APT repository                           
+17/-0   
install-firefox-package.sh
Add script for Firefox package installation logic               
+47/-0   
wrap_chrome_binary
Add Chrome binary wrapper for argument handling                   
+36/-0   
wrap_edge_binary
Add Edge binary wrapper for argument handling                       
+36/-0   
Configuration changes
3 files
chrome-cleanup.conf
Add supervisor config for Chrome cleanup daemon                   
+21/-0   
edge-cleanup.conf
Add supervisor config for Edge cleanup daemon                       
+21/-0   
firefox-cleanup.conf
Add supervisor config for Firefox cleanup daemon                 
+21/-0   
Miscellaneous
1 files
wrap_chrome_binary copy
Duplicate 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.
  • @CLAassistant
    Copy link

    CLAassistant commented May 5, 2025

    CLA assistant check
    All committers have signed the CLA.

    Copy link

    qodo-merge-pro bot commented May 5, 2025

    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

    Copy link

    qodo-merge-pro bot commented May 5, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    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
    • Update

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants