Skip to content
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

K8s: Use KEDA patch image tag for scaler implementation preview #2477

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Nov 26, 2024

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

Add back KEDA patch image tag in Helm configs

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, configuration changes


Description

  • Changed the default value of TEST_PATCHED_KEDA to "true" in the test script.
  • Enhanced the Helm template to include a condition for metricType when scalingType is "deployment".
  • Updated KEDA and KEDA-docs pull request references in the README.
  • Modified the Makefile to update KEDA tag versions and added new release targets.
  • Clarified default values for useCachedMetrics and metricType in the configuration documentation.
  • Added configuration for specifying KEDA component images in the Helm values.

Changes walkthrough 📝

Relevant files
Configuration changes
chart_test.sh
Update default KEDA test configuration                                     

tests/charts/make/chart_test.sh

  • Changed TEST_PATCHED_KEDA default value to "true".
+1/-1     
Makefile
Update KEDA tag versions and release targets                         

Makefile

  • Updated KEDA tag versions and added new release targets.
+5/-5     
values.yaml
Update autoscaling comments and KEDA image configuration 

charts/selenium-grid/values.yaml

  • Clarified comments on useCachedMetrics and metricType.
  • Added configuration for KEDA component images.
  • +17/-3   
    Enhancement
    _helpers.tpl
    Enhance scalingType condition with metricType check           

    charts/selenium-grid/templates/_helpers.tpl

  • Added condition to check metricType when scalingType is "deployment".
  • +1/-1     
    Documentation
    README.md
    Update KEDA documentation links                                                   

    .keda/README.md

    • Updated KEDA and KEDA-docs pull request references.
    +6/-2     
    CONFIGURATION.md
    Clarify autoscaling defaults and specify KEDA images         

    charts/selenium-grid/CONFIGURATION.md

  • Clarified default values for useCachedMetrics and metricType.
  • Added KEDA image specification details.
  • +3/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Logic Change
    The metricType field is now conditionally included only when scalingType is "deployment" and metricType is set. This changes the previous behavior where metricType was always included. Verify this change doesn't break existing deployments.

    Version Mismatch
    The KEDA_TAG_PREV_VERSION was changed from 2.16.0 to 2.15.1 which seems like a downgrade. Verify this version change is intentional and compatible with the system.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Add validation check for required images before executing release operations

    The release targets should verify the existence of grid scaler images before
    pushing, similar to how other images are verified.

    Makefile [480]

    -release: tag_major_minor release_grid_scaler
    +release: tag_major_minor
    +    @if ! docker images $(NAME)/grid-scaler | awk '{ print $$2 }' | grep -q -F $(TAG_VERSION); then echo "$(NAME)/grid-scaler version $(TAG_VERSION) is not yet built. Please run 'make build'"; false; fi
    +    $(MAKE) release_grid_scaler
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds crucial validation to ensure grid-scaler images exist before release, consistent with existing image checks. This prevents incomplete or failed releases due to missing dependencies.

    8
    Possible issue
    Add validation to ensure metric type is not an empty string before including it in the configuration

    The condition should check if metricType is not empty before including it in the
    output, as an empty string could cause validation issues in KEDA.

    charts/selenium-grid/templates/_helpers.tpl [252-254]

    -{{- if and (eq $.Values.autoscaling.scalingType "deployment") $.Values.autoscaling.metricType }}
    +{{- if and (eq $.Values.autoscaling.scalingType "deployment") (ne $.Values.autoscaling.metricType "") }}
       metricType: {{ $.Values.autoscaling.metricType }}
     {{- end }}
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion adds important validation to prevent potential KEDA configuration issues by ensuring metricType is not empty. This improves robustness and helps prevent runtime errors.

    7

    💡 Need additional feedback ? start a PR chat

    @VietND96 VietND96 merged commit 7727b6d into trunk Nov 26, 2024
    48 of 52 checks passed
    @VietND96 VietND96 deleted the keda-2.16-patch branch November 26, 2024 23:26
    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