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

fix!: harmonized mount points to /opt/selenium/config.toml #2343

Merged
merged 2 commits into from
Aug 9, 2024

Conversation

Trigtrig
Copy link
Contributor

@Trigtrig Trigtrig commented Aug 9, 2024

User description

Description

Fixes #2340. Will break existing docker setups using the "old" mount point /opt/bin/config.toml.

Motivation and Context

Fixes #2340

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, Documentation


Description

  • Updated the mount point for the configuration file from /opt/bin/config.toml to /opt/selenium/config.toml in various scripts and configurations.
  • Updated the documentation to reflect the new mount point for the configuration file.
  • This change will break existing Docker setups using the old mount point.

Changes walkthrough 📝

Relevant files
Enhancement
start-selenium-grid-docker.sh
Harmonize mount point in NodeDocker script                             

NodeDocker/start-selenium-grid-docker.sh

  • Updated the mount point for the configuration file from
    /opt/bin/config.toml to /opt/selenium/config.toml.
  • +1/-1     
    start-selenium-grid-docker.sh
    Harmonize mount point in StandaloneDocker script                 

    StandaloneDocker/start-selenium-grid-docker.sh

  • Updated the mount point for the configuration file from
    /opt/bin/config.toml to /opt/selenium/config.toml.
  • +1/-1     
    docker-compose-v3-dynamic-grid.yml
    Update mount point in Docker Compose configuration             

    docker-compose-v3-dynamic-grid.yml

  • Changed the mount point for the configuration file in the Docker
    Compose configuration.
  • +1/-1     
    docker-compose-v3-test-node-docker.yaml
    Update mount point in test Docker Compose configuration   

    tests/docker-compose-v3-test-node-docker.yaml

  • Changed the mount point for the configuration file in the test Docker
    Compose configuration.
  • +1/-1     
    Documentation
    README.md
    Update documentation for new mount point                                 

    README.md

  • Updated documentation to reflect the new mount point for the
    configuration file.
  • +8/-8     

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

    Copy link

    PR Reviewer Guide 🔍

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

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Use an environment variable for the configuration file path to enhance flexibility across different environments

    Ensure that the configuration file path is correctly referenced in all environments.
    Consider using an environment variable to define the base path for configurations,
    which can be set differently depending on the deployment environment.

    NodeDocker/start-selenium-grid-docker.sh [96]

    ---config /opt/selenium/config.toml \
    +--config ${CONFIG_PATH}/config.toml \
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion improves maintainability by allowing the configuration file path to be easily changed based on the environment, which is a significant improvement for deployment flexibility.

    8
    Best practice
    Replace hard-coded paths with environment variables to increase script adaptability

    To avoid hard-coding paths in your scripts, use an environment variable to specify
    the directory path. This change will make the script more adaptable to different
    deployment scenarios.

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

    ---config /opt/selenium/config.toml \
    +--config ${SELENIUM_CONFIG_DIR}/config.toml \
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using environment variables instead of hard-coded paths enhances the adaptability of the script to different deployment scenarios, which is a best practice.

    8
    Standardize the configuration file path across docker-compose files using an environment variable

    To ensure consistency and avoid errors, use a common environment variable across all
    docker-compose files to define the path for configuration files.

    docker-compose-v3-dynamic-grid.yml [10]

    -- ./NodeDocker/config.toml:/opt/selenium/config.toml
    +- ${CONFIG_PATH}/config.toml:/opt/selenium/config.toml
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Standardizing the configuration file path using an environment variable ensures consistency and reduces the risk of errors, which is a best practice.

    8
    Documentation
    Use placeholders for paths in documentation to guide users in configuring their environments

    It's a good practice to avoid hard-coding paths in documentation examples. Instead,
    use placeholders and describe how to set them in the text.

    README.md [761]

    --v ${PWD}/config.toml:/opt/selenium/config.toml \
    +-v ${CONFIG_DIRECTORY}/config.toml:/opt/selenium/config.toml \
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion improves the documentation by making it clearer for users to configure their environments, although it is less critical than code changes.

    7

    @VietND96 VietND96 merged commit ea50649 into SeleniumHQ:trunk Aug 9, 2024
    28 of 29 checks passed
    @VietND96
    Copy link
    Member

    VietND96 commented Aug 9, 2024

    Thank you, @Trigtrig !

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    2 participants