Support use in root and child modules #5
Conversation
WalkthroughThis pull request includes several changes across multiple files, primarily focused on updating documentation and Terraform configurations. The Changes
Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
tests/fixtures/default/outputs.tf (1)
Line range hint
1-21: Consider documenting the module usage patterns.The rename from
core_workspacetocore_helpers_rootsuggests this module provides helper functionality at the root level. Consider adding documentation that explains:
- The distinction between root and child module usage
- When to use this helper module vs. potential child-specific helpers
- Examples of typical usage patterns in both contexts
root/locals.tf (2)
5-16: Consider adding variable validation for required labels.While the labels map is well-structured, some critical tags might be required for your infrastructure. Consider adding variable validation blocks for required input variables to ensure they're not null or empty.
Example validation:
variable "cost_center" { type = string description = "Cost center for resource tagging" validation { condition = length(var.cost_center) > 0 error_message = "Cost center must not be empty." } }
10-14: Standardize naming convention between label keys and variables.The label keys use hyphens (e.g.,
cost-center) while the corresponding variables use underscores (e.g.,var.cost_center). Consider standardizing the naming convention for better maintainability.Example standardization:
labels = { - cost-center = var.cost_center - data-classification = var.data_classification + cost_center = var.cost_center + data_classification = var.data_classification region = local.region - repository = var.repository + repository = var.repository team = var.teamchild/helpers.tf (2)
Line range hint
3-52: Consider enhancing workspace name validation and error handlingThe workspace parsing logic is well-documented and structured, but could benefit from some improvements:
- The region regex
^(us-[a-z]+\\d+)assumes only US regions. Consider making it more generic to support other regions.- The try() function silently returns null on regex failures. Consider adding explicit validation with more descriptive error messages.
Example improvement for better error handling:
parsed_workspace = ( local.workspace == "default" ? { environment = "mock-environment" region = "mock-region" zone = "mock-zone" } : { - environment = try(regex(local.environment_regex, local.workspace)[0], null) + environment = ( + can(regex(local.environment_regex, local.workspace)) ? + regex(local.environment_regex, local.workspace)[0] : + terraform.workspace == "default" ? null : + fail("Invalid workspace name: environment must match ${local.environment_regex}") + ) # Similar changes for region and zone } )
Line range hint
75-80: Add validation to prevent production usage of test-only variableWhile the variable description clearly indicates it's for testing only, consider adding validation rules to prevent accidental use in production.
variable "workspace" { description = "This is used for tests to set the workspace name. Do not set this variable in any other context" type = string default = null + validation { + condition = var.workspace == null || can(regex("^test-", var.workspace)) + error_message = "The workspace variable is for testing only and must start with 'test-' if set." + } }root/helpers.tf (1)
Line range hint
17-45: Consider making mock values configurableThe workspace parsing logic is robust and handles both default and structured workspace names well. However, the mock values used for testing are hardcoded. Consider making these configurable for better testing flexibility.
parsed_workspace = ( local.workspace == "default" ? { - environment = "mock-environment" - region = "mock-region" - zone = "mock-zone" + environment = try(var.mock_environment, "mock-environment") + region = try(var.mock_region, "mock-region") + zone = try(var.mock_zone, "mock-zone") } : {Add these variable declarations:
variable "mock_environment" { description = "Mock environment value for testing" type = string default = null } variable "mock_region" { description = "Mock region value for testing" type = string default = null } variable "mock_zone" { description = "Mock zone value for testing" type = string default = null }shared/helpers.tf (3)
Line range hint
15-17: Consider making regex patterns more restrictiveThe environment regex could be more specific to only match exact environment names.
- environment_regex = "-(non-production|sandbox|production)$" + environment_regex = "-(non-production|sandbox|production)$" + valid_environments = toset(["non-production", "sandbox", "production"])Then add validation:
validate_environment = ( local.environment != null && !contains(local.valid_environments, local.environment) ? fail("Invalid environment ${local.environment}. Must be one of: ${join(", ", local.valid_environments)}") : true )
Line range hint
30-49: Add defensive programming for workspace parsingThe current implementation could silently return null for malformed workspace names. Consider adding explicit validation.
parsed_workspace = ( local.workspace == "default" ? { environment = "mock-environment" region = "mock-region" zone = "mock-zone" } : + can(regex("^[a-z][-a-z0-9]*(-(non-production|sandbox|production))$", local.workspace)) ? { environment = try(regex(local.environment_regex, local.workspace)[0], null) region = try(regex(local.region_regex, local.workspace)[0], null) zone = try(regex(local.zone_regex, local.workspace)[0], null) - } + } : + fail("Invalid workspace name format: ${local.workspace}") )
51-69: Consider adding output value validationWhile the outputs are well documented, consider adding validation to ensure non-null values when required.
output "environment" { description = "The environment name for example production, non-production, sandbox" + precondition { + condition = local.environment != null + error_message = "Environment could not be determined from workspace name" + } value = local.environment }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
.pre-commit-config.yaml(1 hunks)README.md(1 hunks)child/README.md(1 hunks)child/helpers.tf(1 hunks)empty.tf(1 hunks)root/README.md(1 hunks)root/helpers.tf(1 hunks)root/locals.tf(1 hunks)root/outputs.tf(0 hunks)root/variables.tf(0 hunks)shared/helpers.tf(2 hunks)tests/default.tftest.hcl(0 hunks)tests/fixtures/default/main.tf(1 hunks)tests/fixtures/default/outputs.tf(1 hunks)tests/fixtures/default/variables.tf(0 hunks)
💤 Files with no reviewable changes (4)
- root/outputs.tf
- root/variables.tf
- tests/default.tftest.hcl
- tests/fixtures/default/variables.tf
✅ Files skipped from review due to trivial changes (5)
- .pre-commit-config.yaml
- README.md
- child/README.md
- empty.tf
- root/README.md
🧰 Additional context used
📓 Learnings (1)
root/helpers.tf (1)
Learnt from: brettcurtis
PR: osinfra-io/terraform-kubernetes-istio#23
File: regional/locals.tf:7-11
Timestamp: 2024-11-12T12:23:57.379Z
Learning: In `regional/locals.tf`, unexpected workspace names are handled elsewhere in the code, so regex failures in `environment`, `region`, and `zone` locals are acceptable.
🔇 Additional comments (11)
tests/fixtures/default/main.tf (2)
1-2: LGTM! Clear module naming and explicit source path.
The rename to "core_helpers_root" and explicit source path "../../../root" better reflect the module's role and structure in the root/child pattern.
Line range hint 1-8: Verify root/child module structure exists.
Let's confirm the repository structure supports this root/child module pattern.
✅ Verification successful
Module structure verified successfully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and structure of root and child modules
# Check if root module directory exists and contains expected files
echo "Checking root module structure..."
fd -t f . root/ -x echo "Found root module file: {}"
# Look for other potential child modules
echo -e "\nChecking for child modules..."
fd -t d . -d 1 -E root -E tests -E .git -x echo "Found potential child module: {}"
# Verify module structure documentation
echo -e "\nChecking for module documentation..."
fd -g "README.md" . root/ -x echo "Found documentation: {}"
Length of output: 823
tests/fixtures/default/outputs.tf (1)
5-21: Module reference update looks consistent.
The change from core_workspace to core_helpers_root is applied consistently across all outputs and appears to better reflect the module's purpose in the root context.
Let's verify the consistency of this module reference across other test files:
✅ Verification successful
Module reference is consistently updated across all test fixtures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent module naming across test fixtures
# Expected: All references should use core_helpers_root instead of core_workspace
# Check for any remaining references to the old module name
echo "Checking for old module references..."
rg "module\.core_workspace\." tests/
# Verify the new module name usage
echo "Verifying new module references..."
rg "module\.core_helpers_root\." tests/
# Check module source declarations
echo "Checking module source declarations..."
rg 'module "core_helpers_root|module "core_workspace' tests/
Length of output: 834
root/locals.tf (1)
1-3: LGTM! Well-documented with official reference.
The documentation clearly indicates the purpose of the file and includes a link to the official Terraform documentation for local values.
child/helpers.tf (2)
1-1:
Fix invalid file reference syntax
The current syntax for referencing ../shared/helpers.tf is not valid Terraform code. To share code between modules, consider these approaches:
- Use Terraform's native
sourceargument in a module block - Move shared code to a separate module
- Use proper relative paths with terraform_remote_state data source if needed
Remove this line as it's not valid Terraform syntax:
-../shared/helpers.tfLikely invalid or redundant comment.
Line range hint 54-73: Verify consistent output usage across modules
The outputs are well-defined and documented. Let's verify they're being used consistently across parent and child modules.
✅ Verification successful
Generate additional scripts to verify variable references across modules
Outputs are appropriately used only in test modules. No inconsistent usage found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to these outputs in other modules
echo "Checking output references in other modules..."
rg -t tf "output\s+\"(env|environment|region|zone)\"" --type tf
echo "Checking variable references that might consume these outputs..."
rg -t tf "var\.(env|environment|region|zone)\b" --type tf
Length of output: 717
Script:
#!/bin/bash
# Search for module-based references to the outputs
echo "Checking for module-based references to outputs..."
rg -t tf 'module\.[a-zA-Z0-9_-]+\.(env|environment|region|zone)' --type tf
# Search for outputs being used in expressions or assignments
echo "Checking for outputs used in expressions or assignments..."
rg -t tf '(env|environment|region|zone)\s*=' --type tf
Length of output: 2112
root/helpers.tf (4)
Line range hint 4-11: Well-documented workspace naming convention
The documentation clearly explains the workspace naming patterns with comprehensive examples covering various scenarios.
Line range hint 47-71: LGTM! Well-documented outputs and variables
The output and variable declarations are clear, well-documented, and align perfectly with their intended usage. The workspace variable is properly scoped for testing purposes only.
1-1:
Remove or clarify the path reference
The line ../shared/helpers.tf appears to be a leftover comment or incorrect include statement. This could cause confusion about the relationship between this file and the referenced path.
-../shared/helpers.tfLikely invalid or redundant comment.
Line range hint 13-15: Verify regex patterns against all possible workspace names
The regex patterns align with the documented examples. Based on the retrieved learning, regex failures are handled appropriately elsewhere in the code.
✅ Verification successful
Regex patterns are correctly defined and no issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify workspace naming patterns across the codebase
# Look for workspace references that might not match the expected patterns
# Search for workspace name assignments or references
rg -g '!*.md' -g '!*.txt' '(workspace|workspace_name)\s*=\s*["'\'']([^"'\'']+)["'\'']' --json | jq -r '.data.submatches[1].match' | sort -u
Length of output: 146
shared/helpers.tf (1)
Line range hint 1-13: Documentation is clear and helpful!
The examples and comments effectively explain the workspace naming convention and its components.
Summary by CodeRabbit
New Features
Bug Fixes
checkovrepository version for improved static analysis of infrastructure code.Documentation
Chores