Skip to content

SE_NODE_STEREOTYPE_EXTRA to append custom capabilities to default Node stereotype #2624

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 2 commits into from
Jan 31, 2025

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jan 31, 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 #2506

In another case, if you want to retain the default Node stereotype and append additional capabilities, you can use the SE_NODE_STEREOTYPE_EXTRA environment variable to set your capabilities. Those will be merged to the default stereotype. For example:

$ docker run -d \
  -e SE_EVENT_BUS_HOST=<event_bus_ip|event_bus_name> \
  -e SE_EVENT_BUS_PUBLISH_PORT=4442 \
  -e SE_EVENT_BUS_SUBSCRIBE_PORT=4443 \
  -e SE_NODE_STEREOTYPE_EXTRA="{\"myApp:version\":\"beta\", \"myApp:publish:\":\"public\"}" \
  --shm-size="2g" selenium/node-chrome:4.28.1-20250123

This help setting custom capabilities for matching specific Nodes. For example, you added your custom capabilities when starting the Node, and you want assign a test to run on that Node which matches your capabilities. For example in test code:

options = ChromeOptions()
options.set_capability('myApp:version', 'beta')
options.set_capability('myApp:publish', 'public')
driver = webdriver.Remote(options=options, command_executor=SELENIUM_GRID_URL)

Noted: Your custom capabilities with key values should be in W3C capabilities convention, extension capabilities key must contain a ":" (colon) character, denoting an implementation specific namespace.
Noted: Ensure that Node config detect-drivers = false in config.toml (or --detect-drivers false in CLI option) to make feature setting custom capabilities for matching specific Nodes get working.
In addition, default Node stereotype includes capability se:containerName which can visible in node capabilities, or session capabilities to identify the container name where the node/session is running. The prefixed se:containerName is not included in slot matcher. By default, value is getting from hostname command in container, this value is equivalent to the container_id that you saw via docker ps command. If you want to override this value, you can set the environment variable SE_NODE_CONTAINER_NAME to your desired value. For example, when deploy to Kubernetes cluster, you can assign Pod name to env var SE_NODE_CONTAINER_NAME to track a node is running in which Pod.

  env:
    - name: SE_NODE_CONTAINER_NAME
      valueFrom:
        fieldRef:
          fieldPath: metadata.name

In an advanced case, where you control to spawn up a Node container, let it register to Hub, and then trigger a test to be assigned exactly to run on that Node. By default, the value of command $(hostname) is added to capability name container:hostname in Node stereotype. Combine with above feature setting custom capabilities for matching specific Nodes. You can use the hostname of the Node container just spawned up and set it as a custom capability. For example, in Python binding:

$ docker run -d --name my-node-1 -e SE_EVENT_BUS_HOST=localhost -e SE_EVENT_BUS_PUBLISH_PORT=4442 -e SE_EVENT_BUS_SUBSCRIBE_PORT=4443 \
  --shm-size="2g" selenium/node-chrome:4.28.1-20250123
$ docker exec -i my-node-1 hostname
a6971f95bbab
options = ChromeOptions()
options.set_capability('container:hostname', 'a6971f95bbab')
driver = webdriver.Remote(options=options, command_executor=SELENIUM_GRID_URL)

Noted: Those above changes require new image tag where the changeset is included & released.

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


Description

  • Introduced SE_NODE_STEREOTYPE_EXTRA to append custom capabilities to default Node stereotype.

  • Added a Python script (json_merge.py) for merging JSON strings.

  • Updated Dockerfile and configuration scripts to integrate SE_NODE_STEREOTYPE_EXTRA.

  • Enhanced documentation with examples and explanations for new environment variables.


Changes walkthrough 📝

