fix(engines): resolve format variables and multi variables#87
fix(engines): resolve format variables and multi variables#87
Conversation
HomelessDinosaur
requested review from
davemooreuws,
jyecusch,
raksiv,
sean-nitric and
tjholm
📝 WalkthroughWalkthroughReplaces inline string token handling with a dedicated token-resolution flow: new helpers Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
engines/terraform/resolve.go(2 hunks)engines/terraform/token.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
engines/terraform/resolve.go (2)
engines/terraform/deployment.go (1)
TerraformDeployment(16-28)engines/terraform/terraform.go (1)
SpecReference(46-49)
🔇 Additional comments (1)
engines/terraform/token.go (1)
45-46: Guard against nil slices when matching tokens
allTokensPattern.FindString(input)returns an empty string (the zero value) when no token is found, so trimming it is fine. No changes needed.
davemooreuws
changed the title
tjholm
left a comment
tjholm
left a comment
There was a problem hiding this comment.
Marking as request changes as I submitted the last review item as just a comment.
HomelessDinosaur
force-pushed
the
resolve-format-variables
branch
from
b8d30bd to
a0d13ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
engines/terraform/resolve.go(2 hunks)engines/terraform/token.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
engines/terraform/resolve.go (3)
engines/terraform/deployment.go (1)
TerraformDeployment(16-28)engines/terraform/terraform.go (1)
SpecReference(45-48)engines/terraform/platform.go (1)
ResourceBlueprint(233-238)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
engines/terraform/resolve.go (1)
136-156: Byte-offset slicing can panic on multi-byte UTF-8 characters.
token.Startandtoken.Endare byte offsets. If the input string contains multi-byte UTF-8 characters before a token, slicing by byte index can split a rune and cause a panic or corruption.Convert to rune-aware slicing:
result := input + runes := []rune(result) for i := len(tokens) - 1; i >= 0; i-- { token := tokens[i] specRef, err := SpecReferenceFromToken(token.Token) if err != nil { continue } tokenValue, err := td.resolveToken(intentName, specRef) if err != nil { return nil, err } // Convert token to string representation for concatenation replacement := cdktf.Token_AsString(tokenValue, nil) if replacement == nil { return nil, fmt.Errorf("failed to convert token %s to string", token.Token) } - result = result[:token.Start] + *replacement + result[token.End:] + // Convert byte offsets to rune offsets + runeStart := len([]rune(input[:token.Start])) + runeEnd := len([]rune(input[:token.End])) + runes = append(append(runes[:runeStart], []rune(*replacement)...), runes[runeEnd:]...) } + result = string(runes) return result, nil }
🧹 Nitpick comments (3)
engines/Makefile (1)
14-15: Verify gotestsum availability before running test-pretty.The target assumes
gotest.tools/gotestsumis installed. Consider checking or documenting the prerequisite.test-pretty: + @which gotestsum > /dev/null || (echo "gotestsum not found. Install with: go install gotest.tools/gotestsum@latest" && exit 1) @go tool gotest.tools/gotestsum --format=pkgname ./terraform/...engines/terraform/resolve_test.go (1)
1-200: Expand test coverage for infra/var sources and multi-token substitution.The tests validate self-references and token parsing but don't cover:
resolveTokenwith "infra" source (success/error paths)resolveTokenwith "var" source (success/error paths)resolveTokenStringwith multiple tokens in a single string (e.g.,"prefix-${infra.a.b}-middle-${var.c}-suffix")Consider adding tests like:
func TestResolveToken_InfraReference(t *testing.T) { // Test infra resource resolution with valid/missing attributes } func TestResolveToken_VarReference(t *testing.T) { // Test platform variable resolution } func TestResolveTokenString_MultipleTokens(t *testing.T) { // Test string with multiple embedded tokens }engines/terraform/resolve.go (1)
149-153: Token_AsString can fail for complex types.
cdktf.Token_AsStringreturnsnilfor values that cannot be represented as strings (e.g., deeply nested maps). The error message could be more actionable.replacement := cdktf.Token_AsString(tokenValue, nil) if replacement == nil { - return nil, fmt.Errorf("failed to convert token %s to string", token.Token) + return nil, fmt.Errorf("token %s resolved to a type that cannot be embedded in a string (only single tokens may resolve to complex types)", token.Token) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
engines/Makefile(1 hunks)engines/terraform/resolve.go(3 hunks)engines/terraform/resolve_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
engines/terraform/resolve_test.go (3)
engines/terraform/terraform.go (1)
SpecReference(45-48)engines/terraform/resolve.go (1)
SpecReferenceFromToken(11-26)engines/terraform/deployment.go (1)
TerraformDeployment(16-28)
engines/terraform/resolve.go (3)
engines/terraform/deployment.go (1)
TerraformDeployment(16-28)engines/terraform/terraform.go (1)
SpecReference(45-48)engines/terraform/platform.go (1)
ResourceBlueprint(233-238)
🪛 checkmake (0.2.2)
engines/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🔇 Additional comments (10)
engines/terraform/resolve_test.go (3)
11-96: LGTM! Comprehensive token parsing validation.The test covers valid infra/self/var references, nested paths, and multiple error cases with clear assertions.
98-180: LGTM! Self-reference resolution properly validated.Tests cover error cases (missing variable name, non-existent variable) and success cases (single and nested paths) with proper setup.
182-200: LGTM! Unknown source error handling verified.engines/terraform/resolve.go (7)
11-26: LGTM! Token parsing with proper validation.The function correctly extracts token contents and validates minimum path requirements.
28-58: LGTM! Proper delegation to resolveTokenString.String values now delegate to the dedicated token resolution flow, while maintaining recursive traversal for maps and arrays.
60-80: LGTM! Infra resolution with multi-path support and validation.The code now:
- Validates minimum path length
- Joins all path segments beyond the resource name
- Returns descriptive errors for missing attributes
This addresses past review feedback.
81-98: LGTM! Self-reference resolution with multi-path support.The code validates variable names and handles nested paths via
cdktf.Fn_Lookup, addressing past review feedback about multi-path cases.
100-117: LGTM! Var resolution with validation.Platform variable resolution includes proper validation and descriptive errors.
119-134: LGTM! Single-token optimization is sound.When a string contains only one token, returning the resolved value directly (without string conversion) preserves type information.
161-173: LGTM! Nil check prevents writing unresolved values.The guard on line 167 prevents writing
nilto modules, addressing the NIT-339 panic.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
engines/Makefile(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
engines/Makefile
[warning] 2-2: Missing required phony target "all"
(minphony)
[warning] 2-2: Missing required phony target "clean"
(minphony)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
engines/terraform/resolve.go (2)
11-26: Improve error messages with examples.The generic "invalid reference format" doesn't guide users on what's expected. Consider adding examples in the error messages.
Apply this diff:
func SpecReferenceFromToken(token string) (*SpecReference, error) { contents, ok := extractTokenContents(token) if !ok { - return nil, fmt.Errorf("invalid reference format") + return nil, fmt.Errorf("invalid reference format: token must be in the form ${source.path} (e.g., ${infra.db.outputs}, ${self.my_var})") } parts := strings.Split(contents, ".") if len(parts) < 2 { - return nil, fmt.Errorf("invalid reference format") + return nil, fmt.Errorf("invalid reference format: token must have at least a source and one path component (e.g., ${infra.db}, ${self.my_var})") } return &SpecReference{ Source: parts[0], Path: parts[1:], }, nil }
118-135: LGTM! Consider logging malformed token attempts.The routing logic correctly preserves types for single-token values and delegates to string interpolation for multiple tokens. Line 128 silently ignores malformed tokens—this may be intentional for backward compatibility, but consider logging a warning to help debug configuration issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
engines/terraform/resolve.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
engines/terraform/resolve.go (4)
engines/terraform/deployment.go (1)
TerraformDeployment(16-28)engines/terraform/terraform.go (1)
SpecReference(45-48)engines/terraform/token.go (1)
TokenMatch(8-13)engines/terraform/platform.go (1)
ResourceBlueprint(233-238)
🔇 Additional comments (3)
engines/terraform/resolve.go (3)
28-58: LGTM!Clean refactor that properly delegates string token resolution to the new helper while maintaining recursive resolution for maps and arrays.
163-175: LGTM! Nil check prevents panic.The nil check (line 169) correctly prevents setting nil values on modules, addressing the panic mentioned in NIT-339 per the past review comment.
99-111: Consistent multi-path handling for var source
Thevarcase only usesspecRef.Path[0]and ignores any additional segments, unlikeselfwhich supports nested lookup viaFn_Lookup. If your platform variables may be maps/objects (e.g.${var.config.endpoint}), choose one of:
- Implement multi-path resolution for
var(as in lines 93–96)- Or reject/throw an error when
len(specRef.Path) > 1to avoid silent dropsPlease confirm the intended behavior for nested
vartokens and update accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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(1 hunks)engines/terraform/token.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- engines/terraform/resolve_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
engines/terraform/resolve.go (4)
engines/terraform/deployment.go (1)
TerraformDeployment(16-28)engines/terraform/terraform.go (1)
SpecReference(45-48)engines/terraform/token.go (1)
TokenMatch(8-13)engines/terraform/platform.go (1)
ResourceBlueprint(233-238)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
engines/terraform/resolve.go (2)
14-27: Consider more specific error messages for better developer experience.The validation logic is sound, but generic "invalid reference format" messages don't guide users toward fixing their tokens. Past reviewers suggested context-specific messages like "'${var.}' doesn't contain a valid variable reference" or "infra reference requires at least 2 path components."
Consider something like:
func SpecReferenceFromToken(token string) (*SpecReference, error) { contents, ok := extractTokenContents(token) if !ok { - return nil, fmt.Errorf("invalid reference format") + return nil, fmt.Errorf("invalid token format: %s", token) } parts := strings.Split(contents, ".") if len(parts) < 2 { - return nil, fmt.Errorf("invalid reference format") + return nil, fmt.Errorf("reference '%s' must include a source and at least one path component (e.g., ${var.name})", token) } // Validate that all path components are non-empty for _, part := range parts[1:] { if part == "" { - return nil, fmt.Errorf("invalid reference format") + return nil, fmt.Errorf("reference '%s' contains empty path components", token) } }
106-122: Nested var lookups now work, but error message needs refinement.The
Fn_Lookupimplementation (lines 118-121) correctly handles${var.config.endpoint}and addresses prior concerns. However, line 108 formatsspecRef.Source(always "var") instead of providing useful context.Consider:
if len(specRef.Path) < 1 { - return nil, fmt.Errorf("var `%s` doesn't contain a valid variable reference", specRef.Source) + return nil, fmt.Errorf("var reference must specify a variable name (e.g., ${var.my_var})") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
engines/terraform/resolve.go(3 hunks)engines/terraform/resolve_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- engines/terraform/resolve_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
engines/terraform/resolve.go (4)
engines/terraform/deployment.go (1)
TerraformDeployment(16-28)engines/terraform/terraform.go (1)
SpecReference(45-48)engines/terraform/token.go (1)
TokenMatch(8-13)engines/terraform/platform.go (1)
ResourceBlueprint(233-238)
🔇 Additional comments (5)
engines/terraform/resolve.go (5)
35-65: LGTM!The delegation to
resolveTokenValuefor strings cleanly separates concerns, and the recursive handling of maps and arrays is correct.
67-86: Excellent fix for nested infra paths and missing attributes.The change from
specRef.Path[1]tostrings.Join(specRef.Path[1:], ".")(line 75) correctly handles multi-level lookups like${infra.db.outputs.endpoint}, and returning an error for missing attributes (lines 83-85) prevents silent failures. Both address prior critical concerns.
88-104: LGTM! Nested self lookups now supported.The addition of
Fn_Lookupfor multi-segment paths (lines 100-103) correctly resolves references like${self.config.endpoint}, addressing the past concern about multi-path cases.
129-172: LGTM! Clean separation of single-token vs multi-token handling.The logic correctly preserves native types for single-token strings (lines 136-142) while converting multi-token strings via interpolation (line 145). The error message at lines 165-166 is clear and actionable when a non-string-compatible value appears in interpolation context.
174-186: LGTM! Nil check prevents panics.The nil guard (line 180) before setting module properties correctly prevents the panic described in NIT-339.
|
🎉 This PR is included in version 0.1.13 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
7 participants
Closes NIT-339