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

feat: added nodeSelector for keda patch pods #2618

Merged
merged 4 commits into from
Jan 30, 2025

Conversation

amardeep2006
Copy link
Contributor

@amardeep2006 amardeep2006 commented Jan 29, 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

This is to fix the issue #2614

Motivation and Context

Pass a way to pass the nodeSelector to patch object Job in helm chart.

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


Description

  • Added nodeSelector support for KEDA patch job pods.

  • Updated Helm chart configuration to include nodeSelector for patch jobs.

  • Documented the new nodeSelector option in the configuration guide.


Changes walkthrough 📝

Relevant files
Documentation
CONFIGURATION.md
Document `nodeSelector` option for patch jobs                       

charts/selenium-grid/CONFIGURATION.md

  • Added documentation for the new nodeSelector option.
  • Described its purpose for patch job pods.
  • +1/-0     
    Enhancement
    patch-keda-objects-job.yaml
    Add `nodeSelector` to KEDA patch job template                       

    charts/selenium-grid/templates/patch-keda/patch-keda-objects-job.yaml

  • Added nodeSelector configuration to the KEDA patch job template.
  • Ensured it uses the value from Helm chart configuration.
  • +2/-0     
    Configuration changes
    values.yaml
    Add `nodeSelector` field in values.yaml                                   

    charts/selenium-grid/values.yaml

  • Introduced nodeSelector field under autoscaling.patchObjectFinalizers.
  • Provided a placeholder for user-defined node selection.
  • +2/-0     

    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:

    🎫 Ticket compliance analysis ✅

    2614 - PR Code Verified

    Compliant requirements:

    • Add ability to pass nodeSelector to KEDA patch finalizer kubectl pods
    • Support specifying node type (e.g., amd64) for patch job pods

    Requires further human verification:

    • Fix the issue where pods are created on incorrect node architecture (needs testing in an environment with mixed node architectures)
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Default Value

    The nodeSelector is applied unconditionally without checking if it's empty, which might cause issues if no nodeSelector is provided

    nodeSelector:
      {{- toYaml $.Values.autoscaling.patchObjectFinalizers.nodeSelector | nindent 8 }}      

    Copy link

    qodo-merge-pro bot commented Jan 29, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    ✅ Prevent empty nodeSelector in spec
    Suggestion Impact:The commit implemented the suggestion by wrapping the nodeSelector block in a with condition to prevent empty nodeSelector fields

    code diff:

    -      nodeSelector:
    -        {{- toYaml $.Values.autoscaling.patchObjectFinalizers.nodeSelector | nindent 8 }}      
    +    {{- with .Values.autoscaling.patchObjectFinalizers.nodeSelector }}
    +      nodeSelector: {{- toYaml . | nindent 8 }}
    +    {{- end }}

    Add a conditional check around the nodeSelector block to prevent empty nodeSelector
    from being added to the job spec when no selectors are defined.

    charts/selenium-grid/templates/patch-keda/patch-keda-objects-job.yaml [48-49]

    -nodeSelector:
    -    {{- toYaml $.Values.autoscaling.patchObjectFinalizers.nodeSelector | nindent 8 }}
    +{{- with $.Values.autoscaling.patchObjectFinalizers.nodeSelector }}
    +  nodeSelector:
    +    {{- toYaml . | nindent 8 }}
    +{{- end }}
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves template robustness by preventing empty nodeSelector fields in the job spec when no selectors are defined. This is a good Helm chart practice that enhances maintainability and prevents potential issues with empty fields.

    7

    @VietND96 VietND96 force-pushed the feat/added-nodeselector branch from 525185a to 1ea164e Compare January 30, 2025 01:30
    @VietND96 VietND96 merged commit a7b58a4 into SeleniumHQ:trunk Jan 30, 2025
    24 of 26 checks passed
    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