Skip to content

Conversation

@VietND96
Copy link
Member

@VietND96 VietND96 commented Jan 16, 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

Following the practices

Optimize domain name settings: You can specify the domain name that a client pod needs to access based on the following rules. These rules can help minimize the number of attempts to resolve the domain name and make the DNS resolution service more efficient.

If the client pod needs to access a Service in the same namespace, use <service-name> as the domain name. <service-name> indicates the name of the Service.

If the client pod needs to access a Service in another namespace, use <service-name>.<namespace-name> as the domain name. namespace-name specifies the namespace to which the Service belongs.

Fixes #2588

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


Description

  • Updated Kubernetes service names to remove namespace dependency.

  • Replaced hardcoded service URLs with configurable global variables.

  • Adjusted templates to use tpl for dynamic value resolution.

  • Enhanced flexibility for Kubernetes deployments with new global variables.


Changes walkthrough 📝

Relevant files
Tests
test.py
Adjust test for updated service URL format                             

tests/charts/templates/test.py

  • Updated test assertion to reflect new service URL format.
  • Removed namespace dependency in the test URL.
  • +1/-1     
    Enhancement
    _helpers.tpl
    Refactor service URL construction in helpers                         

    charts/selenium-grid/templates/_helpers.tpl

  • Removed namespace from service URL construction.
  • Simplified host URL definitions for components.
  • +2/-2     
    event-bus-configmap.yaml
    Use global variable for event bus host                                     

    charts/selenium-grid/templates/event-bus-configmap.yaml

  • Replaced hardcoded service URL with a configurable global variable.
  • Used tpl for dynamic value resolution.
  • +1/-1     
    node-configmap.yaml
    Use global variables for node configuration                           

    charts/selenium-grid/templates/node-configmap.yaml

  • Replaced hardcoded service URLs with configurable global variables.
  • Used tpl for dynamic value resolution.
  • +3/-3     
    secrets.yaml
    Use global variable for grid URL in secrets                           

    charts/selenium-grid/templates/secrets.yaml

  • Replaced hardcoded grid URL with a configurable global variable.
  • Used tpl for dynamic value resolution.
  • +1/-1     
    Configuration changes
    values.yaml
    Add global variables for service host configurations         

    charts/selenium-grid/values.yaml

  • Added new global variables for service host configurations.
  • Enhanced flexibility for Kubernetes deployments.
  • +5/-0     

    Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-merge-pro
    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

    Test Coverage

    The test only verifies the URL format for router component. Consider adding test cases for other components (distributor, event-bus, hub) to ensure their URLs are also constructed correctly without namespace dependency.

    self.assertTrue(decoded_url == f'https://{RELEASE_NAME}selenium-router:4444/selenium/graphql', decoded_url)

    @qodo-merge-pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Validate that critical URL configurations are not empty after template resolution

    Add validation to ensure that SE_NODE_GRID_URL is not empty after template
    resolution to prevent potential connectivity issues.

    charts/selenium-grid/templates/secrets.yaml [24]

    -SE_NODE_GRID_URL: {{ tpl (default (include "seleniumGrid.url" $) $.Values.global.seleniumGrid.SE_NODE_GRID_URL) $ | b64enc }}
    +{{- $gridUrl := tpl (default (include "seleniumGrid.url" $) $.Values.global.seleniumGrid.SE_NODE_GRID_URL) $ -}}
    +{{- if not $gridUrl }}
    +{{- fail "SE_NODE_GRID_URL cannot be empty after template resolution" -}}
    +{{- end -}}
    +SE_NODE_GRID_URL: {{ $gridUrl | b64enc }}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds crucial validation to prevent silent failures in Kubernetes deployments by ensuring the grid URL is not empty, which could cause connectivity issues in the Selenium Grid setup.

    8
    General
    Add error handling for base64 decoding operations to prevent test failures due to malformed input

    Add error handling for base64 decoding operation to prevent potential crashes if the
    URL is malformed or contains invalid base64 characters.

    tests/charts/templates/test.py [82]

    -decoded_url = base64.b64decode(base64_url).decode('utf-8')
    +try:
    +    decoded_url = base64.b64decode(base64_url).decode('utf-8')
    +except (base64.binascii.Error, UnicodeDecodeError) as e:
    +    self.fail(f"Failed to decode URL: {str(e)}")
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion adds important error handling for base64 decoding operations, which could prevent cryptic test failures and provide clearer error messages when invalid data is encountered.

    7

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 16, 2025

    CI Failure Feedback 🧐

    (Checks updated until commit f5da48f)

    Action: Rerun workflow when failure

    Failed stage: Authenticate GitHub CLI for PR [❌]

    Failure summary:

    The GitHub Action failed because the provided authentication token (GH_CLI_TOKEN_PR) lacks the
    required 'read:org' permission scope. The GitHub CLI (gh) command attempted to authenticate but
    couldn't proceed due to insufficient permissions.

    Key details:

  • Error occurred during the gh auth login command
  • Specific error: "missing required scope 'read:org'"
  • The action exited with code 1

  • 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: 12822689984
    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: 12822689984
    127:  RERUN_FAILED_ONLY: true
    ...
    
    169:  0 upgraded, 0 newly installed, 0 to remove and 73 not upgraded.
    170:  ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token
    171:  �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m
    172:  shell: /usr/bin/bash -e {0}
    173:  env:
    174:  GH_CLI_TOKEN: ***
    175:  GH_CLI_TOKEN_PR: ***
    176:  RUN_ID: 12822689984
    177:  RERUN_FAILED_ONLY: true
    178:  RUN_ATTEMPT: 1
    179:  ##[endgroup]
    180:  error validating token: missing required scope 'read:org'
    181:  ##[error]Process completed with exit code 1.
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    @VietND96 VietND96 force-pushed the svc-name branch 4 times, most recently from 7e85888 to f024a33 Compare January 17, 2025 02:14
    @VietND96 VietND96 changed the title K8s: Component service name in the same namespace K8s: Remove .svc.cluster.local from component host using service name Jan 17, 2025
    @VietND96 VietND96 merged commit 7cbd637 into trunk Jan 17, 2025
    33 of 35 checks passed
    @VietND96 VietND96 deleted the svc-name branch January 17, 2025 07:01
    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]: Could not determine if the address is IPv6 or IPv4

    1 participant