Check CLI options duplicated from env vars and SE_OPTS

[VietND96]

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

Support #2448 to detect duplicated CLI options might have via env vars

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

---

Description

• Enhanced the Selenium Grid scripts to dynamically append command-line options based on environment variables, improving flexibility and reducing redundancy.
• Removed hardcoded options in scripts, allowing for more dynamic configuration through environment variables.
• Improved error handling by checking for required environment variables before proceeding with script execution.
• Updated Dockerfile to include a new CONFIG_FILE environment variable for better configuration management.

---

Changes walkthrough 📝

	Relevant files
Enhancement	start-selenium-grid-distributor.sh Enhance distributor script with dynamic option appending  Distributor/start-selenium-grid-distributor.sh Added checks for environment variables before appending options. Removed hardcoded command-line options in favor of dynamic appending. Improved error handling for missing environment variables. +51/-37  start-selenium-grid-hub.sh Improve hub script with flexible option handling                  Hub/start-selenium-grid-hub.sh Replaced hardcoded options with dynamic appending. Added checks for new environment variables. Improved flexibility in configuration handling. +29/-16  start-selenium-node.sh Update node script for dynamic configuration                          NodeBase/start-selenium-node.sh Added dynamic appending of Selenium options. Introduced checks for new configuration variables. Enhanced configuration flexibility. +14/-7
Configuration changes	Dockerfile Update Dockerfile with new configuration environment variable Base/Dockerfile Added CONFIG_FILE environment variable. Updated environment configuration settings. +1/-0

---

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

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The script now exits immediately when certain environment variables are not set. This could potentially cause issues if the script is run in an environment where these variables are set differently or not required. Configuration Change The script now uses a new environment variable CONFIG_FILE to specify the configuration file path. This change should be validated to ensure it doesn't break existing setups. Potential Oversight The script now appends the --config option based on the CONFIG_FILE environment variable, but it still reads and displays the content of $CONFIG_FILE. This might lead to confusion if the two are not in sync.

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Enhancement	Refactor environment variable checks and option setting into a reusable function to improve code maintainability and reduce repetition Consider using a function to check and set environment variables to reduce code duplication and improve maintainability. Distributor/start-selenium-grid-distributor.sh [31-47] -if [[ ! -z "${SE_EVENT_BUS_HOST}" ]]; then - if [[ ! -z "${SE_EVENT_BUS_PUBLISH_PORT}" ]]; then - append_se_opts "--publish-events" "tcp://${SE_EVENT_BUS_HOST}:${SE_EVENT_BUS_PUBLISH_PORT}" - else - echo "SE_EVENT_BUS_PUBLISH_PORT not set, exiting!" 1>&2 +check_and_set_env_var() { + local var_name=$1 + local option_name=$2 + local additional_var=$3 + + if [[ -z "${!var_name}" ]]; then + echo "${var_name} not set, exiting!" 1>&2 exit 1 fi - if [[ ! -z "${SE_EVENT_BUS_SUBSCRIBE_PORT}" ]]; then - append_se_opts "--subscribe-events" "tcp://${SE_EVENT_BUS_HOST}:${SE_EVENT_BUS_SUBSCRIBE_PORT}" + + if [[ -n "$additional_var" ]]; then + if [[ -z "${!additional_var}" ]]; then + echo "${additional_var} not set, exiting!" 1>&2 + exit 1 + fi + append_se_opts "$option_name" "tcp://${!var_name}:${!additional_var}" else - echo "SE_EVENT_BUS_SUBSCRIBE_PORT not set, exiting!" 1>&2 - exit 1 + append_se_opts "$option_name" "${!var_name}" fi -else - echo "SE_EVENT_BUS_HOST not set, exiting!" 1>&2 - exit 1 -fi +} +check_and_set_env_var "SE_EVENT_BUS_HOST" "--publish-events" "SE_EVENT_BUS_PUBLISH_PORT" +check_and_set_env_var "SE_EVENT_BUS_HOST" "--subscribe-events" "SE_EVENT_BUS_SUBSCRIBE_PORT" + Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion effectively reduces code duplication by introducing a reusable function for checking and setting environment variables, which enhances maintainability and readability.	8
	Simplify configuration by using parameter expansion with default values for environment variables Use parameter expansion to provide default values for environment variables, reducing the need for multiple if statements. Hub/start-selenium-grid-hub.sh [108-122] -if [ ! -z "${SE_SESSION_REQUEST_TIMEOUT}" ]; then - append_se_opts "--session-request-timeout" "${SE_SESSION_REQUEST_TIMEOUT}" -fi +append_se_opts "--session-request-timeout" "${SE_SESSION_REQUEST_TIMEOUT:-300}" +append_se_opts "--session-retry-interval" "${SE_SESSION_RETRY_INTERVAL:-15}" +append_se_opts "--healthcheck-interval" "${SE_HEALTHCHECK_INTERVAL:-60}" +append_se_opts "--relax-checks" "${SE_RELAX_CHECKS:-true}" -if [ ! -z "${SE_SESSION_RETRY_INTERVAL}" ]; then - append_se_opts "--session-retry-interval" "${SE_SESSION_RETRY_INTERVAL}" -fi - -if [ ! -z "${SE_HEALTHCHECK_INTERVAL}" ]; then - append_se_opts "--healthcheck-interval" "${SE_HEALTHCHECK_INTERVAL}" -fi - -if [ ! -z "${SE_RELAX_CHECKS}" ]; then - append_se_opts "--relax-checks" "${SE_RELAX_CHECKS}" -fi - Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion improves code simplicity by using parameter expansion to set default values, reducing the need for multiple conditional checks and enhancing maintainability.	7
