Conversation
📝 WalkthroughWalkthroughThe PR tightens Terraform token regexes in engines/terraform/token.go to require the first character after the prefix be a letter, underscore, or dash, with subsequent characters limited to letters, digits, underscores, dashes, or dots. Tests in engines/terraform/resolve_test.go were updated: TestResolveStringInterpolation now accepts a setupGlobalVars map and an expectedTokens slice, populates td.terraformVariables from setupGlobalVars, extracts token strings from TokenMatch results and asserts token sequences, and adds "string with coalesce function" and "string with ternary operator" scenarios. Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
engines/terraform/token.go (1)
48-49: Approve regex changes. Stricter pattern validated against existing usage; no broken tokens detected. Consider consolidating duplicatetokenPatternandallTokensPattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
engines/terraform/resolve_test.go(1 hunks)engines/terraform/token.go(1 hunks)
🔇 Additional comments (1)
engines/terraform/resolve_test.go (1)
449-457: LGTM! Test validates coalesce with multiple tokens.The test case correctly exercises token interpolation with Terraform's
coalescefunction and ensures the stricter regex still handles this use case.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
engines/terraform/token.go (1)
48-49: Unify identical regex compilation
tokenPatternandallTokensPatternboth compile the same pattern—use a single package-level regex for both single- and multi-match use cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
engines/terraform/resolve_test.go(6 hunks)engines/terraform/token.go(1 hunks)
🔇 Additional comments (5)
engines/terraform/resolve_test.go (4)
399-405: LGTM! Test struct improvements enhance validation.The addition of
setupGlobalVarsandexpectedTokensfields makes the tests more comprehensive by explicitly validating token extraction alongside resolution.
456-466: Good addition: coalesce function test case.This test validates that tokens can be extracted from within function calls like
coalesce(self.project_id, var.project_id), ensuring the regex handles complex interpolation scenarios.
480-487: LGTM! Global variables setup is correct.The nil-check and initialization of
td.terraformVariablesbefore populating fromsetupGlobalVarsprevents nil pointer dereferences.
498-504: LGTM! Token extraction and assertion logic is sound.The extraction of token strings from
TokenMatchresults and comparison againstexpectedTokensprovides clear validation that the regex patterns are working as intended.engines/terraform/token.go (1)
48-49: Tightened regex is safe: no tokens start with digits or dots after prefix.
sean-nitric
left a comment
sean-nitric
left a comment
There was a problem hiding this comment.
Would also be cool to have a ternary test case
${self.project_id == null ? self.project_id : var.project_id}
raksiv
left a comment
raksiv
left a comment
There was a problem hiding this comment.
Looks good - The regex fix doesn't change how ternaries work, so probably not required unless you're wanting to add tests outside the scope of the change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
engines/terraform/token.go (1)
48-49: Consider consolidating duplicate regex patterns.Both
tokenPatternandallTokensPatternare identical. If they're meant to remain the same, consider using a single compiled pattern or adding a comment explaining why they're separate.Apply this diff if consolidation is desired:
+var tokenPatternRegex = regexp.MustCompile(`((?:infra|var|self)\.[a-zA-Z_\-][a-zA-Z0-9_\-\.]*)`) -var tokenPattern = regexp.MustCompile(`((?:infra|var|self)\.[a-zA-Z_\-][a-zA-Z0-9_\-\.]*)`) -var allTokensPattern = regexp.MustCompile(`((?:infra|var|self)\.[a-zA-Z_\-][a-zA-Z0-9_\-\.]*)`) +var tokenPattern = tokenPatternRegex +var allTokensPattern = tokenPatternRegex
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
engines/terraform/resolve_test.go(6 hunks)engines/terraform/token.go(1 hunks)
🔇 Additional comments (9)
engines/terraform/token.go (2)
48-49: Regex logic correctly implements tightened token requirements.The patterns correctly enforce that the first character after the prefix must be a letter, underscore, or dash, with subsequent characters limited to letters, digits, underscores, dashes, or dots. This prevents tokens like
self.123varorself..nested.Note: The escaping of
-and.inside character classes (\-and\.) is unnecessary but harmless—inside brackets,.is literal by default, and-at the end of a class is unambiguous.
48-49: Regex validation tightened and verified
The updated patterns enforce stricter token rules without breaking existing usage; tests for invalid tokens (e.g.,self.,var.) pass as expected.engines/terraform/resolve_test.go (7)
399-405: Test structure improvements enhance validation precision.The addition of
expectedTokensandsetupGlobalVarsfields allows more precise validation of token extraction behavior and enables testing of bothself.*andvar.*references in the same test case.
455-478: Valuable test coverage for complex expressions.The new test cases for coalesce functions and ternary operators validate that the tightened regex correctly extracts tokens from real-world Terraform expressions. These scenarios are critical for ensuring the regex changes don't break legitimate use cases.
508-516: Token sequence validation improves test accuracy.Extracting and comparing token strings directly from
TokenMatchresults ensures the regex not only finds tokens but finds them in the correct order and with the correct values. This catches subtle ordering or extraction bugs.
399-405: LGTM!Test structure correctly updated to support token sequence validation and global variable setup.
455-478: LGTM!New test cases for coalesce functions and ternary operators provide valuable coverage for complex token extraction scenarios.
492-499: LGTM!Global variable setup correctly enables testing of
var.*references in the new test cases.
508-517: LGTM!Token extraction assertion ensures the regex changes produce the expected token sequences.
Added the test case anyway as it doesn't hurt to have and will catch potential regressions if we need to refine this behaviour further. |
|
🎉 This PR is included in version 0.1.23 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
7 participants
No description provided.