Skip to content

Docker: Unified node config session-timeout from generated TOML file #2598

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 20, 2025

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jan 20, 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 #2559
By default, it is present in the generated TOML config file, with no CLI option to avoid unexpected higher precedence value taking effect.

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

Bug fix, Enhancement


Description

  • Unified handling of session-timeout configuration in TOML files.

  • Removed redundant session-timeout environment variable handling in start-selenium-node.sh.

  • Adjusted TOML file generation to ensure proper formatting for session-timeout.

  • Improved consistency in configuration handling across Node and Standalone setups.


Changes walkthrough 📝

Relevant files
Enhancement
start-selenium-node.sh
Removed redundant `session-timeout` environment variable handling

NodeBase/start-selenium-node.sh

  • Removed redundant handling of SE_NODE_SESSION_TIMEOUT environment
    variable.
  • Simplified logic by relying on TOML configuration for session-timeout.
  • +0/-4     
    Bug fix
    generate_config
    Fixed `session-timeout` formatting in TOML generation       

    NodeBase/generate_config

  • Adjusted session-timeout entry to remove unnecessary quotes.
  • Ensured proper formatting for TOML configuration generation.
  • +1/-1     
    generate_config
    Corrected `session-timeout` formatting in Standalone TOML generation

    Standalone/generate_config

  • Corrected session-timeout entry to remove unnecessary quotes.
  • Ensured consistency with NodeBase configuration generation.
  • +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    2559 - Partially compliant

    Compliant requirements:

    • Allow overriding session timeout parameter using config.toml file
    • Move configuration from environment variables to toml file

    Non-compliant requirements:

    • Fix issue where logging parameters in toml file are not being applied

    Requires further human verification:

    • Verify that session-timeout value from toml file is actually being applied at runtime
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Data Type Change

    The session-timeout value is now being written without quotes, which changes its type from string to number in the TOML file. Verify this change is compatible with how Selenium reads this configuration.

    echo "session-timeout = ${SE_NODE_SESSION_TIMEOUT}" >>"$FILENAME"
    Breaking Change

    Removing SE_NODE_SESSION_TIMEOUT handling could break existing deployments that rely on this environment variable. Consider deprecation notice or backward compatibility.

    if [ ! -z "$SE_NODE_ENABLE_MANAGED_DOWNLOADS" ]; then
      append_se_opts "--enable-managed-downloads" "${SE_NODE_ENABLE_MANAGED_DOWNLOADS}"
    fi

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add missing environment variable validation

    Add validation to ensure SE_NODE_SESSION_TIMEOUT is set and contains a valid numeric
    value before writing to config file

    NodeBase/generate_config [47]

    +if [[ -z "${SE_NODE_SESSION_TIMEOUT}" ]] || ! [[ "${SE_NODE_SESSION_TIMEOUT}" =~ ^[0-9]+$ ]]; then
    +  echo "SE_NODE_SESSION_TIMEOUT not set or invalid, using default value" 1>&2
    +  SE_NODE_SESSION_TIMEOUT=300
    +fi
     echo "session-timeout = ${SE_NODE_SESSION_TIMEOUT}" >>"$FILENAME"
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds crucial input validation for SE_NODE_SESSION_TIMEOUT, preventing potential configuration errors by ensuring the variable exists and contains a valid numeric value, with a sensible default fallback.

    8

    @VietND96 VietND96 merged commit 7f756c1 into trunk Jan 20, 2025
    27 checks passed
    @VietND96 VietND96 deleted the session-timeout branch January 20, 2025 09:48
    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]: Unable to override session timeout and other configuration parameters using toml file
    1 participant