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

Update Selenium Grid 4.27.0 and dependencies #2478

Merged
merged 2 commits into from
Nov 27, 2024
Merged

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Nov 26, 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

This pull request includes several updates and improvements to various Dockerfiles and the Makefile. The most important changes include version updates for dependencies, modifications to the Makefile to align with new Selenium versions, and adjustments to the Dockerfiles for better compatibility and functionality.

Dependency Version Updates:

  • Base/Dockerfile: Updated OPENTELEMETRY_VERSION to 1.44.1 and GRPC_VERSION to 1.68.1. Also updated CS_VERSION to 2.1.18.
  • NodeBase/Dockerfile: Changed NOVNC_VERSION to "v1.5.0" and WEBSOCKIFY_VERSION to "v0.12.0".

Makefile Adjustments:

  • Makefile: Updated BASE_RELEASE, BASE_VERSION, BINDING_VERSION, BASE_VERSION_NIGHTLY, and VERSION to align with Selenium 4.27.0.

Dockerfile Enhancements:

  • NodeBase/Dockerfile: Adjusted the RUN command to correctly handle version strings by stripping the 'v' prefix when moving directories.
  • NodeChromium/Dockerfile: Changed the Debian source list to use the "sid" main repository for installing Chromium.

Motivation and Context

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.

PR Type

enhancement, dependencies


Description

  • Updated dependency versions in Base/Dockerfile for OpenTelemetry, gRPC, and CS.
  • Updated Selenium version and related variables in Makefile to 4.27.0.
  • Updated noVNC and websockify versions and adjusted paths in NodeBase/Dockerfile.
  • Changed Chromium source list to "sid" main in NodeChromium/Dockerfile.

Changes walkthrough 📝

Relevant files
Dependencies
Dockerfile
Update dependency versions for OpenTelemetry, gRPC, and CS

Base/Dockerfile

  • Updated OPENTELEMETRY_VERSION to 1.44.1.
  • Updated GRPC_VERSION to 1.68.1.
  • Updated CS_VERSION to 2.1.18.
  • +3/-3     
    Dockerfile
    Update noVNC and websockify versions and paths                     

    NodeBase/Dockerfile

  • Changed NOVNC_SOURCE to "tags" and NOVNC_VERSION to "v1.5.0".
  • Changed WEBSOCKIFY_SOURCE to "tags" and WEBSOCKIFY_VERSION to
    "v0.12.0".
  • Adjusted paths for noVNC and websockify.
  • +7/-7     
    Enhancement
    Makefile
    Update Selenium version and related variables                       

    Makefile

  • Updated BASE_RELEASE and BASE_VERSION to 4.27.0.
  • Updated BINDING_VERSION to 4.27.0.
  • Updated BASE_VERSION_NIGHTLY to 4.28.0-SNAPSHOT.
  • Updated VERSION to 4.27.0.
  • +5/-5     
    Dockerfile
    Update Chromium source list to sid main                                   

    NodeChromium/Dockerfile

    • Changed Chromium source list to use "sid" main.
    +1/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Stability Risk
    Changing Debian repository from 'stable' to 'sid' (unstable) could introduce stability issues as 'sid' contains packages that haven't gone through full testing

    Version Compatibility
    Major version updates for noVNC (to v1.5.0) and websockify (to v0.12.0) should be validated for compatibility with existing functionality

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Using unstable package repositories in production Docker images can lead to system instability and unpredictable behavior

    Using "sid" (Debian unstable) as the main repository source can lead to system
    instability. Consider using "stable" or a specific stable version for production
    environments, or add apt preferences to pin Chromium packages only.

    NodeChromium/Dockerfile [12]

    -RUN echo "deb ${CHROMIUM_DEB_SITE}/ sid main" >> /etc/apt/sources.list \
    +RUN echo "deb ${CHROMIUM_DEB_SITE}/ stable main" >> /etc/apt/sources.list \
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Using Debian's "sid" (unstable) repository in a production Docker image is a significant risk as it can introduce system instability and unpredictable behavior. The suggestion to use "stable" is crucial for maintaining reliability.

    9
    Using relative paths in cleanup operations can lead to failures if directory structure changes unexpectedly

    The cd .. command is executed after cd websockify-${WEBSOCKIFY_VERSION#v} but before
    cleanup, which could fail if the previous directory structure is not as expected.
    Use absolute paths instead.

    NodeBase/Dockerfile [122-130]

     && cd websockify-${WEBSOCKIFY_VERSION#v} \
     && python3 setup.py install \
     && mv websockify /opt/bin/noVNC/utils/websockify \
     && mv run /opt/bin/noVNC/utils/websockify \
     && chmod +x /opt/bin/noVNC/utils/websockify/run \
    -&& cd .. \
    -&& rm -rf websockify-${WEBSOCKIFY_VERSION#v} \
    +&& cd / \
    +&& rm -rf /websockify-${WEBSOCKIFY_VERSION#v} \
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion to use absolute paths instead of relative paths for cleanup operations is a good practice to prevent potential failures, though the current implementation with relative paths would likely work in most scenarios.

    6

    💡 Need additional feedback ? start a PR chat

    Signed-off-by: Viet Nguyen Duc <[email protected]>
    @VietND96 VietND96 merged commit 978c050 into trunk Nov 27, 2024
    3 checks passed
    @VietND96 VietND96 deleted the novnc-stable-version branch November 27, 2024 00:21
    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.

    [🐛 Bug]: novnc does not start: problem with export named 'supportsWebCodecsH264Decode'
    1 participant