Relevant files
Enhancement
5 files
json_merge.py
Added Python script for merging JSON strings                         
+20/-0   
extract_env.py
Updated script to include additional file types                   
+1/-1     
Dockerfile
Integrated `json_merge.py` into Docker image                         
+2/-1     
generate_config
Added logic to merge `SE_NODE_STEREOTYPE_EXTRA` into Node stereotype
+11/-1   
generate_config
Integrated SE_NODE_STEREOTYPE_EXTRA into standalone Node configuration
+11/-1   
Documentation
3 files
ENV_VARIABLES.md
Documented new environment variables for Node configuration
+22/-1   
README.md
Added usage examples for SE_NODE_STEREOTYPE_EXTRA and related features
+50/-1   
description.yaml
Added descriptions for new environment variables                 
+65/-0   
Configuration changes
1 files
value.yaml
Added default values for new environment variables             
+43/-1   

Need help?
  • Type /help how to ... in the comments thread for any questions 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:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The bare except clause catches all exceptions silently and returns the first JSON string. This could mask JSON parsing errors or other issues. Consider catching specific exceptions and providing more detailed error messages.

    except:
      # Print the first JSON string if an error occurs
      print(json_str1)
    JSON Validation

    The code merges JSON strings without validating the structure of SE_NODE_STEREOTYPE_EXTRA. Invalid JSON structure could lead to incorrect node configuration.

    if [[ -n "${SE_NODE_STEREOTYPE_EXTRA}" ]]; then
        echo "Merging SE_NODE_STEREOTYPE_EXTRA=${SE_NODE_STEREOTYPE_EXTRA} to main stereotype"
        SE_NODE_STEREOTYPE="$(python3 /opt/bin/json_merge.py "${SE_NODE_STEREOTYPE}" "${SE_NODE_STEREOTYPE_EXTRA}")"
        if [[ $? -ne 0 ]]; then
            echo "Failed to merge SE_NODE_STEREOTYPE_EXTRA. Please check the format of the JSON string. Keep using main stereotype."
        else
            echo "Merged stereotype: ${SE_NODE_STEREOTYPE}"
        fi
    fi

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Improve error handling specificity

    Add specific error handling instead of catching all exceptions. Handle JSON parsing
    errors separately from other potential issues to provide better error messages.

    NodeBase/json_merge.py [7-20]

     try:
    -  # Parse JSON strings into dictionaries
       dict1 = json.loads(json_str1)
       dict2 = json.loads(json_str2)
    -  # Merge dictionaries
       merged_dict = {**dict1, **dict2}
    -  # Convert merged dictionary back to JSON string
       merged_json_str = json.dumps(merged_dict, separators=(',', ':'), ensure_ascii=True)
    -  # Print the merged JSON string
       print(merged_json_str)
    -except:
    -  # Print the first JSON string if an error occurs
    +except json.JSONDecodeError as e:
    +  print(f"Error parsing JSON: {e}", file=sys.stderr)
    +  print(json_str1)
    +  sys.exit(1)
    +except Exception as e:
    +  print(f"Unexpected error: {e}", file=sys.stderr)
       print(json_str1)
       sys.exit(1)
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves error handling by catching specific exceptions and providing more informative error messages, which helps with debugging and maintainability. The change is important but not critical since the original code already has basic error handling.

    7

    Copy link

    qodo-merge-pro bot commented Jan 31, 2025

    CI Feedback 🧐

    (Feedback updated until commit b8b89f3)

    A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

    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 when trying to authenticate using the
    provided GH_CLI_TOKEN_PR token.

    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:85e6279cec87321a52edac9c87bce653a07cf6c2)
    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: 13078721559
    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: 13078721559
    127:  RERUN_FAILED_ONLY: true
    ...
    
    174:  0 upgraded, 0 newly installed, 0 to remove and 114 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: 13078721559
    182:  RERUN_FAILED_ONLY: true
    183:  RUN_ATTEMPT: 1
    184:  ##[endgroup]
    185:  error validating token: missing required scope 'read:org'
    186:  ##[error]Process completed with exit code 1.
    

    Signed-off-by: Viet Nguyen Duc <[email protected]>
    @VietND96 VietND96 merged commit 8848360 into trunk Jan 31, 2025
    24 of 27 checks passed
    @VietND96 VietND96 deleted the node-custom-caps branch January 31, 2025 22:46
    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]: Grid status response does not have "se:containerName" set
    1 participant