fix(engines): ensure infra created if referenced in depends_on#81
fix(engines): ensure infra created if referenced in depends_on#81
Conversation
WalkthroughThe Terraform engine now uses a two-phase process: first resolving dependencies for resources and infra, then resolving tokens. resolveDependencies was updated to explicitly resolve infra references in depends_on via td.resolveInfraResource and return errors on failure; module IDs are taken from the resolved infra resource’s node ID. In deployment, resolveInfraResource now calls td.createVariablesForIntent before constructing the TerraformHclModule. The main orchestration in terraform.go reflects the dependency-first, token-second flow for both resources and infra. No exported/public signatures were changed. Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (4)
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 |
tjholm
force-pushed
the
ensure-infra-depends
branch
from
3fb3fe9 to
f8472ca
Compare
![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
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.go (1)
36-45: Prevent panic on malformed infra tokensAccessing specRef.Path[1] without length check can panic. Validate path length first.
Apply this diff:
- if specRef.Source == "infra" { - refName := specRef.Path[0] - propertyName := specRef.Path[1] + if specRef.Source == "infra" { + if len(specRef.Path) < 2 { + return nil, fmt.Errorf("invalid infra token: expected infra.<resource>.<property>, got %q", v) + } + refName := specRef.Path[0] + propertyName := specRef.Path[1]
🧹 Nitpick comments (1)
engines/terraform/resolve.go (1)
104-124: Deduplicate depends_on, validate token shape, and guard Node().Id()Avoid duplicate deps, provide clearer errors, and prevent potential nil deref on Id.
Apply this diff:
- dependsOnResources := []*string{} - for _, dependsOn := range resource.DependsOn { - specRef, err := SpecReferenceFromToken(dependsOn) - if err != nil { - return err - } - - if specRef.Source != "infra" { - return fmt.Errorf("depends_on can only reference infra resources") - } - - // Ensure the infra resource is created if it doesn't exist - infraResource, err := td.resolveInfraResource(specRef.Path[0]) - if err != nil { - return fmt.Errorf("failed to resolve infra resource %s: %w", specRef.Path[0], err) - } - - moduleId := fmt.Sprintf("module.%s", *infraResource.Node().Id()) - dependsOnResources = append(dependsOnResources, jsii.String(moduleId)) - } - module.SetDependsOn(&dependsOnResources) + visited := make(map[string]struct{}, len(resource.DependsOn)) + dependsOnResources := make([]*string, 0, len(resource.DependsOn)) + for _, dependsOn := range resource.DependsOn { + specRef, err := SpecReferenceFromToken(dependsOn) + if err != nil { + return fmt.Errorf("invalid depends_on token %q: %w", dependsOn, err) + } + + if specRef.Source != "infra" { + return fmt.Errorf("depends_on can only reference infra resources: %q", dependsOn) + } + if len(specRef.Path) != 1 { + return fmt.Errorf("depends_on must reference only an infra resource name (e.g. ${infra.myres}), got: %q", dependsOn) + } + + refName := specRef.Path[0] + if _, seen := visited[refName]; seen { + continue + } + + // Ensure the infra resource is created if it doesn't exist + infraResource, err := td.resolveInfraResource(refName) + if err != nil { + return fmt.Errorf("failed to resolve infra resource %s: %w", refName, err) + } + + id := infraResource.Node().Id() + if id == nil || *id == "" { + return fmt.Errorf("resolved infra resource %s has empty node id", refName) + } + moduleId := fmt.Sprintf("module.%s", *id) + dependsOnResources = append(dependsOnResources, jsii.String(moduleId)) + visited[refName] = struct{}{} + } + module.SetDependsOn(&dependsOnResources)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
engines/terraform/resolve.go(1 hunks)
🔇 Additional comments (1)
engines/terraform/resolve.go (1)
115-119: Nice: proactively resolving infra in depends_onThis aligns behavior with intent and surfaces misconfigurations early. Looks good.
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/deployment.go(3 hunks)engines/terraform/resolve.go(1 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/deployment.go (1)
engines/terraform/platform.go (1)
IdentitiesBlueprint(227-229)
🔇 Additional comments (2)
engines/terraform/deployment.go (2)
73-74: Good addition; confirm error/idempotency behavior of createVariablesForIntentCalling createVariablesForIntent before module creation makes sense. Please confirm it’s idempotent and cannot fail silently. If it can error, propagate it.
133-134: No-op formatting changeNothing to do.
tjholm
force-pushed
the
ensure-infra-depends
branch
from
4a9289e to
b1f5f2b
Compare
davemooreuws
changed the title
tjholm
force-pushed
the
ensure-infra-depends
branch
from
b1f5f2b to
f64ed82
Compare
fix: enable infra level instance variables.
jyecusch
force-pushed
the
ensure-infra-depends
branch
from
f64ed82 to
5d85cd0
Compare
|
🎉 This PR is included in version 0.1.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
5 participants
No description provided.