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

add kubernetes namespace labels #32

Merged
merged 5 commits into from
Nov 5, 2024
Merged

Conversation

nbucci
Copy link
Contributor

@nbucci nbucci commented Oct 31, 2024

PR Type

Enhancement, Documentation


Description

  • Added support for Kubernetes namespace labels in Drupal projects configuration
  • Removed ignore_changes block from kubernetes_namespace resource, allowing external label modifications to cause drift
  • Updated drupal_projects_list variable to include optional kubernetes_namespace_labels field
  • Removed helm values outputs
  • Updated CHANGELOG and README to reflect these changes
  • Bumped version to 0.13.0 with breaking changes noted

Changes walkthrough 📝

Relevant files
Enhancement
main.tf
Add Kubernetes namespace labels support                                   

main.tf

  • Added labels field to the namespace_list local variable
  • Updated kubernetes_namespace resource to include labels
  • Removed ignore_changes lifecycle block for namespace labels
  • +2/-6     
    variables.tf
    Update Drupal projects variable with namespace labels       

    variables.tf

  • Added kubernetes_namespace_labels optional field to the
    drupal_projects_list variable
  • +1/-0     
    Documentation
    CHANGELOG.md
    Update CHANGELOG with new version and breaking changes     

    CHANGELOG.md

  • Added entry for version 0.13.0
  • Documented new support for Kubernetes namespace labels
  • Noted breaking changes regarding removal of ignore_changes and helm
    values outputs
  • +14/-0   
    README.md
    Update README with new variable and removed outputs           

    README.md

  • Removed template provider from requirements
  • Updated drupal_projects_list input to include
    kubernetes_namespace_labels
  • Removed outputs for helm values
  • +1/-7     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @nbucci nbucci marked this pull request as draft October 31, 2024 10:40
    @nbucci nbucci requested a review from Monska85 October 31, 2024 10:40
    @sparkfabrik-ai-bot
    Copy link

    sparkfabrik-ai-bot bot commented Oct 31, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit 2ed0951)

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

    Potential Drift
    Removal of ignore_changes block for kubernetes_namespace resource may cause unintended drift if labels are modified externally.

    Default Value
    The new kubernetes_namespace_labels variable is set with an empty map as default. Consider if this is the intended behavior or if a set of default labels should be provided.

    @sparkfabrik-ai-bot
    Copy link

    sparkfabrik-ai-bot bot commented Oct 31, 2024

    PR Code Suggestions ✨

    Latest suggestions up to 2ed0951

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add validation for Kubernetes label format compliance

    Consider adding a validation block for the kubernetes_namespace_labels variable to
    ensure that label keys and values conform to Kubernetes naming conventions.

    variables.tf [48]

     kubernetes_namespace_labels          = optional(map(string), {})
    +validation {
    +  condition     = can([for k, v in var.kubernetes_namespace_labels : regex("^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$", k) && regex("^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?$", v)])
    +  error_message = "Kubernetes labels must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character."
    +}
     
    Suggestion importance[1-10]: 9

    Why: This suggestion adds important input validation, preventing potential issues with non-compliant Kubernetes labels.

    9
    Add a conditional check to avoid setting empty label sets

    Consider adding a conditional check to only set labels if they are not empty. This
    can prevent creating unnecessary empty label sets in Kubernetes.

    main.tf [89]

    -labels = each.value.labels
    +labels = length(each.value.labels) > 0 ? each.value.labels : null
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves resource efficiency and adheres to best practices, but it's not addressing a critical issue.

    7
    Possible issue
    Provide a default value for labels to prevent potential null errors

    Consider adding a default empty map for labels in case p.kubernetes_namespace_labels
    is null. This can prevent potential errors if the labels are not provided.

    main.tf [50]

    -labels    = p.kubernetes_namespace_labels
    +labels    = coalesce(p.kubernetes_namespace_labels, {})
     
    Suggestion importance[1-10]: 8

    Why: This suggestion addresses a potential runtime error, which is important for code stability and robustness.

    8
    Enhancement
    Simplify the namespace assignment using the coalesce function

    Consider using the coalesce function instead of the ternary operator for setting the
    namespace value. This can make the code more concise and easier to read.

    main.tf [49]

    -namespace = p.kubernetes_namespace == null ? "${p.project_name}-${p.gitlab_project_id}-${p.release_branch_name}" : p.kubernetes_namespace
    +namespace = coalesce(p.kubernetes_namespace, "${p.project_name}-${p.gitlab_project_id}-${p.release_branch_name}")
     
    Suggestion importance[1-10]: 6

    Why: The suggestion improves code readability and conciseness, but it's not addressing a critical issue or major bug.

    6

    Previous suggestions

    Suggestions up to commit 1854687
    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add validation for Kubernetes label format in the variable definition

    Consider adding a validation block to ensure that the kubernetes_namespace_labels
    map only contains valid Kubernetes label key-value pairs. Kubernetes has specific
    requirements for label keys and values.

    variables.tf [48]

     kubernetes_namespace_labels          = optional(map(string), {})
    +validation {
    +  condition     = can([for k, v in var.kubernetes_namespace_labels : regex("^([a-z0-9A-Z][-a-z0-9A-Z_.]*)?[a-z0-9A-Z]$", k) && length(k) <= 63 && regex("^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?$", v) && length(v) <= 63])
    +  error_message = "Kubernetes labels must be valid: keys and values must be 63 characters or less, begin and end with an alphanumeric character, and contain only alphanumerics, '-', '_' or '.'."
    +}
     
    Suggestion importance[1-10]: 9

    Why: Adding validation for Kubernetes label format is crucial for preventing errors and ensuring compliance with Kubernetes naming conventions, significantly improving code robustness.

    9
    Use a dynamic block to conditionally include labels in Kubernetes metadata

    Consider adding a conditional expression to only include the labels block if there
    are actually labels to apply. This can help avoid creating empty label blocks in
    Kubernetes.

    main.tf [87-89]

     metadata {
       name = each.value.namespace
    -  labels = each.value.labels
    +  dynamic "labels" {
    +    for_each = length(each.value.labels) > 0 ? [1] : []
    +    content {
    +      labels = each.value.labels
    +    }
    +  }
     }
     
    Suggestion importance[1-10]: 8

    Why: This suggestion enhances resource efficiency by avoiding empty label blocks, which is a valuable optimization for Kubernetes resources.

    8
    Enhancement
    Use the merge() function for combining default and provided labels

    Consider using the merge() function instead of the ternary operator for combining
    the default empty map with the provided labels. This approach is more idiomatic in
    Terraform and allows for easier extension in the future.

    main.tf [50]

    -labels    = p.kubernetes_namespace_labels == null ? {} : p.kubernetes_namespace_labels
    +labels    = merge({}, p.kubernetes_namespace_labels)
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code readability and maintainability by using a more idiomatic Terraform approach, though the functional difference is minimal.

    7

    main.tf Outdated Show resolved Hide resolved
    @nbucci nbucci force-pushed the platform/#3230-namespace-labels branch from 1854687 to f86f762 Compare October 31, 2024 10:56
    @nbucci nbucci force-pushed the platform/#3230-namespace-labels branch from 5ffede5 to a7c07af Compare November 4, 2024 14:58
    @nbucci nbucci force-pushed the platform/#3230-namespace-labels branch from 32cc242 to 4fa5303 Compare November 4, 2024 16:32
    CHANGELOG.md Outdated Show resolved Hide resolved
    CHANGELOG.md Outdated Show resolved Hide resolved
    CHANGELOG.md Outdated
    @@ -8,7 +8,19 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

    ## [Unreleased]

    ---
    ## [0.13.0] - 2024-11-04
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Suggested change
    ## [0.13.0] - 2024-11-04
    ## [0.13.0] - 2024-11-04
    [Compare with previous version](https://github.com/sparkfabrik/terraform-google-gcp-cloud-native-drupal-resources/compare/0.12.1...0.13.0)

    @nbucci nbucci force-pushed the platform/#3230-namespace-labels branch from 4fa5303 to ae226bb Compare November 4, 2024 16:42
    CHANGELOG.md Outdated Show resolved Hide resolved
    @nbucci nbucci force-pushed the platform/#3230-namespace-labels branch from ae226bb to 2ed0951 Compare November 5, 2024 15:15
    Copy link
    Contributor

    @Monska85 Monska85 left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM 👍

    @nbucci nbucci marked this pull request as ready for review November 5, 2024 16:50
    @sparkfabrik-ai-bot
    Copy link

    Persistent review updated to latest commit 2ed0951

    @nbucci nbucci merged commit 6d33ff1 into main Nov 5, 2024
    1 check 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