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: remove helm values output #34

Merged
merged 2 commits into from
Nov 4, 2024
Merged

feat: remove helm values output #34

merged 2 commits into from
Nov 4, 2024

Conversation

Stevesibilia
Copy link
Contributor

@Stevesibilia Stevesibilia commented Oct 31, 2024

PR Type

Enhancement


Description

  • Removed Helm values output generation for Drupal apps buckets and databases
  • Deleted helm_bucket.tpl and helm_database.tpl template files
  • Removed related data blocks in secrets.tf that were used to generate Helm values
  • Updated outputs.tf to remove Helm values outputs
  • Simplified the module by removing the Helm values generation logic

Changes walkthrough 📝

Relevant files
Enhancement
outputs.tf
Remove Helm values outputs                                                             

outputs.tf

  • Removed outputs for Drupal apps Helm values related to buckets and
    databases
  • +0/-12   
    secrets.tf
    Remove Helm values template generation                                     

    secrets.tf

  • Removed data blocks for generating Helm values templates for buckets
    and databases
  • +0/-26   
    helm_bucket.tpl
    Remove bucket Helm values template                                             

    files/template/helm_bucket.tpl

    • Deleted entire file containing Helm values template for buckets
    +0/-26   
    helm_database.tpl
    Remove database Helm values template                                         

    files/template/helm_database.tpl

    • Deleted entire file containing Helm values template for databases
    +0/-26   

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

    @sparkfabrik-ai-bot
    Copy link

    PR Reviewer Guide 🔍

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

    Removed Outputs
    The PR removes two outputs: drupal_apps_helm_values_for_buckets and drupal_apps_helm_values_for_databases. Ensure that these outputs are no longer needed by any dependent modules or scripts.

    Removed Data Blocks
    Two data "template_file" blocks have been removed: helm_values_for_buckets and helm_values_for_databases. Verify that these are no longer required and their removal doesn't break any existing functionality.

    @sparkfabrik-ai-bot
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Remove unused data source import

    Consider removing the unused template_file data source import if it's no longer
    needed after removing the helm values outputs.

    secrets.tf [7-10]

    +# Remove the following line if the template_file data source is no longer used
    +# terraform {
    +#   required_providers {
    +#     template = {
    +#       source = "hashicorp/template"
    +#     }
    +#   }
    +# }
    +
     locals {
       map_of_output_drupal_buckets = var.create_buckets == true ? {
         for o in module.drupal_buckets[*].buckets_access_credentials : o.name => o
       } : {}
       map_of_output_drupal_databases = trimspace(var.cloudsql_instance_name) != "" && trimspace(var.cloudsql_privileged_user_name) != "" && trimspace(var.cloudsql_privileged_user_password) != "" && var.create_databases_and_users == true ? {
         for o in module.drupal_databases_and_users[0].sql_users_creds : o.database => o
       } : {}
     }
     
    Suggestion importance[1-10]: 8

    Why: Removing unused imports is a best practice that improves code cleanliness and reduces potential confusion or errors.

    8
    Maintainability
    Add a comment explaining the removal of helm values outputs

    Consider adding a comment explaining why the helm values outputs were removed. This
    can help future maintainers understand the reasoning behind this change.

    outputs.tf [7-10]

    +# Helm values outputs for buckets and databases have been removed as they are no longer needed
    +# If you need to reintroduce them, refer to the previous version of this file
     output "details_of_used_tag_keys" {
       description = "Details of the tag keys passed to this module."
       value       = module.drupal_buckets[*].details_of_used_tag_keys
     }
     
    Suggestion importance[1-10]: 7

    Why: Adding a comment explaining the removal of outputs improves code maintainability and provides context for future developers.

    7

    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.

    Please remove the template from the required providers as suggested by the tflint. This old configuration is also present in the examples/main.tf file, remove the declaration also from there.

    In the examples/outputs.tf file are used the two remove outputs (helm_values_for_buckets and helm_values_for_databases); please remove this old configuration from the example.

    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 👍

    @Stevesibilia Stevesibilia merged commit 36f77b7 into main Nov 4, 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