Skip to content

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Oct 2, 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 #2968
Single/Double quote " is not retained when assigning value to variable in shell script, so it breaks the JSON format.

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


Description

  • Replace eval with printf -v for safer variable assignment

  • Add validation for SE_NODE_MAX_SESSIONS parameter

  • Fix stereotype merging regression in Docker configuration


Diagram Walkthrough

flowchart LR
  A["eval usage"] -- "replace with" --> B["printf -v"]
  C["SE_NODE_MAX_SESSIONS"] -- "add validation" --> D["positive integer check"]
  B --> E["safer variable assignment"]
  D --> F["prevent invalid config"]
Loading

File Walkthrough

Relevant files
Bug fix
generate_config
Improve variable handling safety and add session validation

NodeBase/generate_config

  • Replace eval statements with printf -v for safer variable assignment
  • Add validation for SE_NODE_MAX_SESSIONS to ensure positive integer
    values
  • Preserve quotes verbatim in environment variable handling
  • Fix stereotype merging functionality in Docker configuration
+11/-6   

Copy link
Contributor

qodo-merge-pro bot commented Oct 2, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Misconfiguration risk

Description: The validation for SE_NODE_MAX_SESSIONS silently ignores invalid or zero values instead of
failing fast, which can cause unexpected unlimited concurrency and resource exhaustion if
misconfigured.
generate_config [182-185]

Referred Code
# Validate SE_NODE_MAX_SESSIONS is a positive integer
if [[ "${SE_NODE_MAX_SESSIONS}" =~ ^[0-9]+$ ]] && [[ "${SE_NODE_MAX_SESSIONS}" -gt 0 ]]; then
    echo "max-sessions = ${SE_NODE_MAX_SESSIONS}" >>"$FILENAME"
fi
Ticket Compliance
🟡
🎫 #2968
🟢 SE_NODE_STEREOTYPE_EXTRA must be merged into config.toml so custom stereotype attributes
appear in the Node stereotype in Docker Hub-Node mode.
Provide a supported way to add custom stereotype attributes without breaking ChromeDriver
version detection.
Ensure targeting Nodes via custom capability like `myApp:version` works as expected.
Maintain compatibility with recent versions where regression occurred (last known working
4.8.0).
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link
Contributor

qodo-merge-pro bot commented Oct 2, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Warn user about invalid values

Add a warning message for invalid SE_NODE_MAX_SESSIONS values. Currently,
invalid values are silently ignored, which can be confusing for users.

NodeBase/generate_config [182-185]

 # Validate SE_NODE_MAX_SESSIONS is a positive integer
-if [[ "${SE_NODE_MAX_SESSIONS}" =~ ^[0-9]+$ ]] && [[ "${SE_NODE_MAX_SESSIONS}" -gt 0 ]]; then
-    echo "max-sessions = ${SE_NODE_MAX_SESSIONS}" >>"$FILENAME"
+if [[ -n "${SE_NODE_MAX_SESSIONS}" ]]; then
+    if [[ "${SE_NODE_MAX_SESSIONS}" =~ ^[0-9]+$ ]] && [[ "${SE_NODE_MAX_SESSIONS}" -gt 0 ]]; then
+        echo "max-sessions = ${SE_NODE_MAX_SESSIONS}" >>"$FILENAME"
+    else
+        echo "Warning: SE_NODE_MAX_SESSIONS is set to '${SE_NODE_MAX_SESSIONS}', which is not a positive integer. Ignoring." >&2
+    fi
 fi
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that silently ignoring an invalid configuration value is poor user experience, and providing a warning is a significant improvement for usability.

Low
  • Update

…reotypes

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
@VietND96 VietND96 merged commit 558105a into trunk Oct 2, 2025
55 of 57 checks passed
@VietND96 VietND96 deleted the fix-max-session branch October 2, 2025 10:03
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]: SE_NODE_STEREOTYPE_EXTRA not merged into config.toml in Docker Hub-Node mode
1 participant