feat: migrate infracost to console repo#3626
Conversation
Greptile SummaryThis PR migrates the infracost integration from the deployment-operator repo into the console repo, adding cost estimation for Terraform stack runs. When an
Confidence Score: 3/5Safe to merge for the infracost feature itself, but the restructured The infracost logic is well-structured and the Dockerfile change is clean. The concern is in go/deployment-operator/pkg/harness/tool/terraform/terraform.go needs the nil guard corrected to cover all three
|
| Filename | Overview |
|---|---|
| go/deployment-operator/pkg/harness/tool/terraform/terraform.go | Refactors New() to populate tf.env from config.Run.Env(), but the added nil guard for config.Run comes after two unconditional dereferences of config.Run.Parallelism and config.Run.Refresh in the struct literal, leaving the panic risk unresolved. |
| go/deployment-operator/pkg/harness/tool/terraform/infracost.go | New file implementing infracost integration: plan-to-JSON conversion, subprocess execution, and report parsing. Logic is sound; minor inconsistency in env passing for terraform show vs. infracost subprocess. |
| go/deployment-operator/pkg/harness/tool/terraform/infracost_types.go | Defines JSON-mapping structs for the infracost report format and scope constants; InfracostResourceScopeFree is declared but never used. |
| go/deployment-operator/pkg/harness/controller/controller_hooks.go | Calls in.tool.Infracost() after afterPlan, logs errors non-fatally, and forwards results to UpdateStackRun. Error handling is intentionally soft (infracost is optional). |
| go/deployment-operator/pkg/harness/tool/v1/tool.go | Adds default Infracost() implementation on DefaultTool returning nil, nil — correct no-op for tools that don't support cost estimation. |
| go/deployment-operator/pkg/harness/tool/v1/types.go | Adds Infracost() to the Tool interface; straightforward interface extension. |
| go/deployment-operator/dockerfiles/harness/terraform.Dockerfile | Adds a multi-arch infracost binary download stage using the official GitHub release tarball, then copies the binary into the final image. |
| go/deployment-operator/pkg/harness/tool/terraform/terraform_types.go | Adds the env []string field to Terraform struct for storing stack-run environment variables. |
Reviews (1): Last reviewed commit: "migrate infracost to console repo" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| tf := &Terraform{ | ||
| DefaultTool: v1.DefaultTool{Scanner: config.Scanner}, | ||
| workDir: config.WorkDir, | ||
| dir: config.ExecDir, | ||
| variables: config.Variables, | ||
| parallelism: config.Run.Parallelism, | ||
| refresh: config.Run.Refresh, | ||
| }).init() | ||
| } | ||
| if config.Run != nil { | ||
| tf.env = config.Run.Env() | ||
| } | ||
| return tf.init() | ||
| } |
There was a problem hiding this comment.
The struct literal on lines 275–276 accesses config.Run.Parallelism and config.Run.Refresh before the if config.Run != nil check on line 278. If config.Run is ever nil, the function panics at the struct literal — the guard only protects tf.env. The nil check should wrap all three config.Run accesses, or the existing accesses should be moved inside the guard.
| type InfracostResourceScope string | ||
|
|
||
| const ( | ||
| InfracostResourceScopeBreakdown InfracostResourceScope = "breakdown" | ||
| InfracostResourceScopePastBreakdown InfracostResourceScope = "past_breakdown" | ||
| InfracostResourceScopeDiff InfracostResourceScope = "diff" | ||
| InfracostResourceScopeFree InfracostResourceScope = "free" |
There was a problem hiding this comment.
InfracostResourceScopeFree constant is unused
InfracostResourceScopeFree is declared but never referenced in convertResource or anywhere else in this PR. Resources with no hourly or monthly cost are silently dropped via the if hourlyCost == nil && monthlyCost == nil early return in convertResource, so this scope value is never emitted. Either remove the constant or use it to represent explicitly free resources.
| klog.V(log.LogLevelExtended).InfoS("executing", "command", "terraform show -json "+in.planFileName) | ||
|
|
||
| runErr := cmd.Run() | ||
| if closeErr := tmpFile.Close(); closeErr != nil && runErr == nil { | ||
| runErr = closeErr | ||
| } | ||
| if runErr != nil { | ||
| _ = os.Remove(tmpFile.Name()) | ||
| return "", fmt.Errorf("failed converting plan to JSON: %s: %w", stderr.String(), runErr) | ||
| } | ||
|
|
||
| klog.V(log.LogLevelTrace).InfoS("converted terraform plan to JSON", "tempFile", filepath.Base(tmpFile.Name())) | ||
| return tmpFile.Name(), nil | ||
| } | ||
|
|
There was a problem hiding this comment.
terraform show subprocess does not inherit stack-run env vars
exec.CommandContext without setting cmd.Env inherits the calling process's OS environment, not the user-configured in.env passed to the infracost subprocess via harnessexec.WithEnv(in.env). For the terraform show -json call this is likely benign (plan conversion only reads a local file), but the inconsistency could bite if a custom provider or wrapper reads a credential from the stack-run env. Consider adding cmd.Env = in.env for parity.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
kinjalh
changed the title
kinjalh
force-pushed
the
infracost-deployment-op-migration
branch
from
889ef17 to
8c71b43
Compare
kinjalh
force-pushed
the
infracost-deployment-op-migration
branch
from
8c71b43 to
c34f0b4
Compare
|
@kinjalh I'm not the right person to review this, make sure sebastian, marcin or lukasz look it over. |
2 participants
Test Plan
https://console.kinjal-gitgud.onplural.sh/
Checklist