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

Added support for nodeSelector in helm #25

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bharath97-git
Copy link

@bharath97-git bharath97-git commented Sep 25, 2024

PR Type

enhancement


Description

  • Added support for nodeSelector in the Helm chart for Kubernetes deployments.
  • The nodeSelector is conditionally included based on the presence of values in the Helm chart configuration.

Changes walkthrough 📝

Relevant files
Enhancement
statefulset.yaml
Add nodeSelector support to Helm chart                                     

charts/vespa/templates/statefulset.yaml

  • Added support for nodeSelector in the Helm chart.
  • Conditional inclusion of nodeSelector based on values.
  • +4/-0     

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

    @qodo-merge-pro qodo-merge-pro bot added the enhancement New feature or request label Sep 25, 2024
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Indentation
    The indentation of the new nodeSelector block seems to be misaligned with the rest of the YAML structure.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use the 'with' function instead of 'if' for better template readability and scoping

    Consider using the with function instead of if for better readability and to avoid
    potential issues with scope. This approach is more idiomatic in Helm templates.

    charts/vespa/templates/statefulset.yaml [58-60]

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

    Why: Using 'with' improves readability and scoping in Helm templates, making it a more idiomatic choice. This suggestion aligns well with best practices in Helm templating.

    8
    Enhancement
    Remove unnecessary '$' symbol when accessing Values in Helm templates

    Remove the $ from $.Values as it's not necessary in this context and might cause
    confusion. The .Values is sufficient to access the values.

    charts/vespa/templates/statefulset.yaml [58-60]

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

    Why: Removing the '$' symbol simplifies the code and reduces potential confusion, as '.Values' is sufficient for accessing values in this context.

    7
    Maintainability
    Add a comment to explain the purpose of the nodeSelector block

    Consider adding a comment explaining the purpose of the nodeSelector block. This
    will improve maintainability and make it easier for other developers to understand
    the template.

    charts/vespa/templates/statefulset.yaml [58-60]

    -{{- if $.Values.nodeSelector }}
    -nodeSelector: {{ toYaml $.Values.nodeSelector | nindent 8 }}
    +{{- /* Add nodeSelector if specified in values */}}
    +{{- if .Values.nodeSelector }}
    +nodeSelector: {{ toYaml .Values.nodeSelector | nindent 8 }}
     {{- end }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding a comment enhances maintainability by clarifying the purpose of the nodeSelector block, aiding future developers in understanding the template.

    6

    💡 Need additional feedback ? start a PR chat

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants