fix(engines): update token parsing to use prefixes#110
Conversation
📝 WalkthroughWalkthroughToken parsing now accepts unwrapped prefixed tokens (e.g., Possibly related PRs
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
engines/terraform/resolve.go(3 hunks)engines/terraform/token.go(2 hunks)
🔇 Additional comments (5)
engines/terraform/resolve.go (4)
5-6: LGTM!Standard library imports for iterator operations are appropriate here.
16-16: LGTM!Including the token in error messages improves debuggability.
Also applies to: 21-21, 27-27
97-97: LGTM!Computing available variables for error context is helpful.
101-101: LGTM!Enhanced error message with available variables list improves user experience.
engines/terraform/token.go (1)
15-15: LGTM!Comment accurately reflects the new behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
engines/terraform/resolve.go (1)
97-100: MoveavailableVarscomputation into the error path.Line 97 computes
availableVarsunconditionally, but it's only used in the error case at line 100. Moving it inside the!okblock avoids unnecessary work in the success path.Apply this diff:
- availableVars := slices.Collect(maps.Keys(td.instancedTerraformVariables[intentName])) tfVariable, ok := td.instancedTerraformVariables[intentName][varName] if !ok { + availableVars := slices.Collect(maps.Keys(td.instancedTerraformVariables[intentName])) return nil, fmt.Errorf("variable %s does not exist for provided blueprint available variables are: %v", varName, availableVars) }Optionally, improve the error message readability:
- return nil, fmt.Errorf("variable %s does not exist for provided blueprint available variables are: %v", varName, availableVars) + return nil, fmt.Errorf("variable %s does not exist for provided blueprint. Available variables: %v", varName, availableVars)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
engines/terraform/resolve.go(3 hunks)
🔇 Additional comments (2)
engines/terraform/resolve.go (2)
5-6: LGTM!The new imports support the available variables listing feature. Usage at line 97 is correct.
16-16: Improved error messages.Including the offending token in error messages significantly improves debuggability.
Also applies to: 21-21, 27-27
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engines/terraform/resolve_test.go (1)
397-483: Add output value assertions in interpolation tests.The string interpolation tests at lines 469-480 only verify that the result is not empty (
assert.NotEmpty(t, result)) but don't check the actual output value. This masks incorrect interpolation behavior, such as leaving${...}delimiters in place.Add expected output verification:
func TestResolveStringInterpolation(t *testing.T) { tests := []struct { name string input string setupVars map[string]string expected string expectError bool errorMsg string }{ { name: "string with two tokens", input: "prefix-${self.var1-middle}-${self.var2-suffix}", setupVars: map[string]string{ "var1-middle": "value1", "var2-suffix": "value2", }, + expected: "prefix-value1-value2", expectError: false, }, // ... add expected values to all test cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // ... setup code ... tokens := findAllTokens(tt.input) result, err := td.resolveStringInterpolation("test_intent", tt.input, tokens) if tt.expectError { assert.Error(t, err) if tt.errorMsg != "" { assert.Contains(t, err.Error(), tt.errorMsg) } } else { assert.NoError(t, err) - assert.NotEmpty(t, result) + // Verify actual output matches expected value + assert.Contains(t, result, "var.test_") // TF CDKTF token reference + if tt.expected != "" { + // Can't check exact equality due to CDKTF token wrapping + // but verify structure is correct + assert.NotContains(t, result, "${") + assert.NotContains(t, result, "self.") + assert.NotContains(t, result, "var.") + assert.NotContains(t, result, "infra.") + } } }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
engines/terraform/resolve.go(3 hunks)engines/terraform/resolve_test.go(8 hunks)engines/terraform/token.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
engines/terraform/resolve_test.go (1)
engines/terraform/terraform.go (1)
SpecReference(45-48)
🔇 Additional comments (4)
engines/terraform/resolve.go (3)
16-16: LGTM on enhanced error messages.Including the offending token in validation errors makes debugging much easier.
Also applies to: 21-21, 27-27
97-100: LGTM on listing available variables.Showing available variables when a self-reference is missing is a helpful UX improvement.
5-6: Go version requirement satisfied – engines/go.mod specifies go 1.24.3, which supports themapsandslicespackages.engines/terraform/resolve_test.go (1)
83-90: Verify unclosed brace behavior is intentional.The test case at line 83 expects
"${infra.resource"(unclosed brace) to successfully extract"infra.resource". This seems lenient—should malformed tokens be rejected instead?
tjholm
changed the title
tjholm
changed the title
tjholm
changed the title
tjholm
force-pushed
the
update-token-parsing
branch
from
8105c83 to
1b5d9df
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
engines/terraform/token.go (1)
48-49: Critical interpolation bug persists (duplicate concern).Past reviews correctly identified that these patterns match bare tokens inside
${...}wrappers, causingFindAllStringIndexto return indices that exclude the delimiters. String interpolation replacements then leave${and}in place.Example: For input
"prefix-${self.var1}-suffix":
- Match:
self.var1at indices [9:18]- Replacement:
result[:9] + value + result[18:]→"prefix-${<value>}-suffix"❌See past review comments for detailed fix suggestions.
🧹 Nitpick comments (2)
engines/terraform/token.go (1)
15-15: Clarify supported token formats in comment.The comment should specify that both wrapped (
${infra.X}) and unwrapped (infra.X) formats are accepted, since tests validate both.-// extractTokenContents extracts the contents from a token string (infra/var/self prefix) +// extractTokenContents extracts the contents from a token string (supports both bare "infra.X" and wrapped "${infra.X}" formats)engines/terraform/resolve_test.go (1)
407-483: Add output value verification to interpolation tests.String interpolation tests only assert
NotEmpty(result)without verifying actual output values. This masks the bug flagged in past reviews where wrapped tokens (${self.var1}) leave delimiters in place after replacement.Example enhancement for the test at lines 407-414:
{ name: "string with two tokens", input: "prefix-${self.var1-middle}-${self.var2-suffix}", setupVars: map[string]string{ "var1-middle": "value1", "var2-suffix": "value2", }, + expected: "prefix-value1-value2", expectError: false, },Then update assertions:
} else { assert.NoError(t, err) - assert.NotEmpty(t, result) + if tt.expected != "" { + assert.Equal(t, tt.expected, result) + } else { + assert.NotEmpty(t, result) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
engines/terraform/resolve.go(3 hunks)engines/terraform/resolve_test.go(8 hunks)engines/terraform/token.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- engines/terraform/resolve.go
🧰 Additional context used
🧬 Code graph analysis (1)
engines/terraform/resolve_test.go (1)
engines/terraform/terraform.go (1)
SpecReference(45-48)
🔇 Additional comments (1)
engines/terraform/resolve_test.go (1)
83-90: Unclosed‐brace token extraction is by design
SpecReferenceFromToken’s regex pulls the “infra.resource” substring regardless of trailing “}”, so the${infra.resourcetest is valid.
tjholm
changed the title
tjholm
changed the title
|
🎉 This PR is included in version 0.1.20 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
6 participants
No description provided.