Skip to content

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jan 17, 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 #2571

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


Description

  • Fixed a Helm chart template issue for relay nodes.

  • Updated dependencies in the Helm chart configuration.

  • Added test cases to validate relay node configurations.

  • Incremented chart version to 0.38.5 with dependency updates.


Changes walkthrough 📝

Relevant files
Documentation
CONFIGURATION.md
Update chart version and dependencies in documentation     

charts/selenium-grid/CONFIGURATION.md

  • Updated chart version to 0.38.5.
  • Updated dependencies for Redis, Jaeger, and Prometheus.
  • +4/-4     
    Configuration changes
    Chart.yaml
    Update chart version and dependency versions                         

    charts/selenium-grid/Chart.yaml

  • Incremented chart version to 0.38.5.
  • Updated versions for Redis, Jaeger, and Prometheus dependencies.
  • +4/-4     
    Bug fix
    relay-node-deployment.yaml
    Fix relay node deployment template logic                                 

    charts/selenium-grid/templates/relay-node-deployment.yaml

  • Fixed template logic for relay node uploader configuration.
  • Resolved nil pointer issue in relay node deployment.
  • +1/-1     
    Tests
    dummy.yaml
    Add relay node configuration for tests                                     

    tests/charts/templates/render/dummy.yaml

  • Added relay node configuration for testing.
  • Enabled relay node in dummy test configuration.
  • +3/-0     
    dummy_solution.yaml
    Include relay node in solution test configurations             

    tests/charts/templates/render/dummy_solution.yaml

  • Included relay node configuration in solution tests.
  • Enabled relay node for validation in test solutions.
  • +3/-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.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    2571 - PR Code Verified

    Compliant requirements:

    • Fix nil pointer error when evaluating videoRecorder in relay-node-deployment.yaml template

    Requires further human verification:

    • Fix Helm chart breaking when running only Selenium Hub and Relay Nodes (needs testing with actual Hub + Relay node setup)
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Logic Validation

    Verify that the new uploader configuration logic correctly handles all possible videoRecorder configurations, including when videoRecorder or uploader values are not set

    {{- $_ =  set $podScope "uploader" (get $.Values.videoRecorder ($podScope.recorder.uploader.name | toString)) -}}

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add nil check for uploader configuration to prevent template rendering failures

    The uploader configuration lookup should use $podScope.recorder.uploader.name
    instead of directly accessing .Values.videoRecorder.uploader.name to ensure
    consistent configuration inheritance and prevent nil pointer errors.

    charts/selenium-grid/templates/relay-node-deployment.yaml [36]

    -{{- $_ =  set $podScope "uploader" (get $.Values.videoRecorder ($podScope.recorder.uploader.name | toString)) -}}
    +{{- if $podScope.recorder.uploader -}}
    +{{- $_ = set $podScope "uploader" (get $.Values.videoRecorder ($podScope.recorder.uploader.name | toString)) -}}
    +{{- end -}}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds a crucial nil check that prevents template rendering failures when the uploader configuration is not defined, addressing a potential runtime error that could break the helm chart deployment.

    8

    @VietND96 VietND96 merged commit a195077 into trunk Jan 18, 2025
    18 of 19 checks passed
    @VietND96 VietND96 deleted the chart-fix branch January 18, 2025 14:19
    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]: Helm Chart not working with only Hub + Relay node
    1 participant