Skip to content

Docker: Unified the env var $CONFIG_FILE from Base #2577

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

Merged
merged 1 commit into from
Jan 14, 2025
Merged

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jan 13, 2025

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

Fixes #2556

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

  • Unified the handling of configuration files across scripts.

  • Added support for dynamic configuration file paths via CONFIG_FILE.

  • Updated environment variables and Dockerfiles for consistency.

  • Improved logging of configuration details during startup.


Changes walkthrough 📝

Relevant files
Enhancement
start-selenium-grid-docker.sh
Dynamic configuration file handling for Node Docker           

NodeDocker/start-selenium-grid-docker.sh

  • Added support for dynamic configuration file paths.
  • Improved logging of configuration details.
  • Removed hardcoded configuration file references.
  • +12/-3   
    start-selenium-standalone.sh
    Dynamic configuration file handling for Standalone             

    Standalone/start-selenium-standalone.sh

  • Added support for dynamic configuration file paths.
  • Improved logging of configuration details.
  • Removed hardcoded configuration file references.
  • +5/-2     
    start-selenium-grid-docker.sh
    Dynamic configuration file handling for Standalone Docker

    StandaloneDocker/start-selenium-grid-docker.sh

  • Added support for dynamic configuration file paths.
  • Improved logging of configuration details.
  • Removed hardcoded configuration file references.
  • +8/-3     
    Configuration changes
    Dockerfile
    Added dynamic configuration file environment variables     

    Base/Dockerfile

  • Added CONFIG_FILE and GENERATE_CONFIG environment variables.
  • Updated file paths to use CONFIG_FILE dynamically.
  • Adjusted permissions and initialization for CONFIG_FILE.
  • +5/-3     
    Dockerfile
    Removed redundant configuration environment variables       

    NodeBase/Dockerfile

  • Removed redundant CONFIG_FILE and GENERATE_CONFIG variables.
  • Simplified environment variable definitions.
  • +0/-3     
    Dockerfile
    Removed redundant configuration environment variables       

    Sessions/Dockerfile

  • Removed redundant CONFIG_FILE and GENERATE_CONFIG variables.
  • Simplified environment variable definitions.
  • +0/-3     

    💡 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: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    File permissions:
    The configuration file permissions are set to 775 (world-readable and executable) which might be too permissive if the config file contains sensitive information like credentials or internal network details. Consider using more restrictive permissions (600 or 640) for configuration files.

    ⚡ Recommended focus areas for review

    File Access

    The script attempts to read and display the config file without first checking if it exists, which could cause errors if CONFIG_FILE is not properly set or the file is missing

    echo "Selenium Grid Node Docker configuration: "
    cat "${CONFIG_FILE}"
    Typo

    There's a typo in the startup message ("DOcker" instead of "Docker") which could cause confusion in logs

    echo "Starting Selenium Grid Standalone DOcker..."

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for file operations to prevent script failures

    Add a check to verify that CONFIG_FILE exists and is readable before attempting to
    read it with cat. This prevents potential script failures if the file is missing or
    inaccessible.

    NodeDocker/start-selenium-grid-docker.sh [98-99]

     echo "Selenium Grid Node Docker configuration: "
    -cat "${CONFIG_FILE}"
    +if [ -r "${CONFIG_FILE}" ]; then
    +  cat "${CONFIG_FILE}"
    +else
    +  echo "Warning: Configuration file ${CONFIG_FILE} is not readable or does not exist"
    +fi
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds important error handling to prevent script failures when the config file is missing or inaccessible, which could cause silent failures or confusing error messages in production.

    8
    Security
    Ensure proper file permissions are set during initialization

    Initialize CONFIG_FILE with proper permissions before touching it to ensure it's
    created with the correct ownership and access rights.

    Base/Dockerfile [106-107]

     && touch ${CONFIG_FILE} \
    +&& chown ${SEL_USER}:${SEL_GROUP} ${CONFIG_FILE} \
    +&& chmod 664 ${CONFIG_FILE} \
     && chown -R ${SEL_USER}:${SEL_GROUP} /opt/selenium
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves security by explicitly setting proper file permissions and ownership for the config file, preventing potential permission-related issues.

    7
    General
    Fix inconsistent capitalization in log messages

    Fix the typo in the "Docker" word in the startup message, which currently shows
    "DOcker" with incorrect capitalization.

    StandaloneDocker/start-selenium-grid-docker.sh [101]

    -echo "Starting Selenium Grid Standalone DOcker..."
    +echo "Starting Selenium Grid Standalone Docker..."
    • Apply this suggestion
    Suggestion importance[1-10]: 2

    Why: While the suggestion correctly identifies a typo in the log message, this is a minor cosmetic issue that doesn't affect functionality.

    2

    @VietND96 VietND96 changed the title Docker: Unified the env var from Base Docker: Unified the env var CONFIG_FILE from Base Jan 13, 2025
    @VietND96 VietND96 changed the title Docker: Unified the env var CONFIG_FILE from Base Docker: Unified the env var $CONFIG_FILE from Base Jan 13, 2025
    Copy link

    qodo-merge-pro bot commented Jan 13, 2025

    CI Failure Feedback 🧐

    (Checks updated until commit 54d000c)

    Action: Rerun workflow when failure

    Failed stage: Authenticate GitHub CLI for PR [❌]

    Failure summary:

    The action failed because the GitHub authentication token lacks the required 'read:org' permission
    scope. The error occurred during the gh auth login command, which requires proper authorization
    scopes to authenticate with GitHub CLI.

    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:cbb722410c2e876e24abbe8de2cc27693e501dcb)
    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: 12755004542
    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: 12755004542
    127:  RERUN_FAILED_ONLY: true
    ...
    
    176:  0 upgraded, 0 newly installed, 0 to remove and 55 not upgraded.
    177:  ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token
    178:  �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m
    179:  shell: /usr/bin/bash -e {0}
    180:  env:
    181:  GH_CLI_TOKEN: ***
    182:  GH_CLI_TOKEN_PR: ***
    183:  RUN_ID: 12755004542
    184:  RERUN_FAILED_ONLY: true
    185:  RUN_ATTEMPT: 1
    186:  ##[endgroup]
    187:  error validating token: missing required scope 'read:org'
    188:  ##[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.

    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @VietND96 VietND96 merged commit 758e133 into trunk Jan 14, 2025
    36 checks passed
    @VietND96 VietND96 deleted the config-file branch January 14, 2025 00:10
    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]: $CONFIG_FILE not working in standalone
    1 participant