Best practice	Implement a function to check for required environment variables, improving code readability and maintainability Use a more robust method to check if environment variables are set and not empty. NodeBase/start-selenium-node.sh [38-51] -if [[ -z "${SE_EVENT_BUS_HOST}" ]]; then - echo "SE_EVENT_BUS_HOST not set, exiting!" 1>&2 - exit 1 -fi +check_required_var() { + if [[ -z "${!1}" ]]; then + echo "$1 is not set or is empty, exiting!" 1>&2 + exit 1 + fi +} -if [[ -z "${SE_EVENT_BUS_PUBLISH_PORT}" ]]; then - echo "SE_EVENT_BUS_PUBLISH_PORT not set, exiting!" 1>&2 - exit 1 -fi +check_required_var SE_EVENT_BUS_HOST +check_required_var SE_EVENT_BUS_PUBLISH_PORT +check_required_var SE_EVENT_BUS_SUBSCRIBE_PORT -if [[ -z "${SE_EVENT_BUS_SUBSCRIBE_PORT}" ]]; then - echo "SE_EVENT_BUS_SUBSCRIBE_PORT not set, exiting!" 1>&2 - exit 1 -fi - Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion introduces a function to check for required environment variables, which enhances code readability and maintainability by reducing redundancy and improving clarity.	8

💡 Need additional feedback ? start a PR chat

[qodo-merge-pro]

CI Failure Feedback 🧐

(Checks updated until commit 6a3707f)

Action: Rerun workflow when failure

Failed stage: Authenticate GitHub CLI for PR [❌]

Failure summary: The action failed due to an error in validating the GitHub CLI token: The token is missing the required scope read:org, which is necessary for the authentication process. As a result, the process was completed with exit code 1, indicating a failure.

Relevant error logs: 1: ##[group]Operating System 2: Ubuntu ... 28: SecurityEvents: write 29: Statuses: write 30: ##[endgroup] 31: Secret source: Actions 32: Prepare workflow directory 33: Prepare all required actions 34: Getting action download info 35: Download action repository 'actions/checkout@main' (SHA:11bd71901bbe5b1630ceea73d27597364c9af683) 36: Complete job name: Rerun workflow when failure ... 48: show-progress: true 49: lfs: false 50: submodules: false 51: set-safe-directory: true 52: env: 53: GH_CLI_TOKEN: *** 54: GH_CLI_TOKEN_PR: *** 55: RUN_ID: 11621805281 56: RERUN_FAILED_ONLY: true ... 119: ##[group]Run sudo apt update 120: �[36;1msudo apt update�[0m 121: �[36;1msudo apt install gh�[0m 122: shell: /usr/bin/bash -e {0} 123: env: 124: GH_CLI_TOKEN: *** 125: GH_CLI_TOKEN_PR: *** 126: RUN_ID: 11621805281 127: RERUN_FAILED_ONLY: true ... 174: 0 upgraded, 0 newly installed, 0 to remove and 33 not upgraded. 175: ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token 176: �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m 177: shell: /usr/bin/bash -e {0} 178: env: 179: GH_CLI_TOKEN: *** 180: GH_CLI_TOKEN_PR: *** 181: RUN_ID: 11621805281 182: RERUN_FAILED_ONLY: true 183: RUN_ATTEMPT: 2 184: ##[endgroup] 185: error validating token: missing required scope 'read:org' 186: ##[error]Process completed with exit code 1.

---

✨ CI feedback usage guide:

---

The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
The tool analyzes the failed checks and provides several feedbacks:

• Failed stage
• Failed test name
• Failure summary
• Relevant error logs

In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

/checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"

where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

Configuration options

• enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
• excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
• enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
• persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
• final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

See more information about the checks tool in the docs.