Skip to content

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Feb 27, 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

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


Description

  • Added support for setting custom Node capabilities in Selenium Grid Helm charts.

  • Updated Helm templates and configuration files to include nodeCustomCapabilities.

  • Enhanced test scripts to validate custom capabilities functionality.

  • Updated documentation to reflect new configuration options.


Changes walkthrough 📝

Relevant files
Tests
chart_test.sh
Add test support for custom Node capabilities                       

tests/charts/make/chart_test.sh

  • Introduced TEST_CUSTOM_SPECIFIC_NAME environment variable.
  • Added logic to set custom Node capabilities in Helm commands.
  • Exported TEST_CUSTOM_SPECIFIC_NAME for test scenarios.
  • +8/-0     
    dummy.yaml
    Update dummy template with custom capabilities                     

    tests/charts/templates/render/dummy.yaml

  • Added nodeCustomCapabilities with example values.
  • Enabled managed downloads for testing.
  • +2/-0     
    dummy_solution.yaml
    Update dummy solution template with custom capabilities   

    tests/charts/templates/render/dummy_solution.yaml

  • Added nodeCustomCapabilities with example values.
  • Enabled managed downloads for solution template.
  • +2/-0     
    Enhancement
    _helpers.tpl
    Integrate custom capabilities into Helm templates               

    charts/selenium-grid/templates/_helpers.tpl

  • Added nodeCustomCapabilities variable to autoscaling and pod
    templates.
  • Updated environment variables to include custom capabilities.
  • +7/-4     
    values.yaml
    Add custom capabilities to Helm values                                     

    charts/selenium-grid/values.yaml

  • Added nodeCustomCapabilities to global and Node-specific
    configurations.
  • Removed redundant capabilities field from HPA configurations.
  • +10/-9   
    Documentation
    CONFIGURATION.md
    Update documentation for custom Node capabilities               

    charts/selenium-grid/CONFIGURATION.md

  • Documented nodeCustomCapabilities configuration option.
  • Removed redundant capabilities field for HPA configurations.
  • +5/-4     

    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
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Data Validation

    The nodeCustomCapabilities value is passed directly to environment variables without any validation or sanitization of the JSON/string format, which could cause runtime issues if malformed.

    - name: SE_NODE_STEREOTYPE_EXTRA
      value: {{ $nodeCustomCapabilities | quote }}

    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 27, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix JSON string escaping

    The custom capabilities are hardcoded as a JSON string in the Helm command,
    which could lead to parsing errors. Use proper JSON escaping or a separate
    variable to define the capabilities.

    tests/charts/make/chart_test.sh [191-195]

     if [ "${TEST_CUSTOM_SPECIFIC_NAME}" = "true" ]; then
    +  CUSTOM_CAPS='{"myApp:version":"beta","myApp:publish":"public"}'
       HELM_COMMAND_SET_IMAGES="${HELM_COMMAND_SET_IMAGES} \
    -  --set global.seleniumGrid.nodeCustomCapabilities={'myApp:version':'beta','myApp:publish':'public'} \
    +  --set global.seleniumGrid.nodeCustomCapabilities=${CUSTOM_CAPS} \
       "
     fi
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The current hardcoded JSON string with single quotes could cause parsing issues in Helm. Using a separate variable with proper JSON formatting would make it more reliable and maintainable.

    Medium
    • Update

    Signed-off-by: Viet Nguyen Duc <[email protected]>
    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 27, 2025

    CI Feedback 🧐

    (Feedback updated until commit a7fe345)

    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, which requires proper authorization to
    access organizational data.

    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: 13574054823
    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: 13574054823
    127:  RERUN_FAILED_ONLY: true
    ...
    
    176:  0 upgraded, 0 newly installed, 0 to remove and 41 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: 13574054823
    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.
    

    @VietND96 VietND96 merged commit 7276e54 into trunk Feb 28, 2025
    27 checks passed
    @VietND96 VietND96 deleted the chart-custom-caps branch February 28, 2025 04:28
    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.

    1 participant