Skip to content

K8s: Bump new KEDA image for feature preview #2681

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 5 commits into from
Feb 27, 2025
Merged

K8s: Bump new KEDA image for feature preview #2681

merged 5 commits into from
Feb 27, 2025

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Feb 26, 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, Tests, Documentation


Description

  • Added support for enableManagedDownloads in Selenium Grid scaler.

    • Introduced new metadata field enableManagedDownloads for scaler configuration.
    • Updated logic to handle managed downloads in scaler and capabilities parsing.
  • Enhanced test coverage for enableManagedDownloads functionality.

    • Added new test cases to validate managed downloads and capability handling.
  • Updated documentation for Selenium Grid scaler.

    • Added examples and explanations for enableManagedDownloads.
  • Minor configuration and dependency updates.

    • Updated KEDA_BASED_TAG in Makefile and fixed a condition in Helm chart dependencies.

Changes walkthrough 📝

Relevant files
Enhancement
selenium_grid_scaler.go
Support managed downloads in Selenium Grid scaler               

.keda/scalers/selenium_grid_scaler.go

  • Added enableManagedDownloads field to scaler metadata.
  • Updated logic to parse and handle enableManagedDownloads.
  • Refactored capability parsing into a separate function.
  • Adjusted getCountFromSeleniumResponse to include
    enableManagedDownloads.
  • +35/-22 
    Tests
    selenium_grid_scaler_test.go
    Add tests for managed downloads in scaler                               

    .keda/scalers/selenium_grid_scaler_test.go

  • Added test cases for enableManagedDownloads.
  • Updated existing tests to include enableManagedDownloads parameter.
  • Enhanced test coverage for capability parsing and session handling.
  • +85/-28 
    Documentation
    README.md
    Update README with new PR references                                         

    .keda/README.md

  • Added references to new PRs in the KEDA project.
  • Included documentation links for managed downloads.
  • +4/-0     
    selenium-grid-scaler.md
    Document managed downloads in Selenium Grid scaler             

    .keda/scalers/selenium-grid-scaler.md

  • Documented enableManagedDownloads metadata field.
  • Added examples for configuring managed downloads in scaler.
  • Explained Node configuration for managed downloads.
  • +65/-0   
    Configuration changes
    Makefile
    Update KEDA_BASED_TAG in Makefile                                               

    Makefile

    • Updated KEDA_BASED_TAG to a new version.
    +1/-1     
    Chart.yaml
    Fix Helm chart dependency condition                                           

    charts/selenium-grid/Chart.yaml

    • Fixed condition for kube-prometheus-stack dependency.
    +1/-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 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The error from parseCapabilitiesToMap() is logged but ignored, which could lead to unexpected behavior if capabilities parsing fails

    capabilities, err := parseCapabilitiesToMap(_capabilities)
    if err != nil {
    	logger.Error(err, fmt.Sprintf("Error when unmarshaling trigger metadata 'capabilities': %s", err))
    } else if enableManagedDownloads {
    	capabilities[EnableManagedDownloadsCapability] = true
    }

    Configuration Issue
    The condition for kube-prometheus-stack dependency was updated but the old condition name 'prometheus-stack.enabled' may still be referenced elsewhere

    Copy link

    qodo-merge-pro bot commented Feb 26, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Prevent nil pointer on parse error

    The error handling in parseCapabilitiesToMap should propagate up to prevent
    potential nil pointer dereference. When capabilities parsing fails, the error
    should be returned to the caller.

    .keda/scalers/selenium_grid_scaler.go [397-402]

     capabilities, err := parseCapabilitiesToMap(_capabilities)
     if err != nil {
         logger.Error(err, fmt.Sprintf("Error when unmarshaling trigger metadata 'capabilities': %s", err))
    -} else if enableManagedDownloads {
    +    return 0, 0, err
    +}
    +if enableManagedDownloads {
         capabilities[EnableManagedDownloadsCapability] = true
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion addresses a critical error handling issue that could lead to a nil pointer dereference. Propagating the error up is essential for proper error handling and system stability.

    Medium
    General
    Simplify single-value capability prefix

    The FunctionCapabilitiesPrefixes slice contains only one constant value.
    Consider using a single constant instead of a slice for better code clarity and
    performance.

    .keda/scalers/selenium_grid_scaler.go [108]

    -var FunctionCapabilitiesPrefixes = []string{EnableManagedDownloadsCapability}
    +const FunctionCapabilityPrefix = EnableManagedDownloadsCapability
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    __

    Why: While valid, this suggestion offers only a minor improvement in code clarity and performance since the slice contains just one value. The current implementation remains functional and maintainable.

    Low
    • Update

    Copy link

    qodo-merge-pro bot commented Feb 26, 2025

    CI Feedback 🧐

    (Feedback updated until commit 2af207a)

    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. Specifically:

  • The workflow attempted to authenticate using gh auth login --with-token
  • The provided token (GH_CLI_TOKEN_PR) was invalid due to missing the 'read:org' scope
  • This resulted in an authentication error and the workflow failed with exit 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: 13555937175
    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: 13555937175
    127:  RERUN_FAILED_ONLY: true
    ...
    
    166:  0 upgraded, 0 newly installed, 0 to remove and 35 not upgraded.
    167:  ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token
    168:  �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m
    169:  shell: /usr/bin/bash -e {0}
    170:  env:
    171:  GH_CLI_TOKEN: ***
    172:  GH_CLI_TOKEN_PR: ***
    173:  RUN_ID: 13555937175
    174:  RERUN_FAILED_ONLY: true
    175:  RUN_ATTEMPT: 1
    176:  ##[endgroup]
    177:  error validating token: missing required scope 'read:org'
    178:  ##[error]Process completed with exit code 1.
    

    @VietND96 VietND96 merged commit 1b42465 into trunk Feb 27, 2025
    1 check passed
    @VietND96 VietND96 deleted the keda-preview branch February 27, 2025 04:36
    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.

    2 participants