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: Enable reject-unsupported-caps in standalone by default #2346

Merged
merged 1 commit into from
Aug 10, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Aug 10, 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

Fixes SeleniumHQ/selenium#14339

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, Configuration changes


Description

  • Added --relax-checks and --detect-drivers false options to the standalone Selenium startup script.
  • Added multiple environment variable checks in the Docker Selenium Grid startup script to append additional Selenium options.
  • Introduced SE_REJECT_UNSUPPORTED_CAPS environment variable in the Dockerfile with a default value of true.

Changes walkthrough 📝

Relevant files
Enhancement
start-selenium-standalone.sh
Add relax checks and disable driver detection in standalone script

Standalone/start-selenium-standalone.sh

  • Added --relax-checks and --detect-drivers false options to the
    standalone Selenium startup script.
  • +2/-0     
    start-selenium-grid-docker.sh
    Add environment variable checks for additional Selenium options

    StandaloneDocker/start-selenium-grid-docker.sh

  • Added multiple environment variable checks to append Selenium options.
  • Included options for managed downloads, CDP, register period, register
    cycle, and heartbeat period.
  • +25/-0   
    Configuration changes
    Dockerfile
    Add reject unsupported capabilities environment variable 

    Standalone/Dockerfile

  • Added SE_REJECT_UNSUPPORTED_CAPS environment variable with default
    value true.
  • +1/-0     

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

    @VietND96 VietND96 merged commit 821eff3 into SeleniumHQ:trunk Aug 10, 2024
    7 of 15 checks passed
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Configuration Consistency
    The new flags --relax-checks ${SE_RELAX_CHECKS} and --detect-drivers false are added, but there is no corresponding update in the Dockerfile or documentation to reflect these changes or their default values.

    Script Output Consistency
    The script outputs messages for new configuration options, but does not handle potential issues with unset environment variables which could lead to incorrect or misleading output.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Refactor repeated environment variable checks into a function

    Consolidate the repeated pattern of checking for environment variables and appending
    Selenium options into a function. This will improve the maintainability and
    readability of the script.

    StandaloneDocker/start-selenium-grid-docker.sh [17-39]

    -if [ ! -z "$SE_NODE_ENABLE_MANAGED_DOWNLOADS" ]; then
    -  echo "Appending Selenium options: --enable-managed-downloads ${SE_NODE_ENABLE_MANAGED_DOWNLOADS}"
    -  SE_OPTS="$SE_OPTS --enable-managed-downloads ${SE_NODE_ENABLE_MANAGED_DOWNLOADS}"
    -fi
    +append_option_if_set() {
    +  local var_name=$1
    +  local option_name=$2
    +  local value=$(eval echo \$$var_name)
    +  if [ ! -z "$value" ]; then
    +    echo "Appending Selenium options: $option_name $value"
    +    SE_OPTS="$SE_OPTS $option_name $value"
    +  fi
    +}
    +append_option_if_set "SE_NODE_ENABLE_MANAGED_DOWNLOADS" "--enable-managed-downloads"
     ...
     
    Suggestion importance[1-10]: 9

    Why: Consolidating repeated patterns into a function significantly improves the maintainability and readability of the script. This refactor reduces code duplication and makes future updates easier.

    9
    Best practice
    Add a conditional check for the SE_RELAX_CHECKS option

    Consider adding a conditional check for SE_RELAX_CHECKS before appending it to the
    Java options. This ensures that the option is only added when explicitly configured,
    enhancing configurability and preventing potential issues with default settings.

    Standalone/start-selenium-standalone.sh [164]

    ---relax-checks ${SE_RELAX_CHECKS} \
    +if [ ! -z "$SE_RELAX_CHECKS" ]; then
    +  --relax-checks ${SE_RELAX_CHECKS} \
    +fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a conditional check for SE_RELAX_CHECKS enhances configurability and prevents potential issues with default settings. This is a best practice for handling optional configuration parameters.

    8
    Documentation
    Document the SE_REJECT_UNSUPPORTED_CAPS environment variable

    Ensure that the new environment variable SE_REJECT_UNSUPPORTED_CAPS is documented in
    the Dockerfile comments to inform users of its purpose and default setting.

    Standalone/Dockerfile [31]

    +# Boolean value, maps "--reject-unsupported-caps", default is true
     SE_REJECT_UNSUPPORTED_CAPS=true \
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding documentation for the new environment variable SE_REJECT_UNSUPPORTED_CAPS helps inform users of its purpose and default setting, improving the usability and clarity of the Dockerfile.

    7

    @VietND96 VietND96 added this to the 4.23.1 milestone Aug 10, 2024
    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]: Fly.io
    1 participant