Skip to content
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

[build][doc]: build images with combination Grid and browser versions #2218

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Apr 21, 2024

User description

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Support local build images with a specific combination of Grid and browser versions and full tag images to pin version following Tagging Conventions

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

  • I have read the contributing document.
  • 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.

Type

enhancement, documentation


Description

  • Added scripts and configurations to build Docker images with specific versions of Selenium Grid and browsers.
  • Enhanced Dockerfiles for Chrome and Edge to handle different version formats and conditional installations.
  • Updated README with detailed instructions for building images with specific versions and upgrading browser versions.
  • Introduced new YAML configurations for managing version matrices for Selenium and browsers.

Changes walkthrough

Relevant files
Enhancement
tag_and_push_browser_images.sh
Add logging for image tagging in shell script                       

tag_and_push_browser_images.sh

  • Added echo statements to log tagging of browser images for chrome,
    edge, and firefox.
  • +6/-0     
    bootstrap.sh
    Implement script for building browser-specific Docker images

    tests/build-backward-compatible/bootstrap.sh

  • Created a new script to setup environment and build images with
    specific browser versions.
  • Handles different browsers and versions based on command line
    arguments.
  • Supports optional image pushing to a Docker registry.
  • +66/-0   
    builder.py
    Add Python script for handling version matrices and .env generation

    tests/build-backward-compatible/builder.py

  • New Python script to merge Selenium and CDP version matrices and
    generate .env file.
  • Handles command line arguments for version selection.
  • +48/-0   
    Dockerfile
    Update Chrome version handling in Dockerfile                         

    NodeChrome/Dockerfile

  • Modified regex and added transformation for CHROME_VERSION to handle
    different version formats.
  • +3/-2     
    Dockerfile
    Enhance Edge version parsing and installation in Dockerfile

    NodeEdge/Dockerfile

  • Updated EDGE_VERSION handling with regex and conditional logic for
    version format.
  • +10/-1   
    Documentation
    README.md
    Update README with new build instructions and version upgrades

    README.md

  • Updated documentation to include instructions for building images with
    specific versions.
  • Added sections on building images with specific browser versions and
    upgrading browser versions.
  • +29/-0   
    Configuration changes
    cdp-matrix.yml
    Define browser version matrix in new YAML file                     

    tests/build-backward-compatible/cdp-matrix.yml

  • Added a new YAML file defining browser versions for Chrome, Edge, and
    Firefox.
  • +58/-0   
    selenium-matrix.yml
    Introduce Selenium version matrix configuration                   

    tests/build-backward-compatible/selenium-matrix.yml

  • Introduced a new YAML file with Selenium version matrix and related
    CDP version mappings.
  • +139/-0 

    PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Description updated to latest commit (541b14a)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    3, because the PR includes multiple changes across shell scripts, Dockerfiles, and Python scripts, which requires understanding of Docker, shell scripting, and Python environments. Additionally, the changes involve version handling and tagging which need careful review to ensure compatibility and correctness.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The use of grep -qE with regex patterns in Dockerfiles might not correctly handle all edge cases for version strings, potentially leading to incorrect parsing or matching.

    Error Handling: The shell scripts and Python scripts lack comprehensive error handling, which might lead to failures in build processes not being caught or logged appropriately.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Use pip's quiet mode to reduce output verbosity instead of filtering with grep.

    Instead of using grep -v 'Requirement already satisfied' to filter pip install output,
    consider using pip install --quiet to reduce the verbosity of the output, which is more
    reliable and cleaner.

    tests/build-backward-compatible/bootstrap.sh [5]

    -pip3 install virtualenv | grep -v 'Requirement already satisfied'
    +pip3 install --quiet virtualenv
     
    Replace wget with curl for downloading files to enhance versatility and protocol support.

    Consider using curl instead of wget for downloading files. curl is generally preferred due
    to its versatility and extensive protocol support. Replace wget with curl and adjust the
    syntax accordingly.

    NodeChrome/Dockerfile [25]

    -wget -qO google-chrome.deb "https://dl.google.com/linux/chrome/deb/pool/main/g/google-chrome-stable/${CHROME_VERSION}_${TARGETARCH}.deb"
    +curl -sLo google-chrome.deb "https://dl.google.com/linux/chrome/deb/pool/main/g/google-chrome-stable/${CHROME_VERSION}_${TARGETARCH}.deb"
     
    Best practice
    Improve script portability by using the dot command for activating virtual environments.

    Instead of using source to activate the virtual environment, use the . (dot) command for
    better portability across different shells.

    tests/build-backward-compatible/bootstrap.sh [7]

    -source docker-selenium-tests/bin/activate
    +. docker-selenium-tests/bin/activate
     
    Improve error handling by checking the exit status of commands directly.

    Consider checking the exit status of the cd command directly instead of using || true,
    which suppresses all errors unconditionally. This can be done by using set -e at the
    beginning of the script to stop the script on any errors.

    tests/build-backward-compatible/bootstrap.sh [2]

    -cd tests || true
    +set -e
    +cd tests
     
    Use mapfile for better script readability and adherence to conventions.

    Use mapfile instead of readarray for better readability and to adhere to more commonly
    used conventions in bash scripting.

    tests/build-backward-compatible/bootstrap.sh [63]

    -readarray -t LOG_LINES <<< "$TAG_LOG_OUTPUT"
    +mapfile -t LOG_LINES <<< "$TAG_LOG_OUTPUT"
     
    Security
    Enhance security by properly handling command line arguments to avoid injection.

    To avoid potential command injection vulnerabilities, consider using an array to store
    BUILD_ARGS and then expand it properly when calling make.

    tests/build-backward-compatible/bootstrap.sh [27-28]

    -BUILD_ARGS="--build-arg FIREFOX_VERSION=${FIREFOX_VERSION}"
    -BUILD_ARGS="${BUILD_ARGS}" make standalone_firefox
    +BUILD_ARGS=(--build-arg "FIREFOX_VERSION=${FIREFOX_VERSION}")
    +make standalone_firefox "${BUILD_ARGS[@]}"
     
    Maintainability
    Improve the robustness of CHROME_VERSION variable manipulation to handle complex version strings.

    Ensure that the CHROME_VERSION variable transformation handles both underscore and equals
    sign correctly. The current approach might not correctly handle cases where the version
    string is complex or formatted differently. Consider using a more robust pattern matching
    or string manipulation technique.

    NodeChrome/Dockerfile [24]

    -CHROME_VERSION=$(echo "$CHROME_VERSION" | tr '=' '_')
    +CHROME_VERSION=$(echo "$CHROME_VERSION" | sed 's/[=_]/_/g')
     
    Break complex chained commands into multiple RUN instructions to enhance readability and maintainability.

    To improve the readability and maintainability of the Dockerfile, consider breaking
    complex chained commands into multiple RUN instructions, especially when they include
    conditional logic.

    NodeChrome/Dockerfile [22-27]

    -&& if echo "${CHROME_VERSION}" | grep -qE "google-chrome-stable[_|=][0-9]*"; \
    -then \
    -CHROME_VERSION=$(echo "$CHROME_VERSION" | tr '=' '_') \
    -&& wget -qO google-chrome.deb "https://dl.google.com/linux/chrome/deb/pool/main/g/google-chrome-stable/${CHROME_VERSION}_${TARGETARCH}.deb" \
    -&& apt-get -qqy --no-install-recommends install --allow-downgrades ./google-chrome.deb \
    -&& rm -rf google-chrome.deb ; \
    +RUN if echo "${CHROME_VERSION}" | grep -qE "google-chrome-stable[_|=][0-9]*"; then \
    +    CHROME_VERSION=$(echo "$CHROME_VERSION" | tr '=' '_') \
    +    && wget -qO google-chrome.deb "https://dl.google.com/linux/chrome/deb/pool/main/g/google-chrome-stable/${CHROME_VERSION}_${TARGETARCH}.deb" \
    +    && apt-get -qqy --no-install-recommends install --allow-downgrades ./google-chrome.deb \
    +    && rm -rf google-chrome.deb ; \
     else \
     
    Reliability
    Add error handling to the wget command to ensure script reliability in case of download failures.

    Add error handling for the wget command to ensure that the script stops executing if the
    download fails. This can prevent subsequent commands from running which might depend on
    the successful download of the Chrome package.

    NodeChrome/Dockerfile [25]

    -wget -qO google-chrome.deb "https://dl.google.com/linux/chrome/deb/pool/main/g/google-chrome-stable/${CHROME_VERSION}_${TARGETARCH}.deb"
    +wget -qO google-chrome.deb "https://dl.google.com/linux/chrome/deb/pool/main/g/google-chrome-stable/${CHROME_VERSION}_${TARGETARCH}.deb" || exit 1
     
    Performance
    Consolidate apt-get update and apt-get install into a single RUN instruction to optimize Docker layers.

    Consider consolidating the apt-get update and apt-get install commands into a single RUN
    instruction to reduce the number of layers in the Docker image, which can help in
    optimizing the build process and the size of the final image.

    NodeChrome/Dockerfile [21-26]

    -&& apt-get update -qqy \
    -&& apt-get -qqy --no-install-recommends install --allow-downgrades ./google-chrome.deb
    +&& apt-get update -qqy && apt-get -qqy --no-install-recommends install --allow-downgrades ./google-chrome.deb
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @VietND96 VietND96 merged commit 677168f into SeleniumHQ:trunk Apr 23, 2024
    12 checks passed
    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.

    1 participant