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

fix: CSI driver so that we deploy a defaultStorageClass #196

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

venkatamutyala
Copy link
Contributor

@venkatamutyala venkatamutyala commented Dec 9, 2024

PR Type

enhancement


Description

  • Introduced a new configuration for deploying a default storage class in the CSI driver.
  • Merged existing node tolerations with the new default storage class settings to enhance the CSI configuration.

Changes walkthrough 📝

Relevant files
Enhancement
variables.tf
Add default storage class configuration for CSI driver     

variables.tf

  • Added a new local variable map_csi_default_storage to define default
    storage class settings.
  • Merged map_csi_addon_node_tolerations with map_csi_default_storage for
    CSI configuration.
  • +9/-3     

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

    Copy link
    Contributor

    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

    Configuration Validation
    The defaultStorageClass configuration is hardcoded to be enabled. Consider making this configurable via a variable to allow more flexibility.

    Type Safety
    The merge operation between node tolerations and storage class settings combines unrelated configuration types which could lead to unexpected behavior.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Avoid merging unrelated configuration maps that could lead to invalid CSI driver configuration

    The merge operation combines two maps with different structures - one for node
    tolerations and another for storage class settings. This will result in incorrect
    configuration. Use separate configurations for these distinct settings.

    variables.tf [37]

    -csi_addon_node_tolerations = jsonencode(merge(local.map_csi_addon_node_tolerations, local.map_csi_default_storage))
    +csi_addon_config = jsonencode({
    +  controller = local.nodeselector_and_pod_tolerations
    +  defaultStorageClass = {
    +    enabled = true
    +  }
    +})
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a critical issue where merging unrelated configuration maps (node tolerations and storage settings) could lead to invalid CSI driver configuration. The proposed solution properly separates these concerns into a more structured configuration object.

    9

    💡 Need additional feedback ? start a PR chat

    Copy link

    sonarqubecloud bot commented Dec 9, 2024

    @github-actions github-actions bot added enhancement New feature or request minor labels Dec 9, 2024
    @venkatamutyala venkatamutyala merged commit 77c7c1d into main Dec 9, 2024
    7 checks passed
    @venkatamutyala venkatamutyala deleted the fix/default-storageclass branch December 9, 2024 13:23
    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