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

3237 enhance output of module terraform google gcp cloud native drupal resources #45

Conversation

Stevesibilia
Copy link
Contributor

@Stevesibilia Stevesibilia commented Nov 28, 2024

PR Type

Enhancement


Description

  • Introduced a consolidated data structure (all_data) to efficiently manage Drupal project information
  • Added new output variables for improved project data accessibility:
    • Bucket and database credentials
    • Kubernetes secrets for buckets and databases
    • Project-specific namespaces
  • Implemented safer credential retrieval using try() function to handle potential null values
  • Updated documentation to reflect new output variables and their descriptions

Changes walkthrough 📝

Relevant files
Enhancement
outputs.tf
Enhance Drupal project outputs with consolidated data structure

outputs.tf

  • Added local variable all_data to aggregate project data including
    namespaces, credentials and secrets
  • Created maps for bucket and database secrets with secret names and
    namespaces
  • Added new outputs for Drupal project credentials, secrets, and
    namespaces
  • Implemented safer credential retrieval using try() function
  • +86/-0   
    Documentation
    README.md
    Document new Drupal project output variables                         

    README.md

  • Added documentation for new output variables related to Drupal project
    data
  • Updated outputs table with descriptions for bucket credentials,
    secrets, and namespaces
  • +6/-0     

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

    @sparkfabrik-ai-bot
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Sensitive information exposure:
    While most sensitive outputs are correctly marked with sensitive = true, the main drupal_all_data output (lines 42-45) contains both sensitive credentials and non-sensitive data but lacks the sensitive flag. This could potentially expose credentials if the output is logged or displayed.

    ⚡ Recommended focus areas for review

    Error Handling
    Multiple try() functions are used to handle potential null values, but there's no explicit error handling or logging if the retrievals fail. Consider adding error messages or fallback values.

    Code Duplication
    Similar try() patterns are repeated in bucket_secrets_map and database_secrets_map locals. Consider extracting common logic into a reusable function.

    Sensitive Data
    The namespace output is marked as sensitive but typically namespace names are not considered sensitive information. This may unnecessarily restrict access to non-sensitive data.

    @sparkfabrik-ai-bot
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Remove unnecessary sensitive flag from non-sensitive data output

    The sensitive outputs contain credentials and secrets, but the namespace output is
    also marked as sensitive which is unnecessary and could complicate debugging.

    outputs.tf [79-85]

     output "drupal_all_namespaces" {
       description = "Namespace for each Drupal project"
    -  sensitive   = true
       value = {
         for key, value in local.all_data : key => value.namespace
       }
     }
    Suggestion importance[1-10]: 8

    Why: Marking namespace output as sensitive is unnecessary since namespaces are not confidential information. Removing this flag improves debugging capabilities while maintaining proper security practices for truly sensitive data.

    8
    Best practice
    Improve error handling by providing meaningful default values instead of null

    Consider using a more robust error handling approach for the try() functions by
    providing a meaningful default value instead of null, which could help prevent
    potential issues downstream.

    outputs.tf [7]

    -bucket_credentials = try(module.drupal_buckets[0].buckets_access_credentials["${p.project_name}-${p.gitlab_project_id}-${p.release_branch_name}-drupal"], null)
    +bucket_credentials = try(module.drupal_buckets[0].buckets_access_credentials["${p.project_name}-${p.gitlab_project_id}-${p.release_branch_name}-drupal"], {})
    Suggestion importance[1-10]: 7

    Why: Using an empty map as a default value instead of null is a better practice as it can prevent potential null reference errors downstream and make the code more robust.

    7
    Maintainability
    Use string formatting function to improve code readability and maintainability

    The complex string concatenation for project keys could be simplified using format()
    function to improve readability and maintainability.

    outputs.tf [3]

    -for p in var.drupal_projects_list : "${p.project_name}-${p.gitlab_project_id}-${p.release_branch_name}" => {
    +for p in var.drupal_projects_list : format("%s-%s-%s", p.project_name, p.gitlab_project_id, p.release_branch_name) => {
    Suggestion importance[1-10]: 4

    Why: While using format() function is a valid alternative to string interpolation, the current string interpolation syntax is already clear and commonly used in Terraform. The suggested change offers only marginal improvement in readability.

    4

    README.md Outdated Show resolved Hide resolved
    outputs.tf Outdated Show resolved Hide resolved
    outputs.tf Outdated Show resolved Hide resolved
    Stevesibilia and others added 5 commits November 29, 2024 14:34
    Co-authored-by: Andrea Panisson <[email protected]>
    Co-authored-by: Andrea Panisson <[email protected]>
    Co-authored-by: Andrea Panisson <[email protected]>
    …tf for consistency with Drupal project naming
    @Stevesibilia Stevesibilia merged commit 23d239f into main Nov 29, 2024
    1 check passed
    @Stevesibilia Stevesibilia deleted the 3237-enhance-output-of-module-terraform-google-gcp-cloud-native-drupal-resources branch November 29, 2024 13:46
    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