Skip to content

support PLANOUT #2077

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
motatoes merged 14 commits into from
Aug 11, 2025
Merged

support PLANOUT #2077

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0e4c1ea
move execution below
motatoes Aug 9, 2025
af41122
add tf json output
motatoes Aug 9, 2025
a61cd78
fix args order
motatoes Aug 9, 2025
b90269b
fix tests
motatoes Aug 9, 2025
e4c90db
refactor to use postprocess step
motatoes Aug 10, 2025
5062781
fix tests
motatoes Aug 11, 2025
e69c284
refactor and capture errors
motatoes Aug 11, 2025
94e494f
fix other methods
motatoes Aug 11, 2025
05c88b0
add more logs
motatoes Aug 11, 2025
20aff88
return the right params
motatoes Aug 11, 2025
d317ac7
Update libs/execution/tf.go
motatoes Aug 11, 2025
480b5e5
fix errors
motatoes Aug 11, 2025
eee9a3c
use defer
motatoes Aug 11, 2025
329ac93
add docs on overriding plan
motatoes Aug 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cli/pkg/digger/digger_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func (m *MockTerraformExecutor) Destroy(params []string, envs map[string]string)
return "", "", nil
}

func (m *MockTerraformExecutor) Show(params []string, envs map[string]string, planJsonFilePath string) (string, string, error) {
func (m *MockTerraformExecutor) Show(params []string, envs map[string]string, planJsonFilePath string, b bool) (string, string, error) {
Copy link
Contributor

@greptile-apps greptile-apps bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Parameter name 'b' is non-descriptive. Consider using 'returnJson' or 'asJson' to match the intent of the real implementation.

nonEmptyTerraformPlanJson := "{\"format_version\":\"1.1\",\"terraform_version\":\"1.4.6\",\"planned_values\":{\"root_module\":{\"resources\":[{\"address\":\"null_resource.test\",\"mode\":\"managed\",\"type\":\"null_resource\",\"name\":\"test\",\"provider_name\":\"registry.terraform.io/hashicorp/null\",\"schema_version\":0,\"values\":{\"id\":\"7587790946951100994\",\"triggers\":null},\"sensitive_values\":{}},{\"address\":\"null_resource.testx\",\"mode\":\"managed\",\"type\":\"null_resource\",\"name\":\"testx\",\"provider_name\":\"registry.terraform.io/hashicorp/null\",\"schema_version\":0,\"values\":{\"triggers\":null},\"sensitive_values\":{}}]}},\"resource_changes\":[{\"address\":\"null_resource.test\",\"mode\":\"managed\",\"type\":\"null_resource\",\"name\":\"test\",\"provider_name\":\"registry.terraform.io/hashicorp/null\",\"change\":{\"actions\":[\"no-op\"],\"before\":{\"id\":\"7587790946951100994\",\"triggers\":null},\"after\":{\"id\":\"7587790946951100994\",\"triggers\":null},\"after_unknown\":{},\"before_sensitive\":{},\"after_sensitive\":{}}},{\"address\":\"null_resource.testx\",\"mode\":\"managed\",\"type\":\"null_resource\",\"name\":\"testx\",\"provider_name\":\"registry.terraform.io/hashicorp/null\",\"change\":{\"actions\":[\"create\"],\"before\":null,\"after\":{\"triggers\":null},\"after_unknown\":{\"id\":true},\"before_sensitive\":false,\"after_sensitive\":{}}}],\"prior_state\":{\"format_version\":\"1.0\",\"terraform_version\":\"1.4.6\",\"values\":{\"root_module\":{\"resources\":[{\"address\":\"null_resource.test\",\"mode\":\"managed\",\"type\":\"null_resource\",\"name\":\"test\",\"provider_name\":\"registry.terraform.io/hashicorp/null\",\"schema_version\":0,\"values\":{\"id\":\"7587790946951100994\",\"triggers\":null},\"sensitive_values\":{}}]}}},\"configuration\":{\"provider_config\":{\"null\":{\"name\":\"null\",\"full_name\":\"registry.terraform.io/hashicorp/null\"}},\"root_module\":{\"resources\":[{\"address\":\"null_resource.test\",\"mode\":\"managed\",\"type\":\"null_resource\",\"name\":\"test\",\"provider_config_key\":\"null\",\"schema_version\":0},{\"address\":\"null_resource.testx\",\"mode\":\"managed\",\"type\":\"null_resource\",\"name\":\"testx\",\"provider_config_key\":\"null\",\"schema_version\":0}]}}}\n"
m.Commands = append(m.Commands, RunInfo{"Show", strings.Join(params, " "), time.Now()})
return nonEmptyTerraformPlanJson, "", nil
Expand Down Expand Up @@ -391,7 +391,7 @@ func TestCorrectCommandExecutionWhenPlanning(t *testing.T) {

commandStrings := allCommandsInOrderWithParams(terraformExecutor, commandRunner, prManager, lock, planStorage, planPathProvider)

assert.Equal(t, []string{"Init ", "Plan ", "Show ", "StorePlanFile plan", "Run echo"}, commandStrings)
assert.Equal(t, []string{"Init ", "Plan ", "Run echo", "Show ", "StorePlanFile plan", "Show "}, commandStrings)
}

func allCommandsInOrderWithParams(terraformExecutor *MockTerraformExecutor, commandRunner *MockCommandRunner, prManager *MockPRManager, lock *MockProjectLock, planStorage *MockPlanStorage, planPathProvider *MockPlanPathProvider) []string {
Expand Down
92 changes: 52 additions & 40 deletions libs/execution/execution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ func (d DiggerExecutor) RetrievePlanJson() (string, error) {
}

showArgs := make([]string, 0)
terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath)
terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
Copy link
Contributor

@bismuthdev bismuthdev bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code ignores the error returned from the Show method of the TerraformExecutor interface. If the Show method encounters an error, it will be silently ignored, and the function will continue execution with potentially invalid or missing plan data. This could lead to unexpected behavior or failures later in the execution flow.

The Show method can return an error as seen in its implementation in tf.go, and this error should be properly handled to ensure the system behaves correctly when terraform show command fails.

Suggested change
terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
terraformPlanOutput, _, err := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
if err != nil {
return "", fmt.Errorf("failed to show plan: %v", err)
}

return terraformPlanOutput, nil
Comment on lines 194 to 196
Copy link
Contributor

@bismuthdev bismuthdev bot Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RetrievePlanJson method in libs/execution/execution.go ignores errors from the Show command. If the Show command fails, the error is silently ignored and the method returns potentially empty or incomplete output. This could lead to downstream issues where the plan JSON is expected to be valid but is actually empty or malformed.

The fix adds proper error handling to check if the Show command fails and returns an appropriate error message that includes both the error and stderr output for better debugging.

Suggested change
showArgs := make([]string, 0)
terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath)
terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
return terraformPlanOutput, nil
showArgs := make([]string, 0)
terraformPlanOutput, stderr, err := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
if err != nil {
return "", fmt.Errorf("failed to show plan: %v, stderr: %s", err, stderr)
}
return terraformPlanOutput, nil

Comment on lines 194 to 196
Copy link
Contributor

@bismuthdev bismuthdev bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the RetrievePlanJson method in libs/execution/execution.go, there's a bug where the error returned from the Show method is being silently ignored.

The method calls executor.TerraformExecutor.Show() but discards the error value with _, _, _. If the Show method fails (which could happen if there's an issue with the plan file or if Terraform encounters an error), this error is not propagated to the caller.

This is particularly problematic because the method assumes that the output from Show will be valid JSON when returnJson=true is specified, but if there's an error, this might not be the case. The caller of RetrievePlanJson would then receive potentially invalid JSON without any indication that an error occurred.

The fix properly captures and returns the error from the Show method, allowing the caller to handle the error appropriately.

Suggested change
showArgs := make([]string, 0)
terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath)
terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
return terraformPlanOutput, nil
showArgs := make([]string, 0)
terraformPlanOutput, _, err := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
if err != nil {
return "", fmt.Errorf("error running terraform show: %v", err)
}
return terraformPlanOutput, nil

Comment on lines 194 to 196
Copy link
Contributor

@bismuthdev bismuthdev bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RetrievePlanJson method in libs/execution/execution.go ignores errors from the Show method call by using the blank identifier (_) for both the stderr output and error return values. This can lead to silent failures if the Show method encounters an error.

If the Show method fails (for example, due to invalid plan file format, permission issues, or other errors), the code will continue execution and return potentially invalid or empty plan JSON data. This could cause downstream issues when this data is used for further processing.

The fix adds proper error handling to check if the Show method returns an error, and if so, returns an appropriate error message that includes both the error and stderr output for better debugging.

Suggested change
showArgs := make([]string, 0)
terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath)
terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
return terraformPlanOutput, nil
showArgs := make([]string, 0)
terraformPlanOutput, stderr, err := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
if err != nil {
return "", fmt.Errorf("failed to show plan: %v, stderr: %v", err, stderr)
}
return terraformPlanOutput, nil


} else {
Expand All @@ -202,7 +202,7 @@ func (d DiggerExecutor) RetrievePlanJson() (string, error) {

func (d DiggerExecutor) Plan() (*iac_utils.IacSummary, bool, bool, string, string, error) {
plan := ""
terraformPlanOutput := ""
terraformPlanOutputJsonString := ""
Copy link
Contributor

@bismuthdev bismuthdev bot Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable terraformPlanOutputJsonString is initialized but never populated with any value before being returned from the Plan() function. This is a bug because the function signature indicates it should return the JSON output from the Show command as its fifth return value.

The function already calls Show() in the postProcessPlan() method, but it doesn't store the result in the terraformPlanOutputJsonString variable. This means that any code expecting to receive the JSON output from the Plan function will receive an empty string instead.

The fix adds a call to the Show command to populate the variable with the JSON output before returning it.

Suggested change
terraformPlanOutputJsonString := ""
terraformPlanOutput, _, _ := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
terraformPlanOutputJsonString := terraformPlanOutput

Copy link
Contributor

@bismuthdev bismuthdev bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Plan() method of DiggerExecutor, the variable terraformPlanOutputJsonString is initialized but never populated with a value before being returned. This variable should be set to the output of the Show command, which is captured in the postProcessPlan method as terraformPlanOutput.

The bug is that the value from terraformPlanOutput in the postProcessPlan method is never assigned to terraformPlanOutputJsonString in the Plan method, resulting in an empty string being returned instead of the actual JSON output from Terraform.

This is a bug because the return value is expected to contain the JSON output from Terraform's plan, but it's always returning an empty string, which could cause issues for any code that depends on this value.

Suggested change
terraformPlanOutputJsonString := ""
terraformPlanOutputJsonString := ""

planSummary := &iac_utils.IacSummary{}
isEmptyPlan := true
var planSteps []scheduler.Step
Expand Down Expand Up @@ -234,47 +234,11 @@ func (d DiggerExecutor) Plan() (*iac_utils.IacSummary, bool, bool, string, strin
// TODO remove those only for pulumi project
planArgs = append(planArgs, step.ExtraArgs...)

_, stdout, stderr, err := d.TerraformExecutor.Plan(planArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), d.PlanStage.FilterRegex)
_, _, _, err := d.TerraformExecutor.Plan(planArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), d.PlanStage.FilterRegex)
Copy link
Contributor

@bismuthdev bismuthdev bot Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a potential null pointer dereference in the Plan method of DiggerExecutor. When d.PlanStage is nil, the code correctly creates default plan steps, but later when calling the Plan method on the TerraformExecutor, it directly uses d.PlanStage.FilterRegex without checking if d.PlanStage is nil. This could lead to a null pointer dereference.

The fix adds a check to safely handle the case when d.PlanStage is nil by creating a local filterRegex variable that is only set from d.PlanStage.FilterRegex when d.PlanStage is not nil.

Suggested change
_, _, _, err := d.TerraformExecutor.Plan(planArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), d.PlanStage.FilterRegex)
var filterRegex *string
if d.PlanStage != nil {
filterRegex = d.PlanStage.FilterRegex
}
_, _, _, err := d.TerraformExecutor.Plan(planArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), filterRegex)

Copy link
Contributor

@bismuthdev bismuthdev bot Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a potential nil pointer dereference in the Plan method of DiggerExecutor. On line 241, d.PlanStage.FilterRegex is accessed without first checking if d.PlanStage is nil.

While there is a check at the beginning of the method (lines 209-220) to initialize planSteps if d.PlanStage is nil, there's no similar check when accessing d.PlanStage.FilterRegex directly in the call to d.TerraformExecutor.Plan().

If d.PlanStage is nil at this point, accessing d.PlanStage.FilterRegex would cause a nil pointer dereference and result in a panic.

The fix adds a check to safely handle the case where d.PlanStage might be nil by creating a local variable filterRegex that is only set from d.PlanStage.FilterRegex if d.PlanStage is not nil.

Suggested change
_, _, _, err := d.TerraformExecutor.Plan(planArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), d.PlanStage.FilterRegex)
var filterRegex *string
if d.PlanStage != nil {
filterRegex = d.PlanStage.FilterRegex
}
_, _, _, err := d.TerraformExecutor.Plan(planArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), filterRegex)

if err != nil {
return nil, false, false, "", "", fmt.Errorf("error executing plan: %v", err)
}
showArgs := make([]string, 0)
terraformPlanOutput, _, _ = d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath())

isEmptyPlan, planSummary, err = d.IacUtils.GetSummaryFromPlanJson(terraformPlanOutput)
if err != nil {
return nil, false, false, "", "", fmt.Errorf("error checking for empty plan: %v", err)
}

if !isEmptyPlan {
nonEmptyPlanFilepath := strings.Replace(d.PlanPathProvider.LocalPlanFilePath(), d.PlanPathProvider.StoredPlanFilePath(), "isNonEmptyPlan.txt", 1)
file, err := os.Create(nonEmptyPlanFilepath)
if err != nil {
return nil, false, false, "", "", fmt.Errorf("unable to create file: %v", err)
}
defer file.Close()
}

if d.PlanStorage != nil {

fileBytes, err := os.ReadFile(d.PlanPathProvider.LocalPlanFilePath())
if err != nil {
fmt.Println("Error reading file:", err)
return nil, false, false, "", "", fmt.Errorf("error reading file bytes: %v", err)
}

err = d.PlanStorage.StorePlanFile(fileBytes, d.PlanPathProvider.ArtifactName(), d.PlanPathProvider.StoredPlanFilePath())
if err != nil {
fmt.Println("Error storing artifact file:", err)
return nil, false, false, "", "", fmt.Errorf("error storing artifact file: %v", err)
}
}

// TODO: move this function to iacUtils interface and implement for pulumi
plan = cleanupTerraformPlan(!isEmptyPlan, err, stdout, stderr)
if err != nil {
slog.Error("error publishing comment", "error", err)
}
}
if step.Action == "run" {
var commands []string
Expand All @@ -297,8 +261,56 @@ func (d DiggerExecutor) Plan() (*iac_utils.IacSummary, bool, bool, string, strin
}
}
}

///////
showArgs := make([]string, 0)
terraformPlanOutputJsonString, _, err := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
if err != nil {
return nil, false, false, "", "", fmt.Errorf("error showing plan: %v", err)
}

isEmptyPlan, planSummary, err = d.IacUtils.GetSummaryFromPlanJson(terraformPlanOutputJsonString)
if err != nil {
return nil, false, false, "", "", fmt.Errorf("error checking for empty plan: %v", err)
}

if !isEmptyPlan {
nonEmptyPlanFilepath := strings.Replace(d.PlanPathProvider.LocalPlanFilePath(), d.PlanPathProvider.StoredPlanFilePath(), "isNonEmptyPlan.txt", 1)
file, err := os.Create(nonEmptyPlanFilepath)
if err != nil {
return nil, false, false, "", "", fmt.Errorf("unable to create file: %v", err)
}
defer file.Close()
}

if d.PlanStorage != nil {
fileBytes, err := os.ReadFile(d.PlanPathProvider.LocalPlanFilePath())
if err != nil {
fmt.Println("Error reading file:", err)
return nil, false, false, "", "", fmt.Errorf("error reading file bytes: %v", err)
}

err = d.PlanStorage.StorePlanFile(fileBytes, d.PlanPathProvider.ArtifactName(), d.PlanPathProvider.StoredPlanFilePath())
if err != nil {
fmt.Println("Error storing artifact file:", err)
return nil, false, false, "", "", fmt.Errorf("error storing artifact file: %v", err)
}
}

terraformPlanOutput, _, err := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
Copy link
Contributor

@bismuthdev bismuthdev bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Plan method in DiggerExecutor calls Show twice - once with the boolean parameter true to get JSON output for parsing, and once with false to get human-readable output. However, the implementations of Show in Terragrunt, Pulumi, and OpenTofu always include the -json flag regardless of the boolean parameter value, making the second call redundant and potentially causing unexpected behavior.

The fix checks if the executor is specifically a Terraform executor (which respects the boolean parameter) and only makes the second call in that case. For other executors, it reuses the JSON output from the first call since they always return JSON format anyway.

if err != nil {
return nil, false, false, "", "", fmt.Errorf("error showing plan: %v", err)
}

// TODO: move this function to iacUtils interface and implement for pulumi
plan = cleanupTerraformPlan(!isEmptyPlan, nil, terraformPlanOutput, "")
if err != nil {
slog.Error("error publishing comment", "error", err)
}
Copy link
Contributor

@greptile-apps greptile-apps bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Logic error: 'err' variable is nil at this point (last assignment was from Show() call), but you're checking if err != nil. This will never execute the error logging.

//////

reportAdditionalOutput(d.Reporter, d.projectId())
return planSummary, true, !isEmptyPlan, plan, terraformPlanOutput, nil
return planSummary, true, !isEmptyPlan, plan, terraformPlanOutputJsonString, nil
}

func reportError(r reporting.Reporter, stderr string) {
Expand Down
2 changes: 1 addition & 1 deletion libs/execution/opentofu.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func (tf OpenTofu) Plan(params []string, envs map[string]string, planArtefactFil
return statusCode == 2, stdout, stderr, nil
}

func (tf OpenTofu) Show(params []string, envs map[string]string, planArtefactFilePath string) (string, string, error) {
func (tf OpenTofu) Show(params []string, envs map[string]string, planArtefactFilePath string, b bool) (string, string, error) {
params = append(params, []string{"-no-color", "-json", planArtefactFilePath}...)
Copy link
Contributor

@greptile-apps greptile-apps bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: The boolean parameter b is added but not used. Unlike the Terraform implementation, this always includes -json regardless of the parameter value.

stdout, stderr, _, err := tf.runOpentofuCommand("show", false, envs, nil, params...)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion libs/execution/pulumi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func (pl Pulumi) Plan(params []string, envs map[string]string, planArtefactFileP
return statusCode == 2, stdout, stderr, nil
}

func (pl Pulumi) Show(params []string, envs map[string]string, planArtefactFilePath string) (string, string, error) {
func (pl Pulumi) Show(params []string, envs map[string]string, planArtefactFilePath string, b bool) (string, string, error) {
pl.selectStack()
// TODO figure out how to avoid running a second plan (preview) here
params = append(params, []string{"--json"}...)
Expand Down
2 changes: 1 addition & 1 deletion libs/execution/terragrunt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func (terragrunt Terragrunt) Plan(params []string, envs map[string]string, planA
return true, stdout, stderr, err
}

func (terragrunt Terragrunt) Show(params []string, envs map[string]string, planArtefactFilePath string) (string, string, error) {
func (terragrunt Terragrunt) Show(params []string, envs map[string]string, planArtefactFilePath string, b bool) (string, string, error) {
Copy link
Contributor

@greptile-apps greptile-apps bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: The boolean parameter b is added but never used. Consider renaming it to returnJson for clarity and implementing the conditional logic like in the Terraform implementation.

Copy link
Contributor

@bismuthdev bismuthdev bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter b in the Show method is not descriptive and makes the code harder to maintain. The parameter appears to indicate whether to return JSON format, as seen in the Terraform implementation where it's named retrunJson (though misspelled). Renaming it to returnJson would make the code more readable and maintainable.

Suggested change
func (terragrunt Terragrunt) Show(params []string, envs map[string]string, planArtefactFilePath string, b bool) (string, string, error) {
func (terragrunt Terragrunt) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {

params = append(params, []string{"-no-color", "-json", planArtefactFilePath}...)
Copy link
Contributor

@greptile-apps greptile-apps bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Unlike the Terraform implementation, the -json flag is hardcoded here. Consider making this conditional based on the boolean parameter for consistency across executors.

stdout, stderr, exitCode, err := terragrunt.runTerragruntCommand("show", false, envs, nil, params...)
if exitCode != 0 {
Expand Down
10 changes: 7 additions & 3 deletions libs/execution/tf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ type TerraformExecutor interface {
Apply([]string, *string, map[string]string) (string, string, error)
Destroy([]string, map[string]string) (string, string, error)
Plan([]string, map[string]string, string, *string) (bool, string, string, error)
Show([]string, map[string]string, string) (string, string, error)
Show([]string, map[string]string, string, bool) (string, string, error)
Copy link
Contributor

@bismuthdev bismuthdev bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The boolean parameter in the Show method interface is not descriptive. It should be documented with a comment to indicate its purpose (returnJson) for better code readability and maintainability.

Suggested change
Show([]string, map[string]string, string, bool) (string, string, error)
Show([]string, map[string]string, string, bool /* returnJson */) (string, string, error)

Copy link
Contributor

@bismuthdev bismuthdev bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TerraformExecutor interface in libs/execution/tf.go has a boolean parameter in the Show method signature without any documentation explaining its purpose. This makes it difficult for future developers to understand what this parameter does without digging through implementation code.

Additionally, there's a typo in the implementation where the parameter is named retrunJson instead of returnJson, which further confuses the purpose of this parameter.

The fix adds proper documentation to the interface method explaining that the boolean parameter controls whether the output should be in JSON format or human-readable format.

Suggested change
Show([]string, map[string]string, string, bool) (string, string, error)
// Show displays the plan output in various formats
// params: Additional command line parameters to pass to the terraform show command
// envs: Environment variables to set when running the command
// planArtefactFilePath: Path to the plan file to show
// returnJson: When true, outputs the plan in JSON format, otherwise in human-readable format
Show([]string, map[string]string, string, bool) (string, string, error)

Copy link
Contributor

@bismuthdev bismuthdev bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the TerraformExecutor interface, the parameter in the Show method is named simply 'bool', which is not descriptive of its purpose. In the implementation (Terraform.Show method), this parameter is named 'retrunJson' (which also has a typo - it should be 'returnJson').

The interface should be updated to use a more descriptive parameter name that matches its purpose, which is to indicate whether the output should be returned in JSON format. This improves code readability and makes the interface more self-documenting.

Suggested change
Show([]string, map[string]string, string, bool) (string, string, error)
Show([]string, map[string]string, string, returnJson bool) (string, string, error)

}

type Terraform struct {
Expand Down Expand Up @@ -167,8 +167,12 @@ func (tf Terraform) Plan(params []string, envs map[string]string, planArtefactFi
return statusCode == 2, stdout, stderr, nil
}

func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string) (string, string, error) {
params = append(params, []string{"-no-color", "-json", planArtefactFilePath}...)
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
Copy link
Contributor

@greptile-apps greptile-apps bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: Parameter name has typo: retrunJson should be returnJson

Copy link
Contributor

@bismuthdev bismuthdev bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter name 'retrunJson' in the Show method has a typo. It should be 'returnJson' instead. This is a simple spelling error that should be fixed for code clarity and consistency.

Suggested change
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {

Copy link
Contributor

@bismuthdev bismuthdev bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter name retrunJson in the Terraform.Show method has a typo. It should be returnJson instead of retrunJson.

Suggested change
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {

Copy link
Contributor

@bismuthdev bismuthdev bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter retrunJson in the Show method is misspelled and should be returnJson. This typo makes the code harder to maintain and understand. Correcting the spelling would improve code readability and consistency with the other implementations.

Suggested change
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {

Copy link
Contributor

@bismuthdev bismuthdev bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo in the parameter name in the Terraform.Show method implementation. The parameter is named 'retrunJson' but should be 'returnJson'. This typo should be fixed to maintain consistent and correct naming throughout the codebase.

Suggested change
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {

params = append(params, "-no-color")
if retrunJson {
Copy link
Contributor

@bismuthdev bismuthdev bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo in the parameter name of the Show method in the Terraform struct. The parameter is named retrunJson instead of returnJson. This typo should be fixed for better code readability and consistency.

Suggested change
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
params = append(params, "-no-color")
if retrunJson {
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {
params = append(params, "-no-color")
if returnJson {

params = append(params, "-json")
}
Copy link
Contributor

@bismuthdev bismuthdev bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo in the parameter name 'retrunJson' in the Show method of the Terraform struct. The parameter should be named 'returnJson' instead. This typo could lead to confusion for developers maintaining the code, as it doesn't follow the standard naming convention and could be easily misread. Consistent parameter naming is important for code readability and maintainability.

Suggested change
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
params = append(params, "-no-color")
if retrunJson {
params = append(params, "-json")
}
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {
params = append(params, "-no-color")
if returnJson {
params = append(params, "-json")
}

Copy link
Contributor

@bismuthdev bismuthdev bot Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo in the parameter name retrunJson in the Show method of the Terraform struct. The parameter should be named returnJson instead. This typo could cause confusion for developers who are trying to understand or use this method, as the parameter name doesn't clearly convey its purpose due to the misspelling.

Suggested change
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
params = append(params, "-no-color")
if retrunJson {
params = append(params, "-json")
}
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {
params = append(params, "-no-color")
if returnJson {
params = append(params, "-json")
}

Copy link
Contributor

@bismuthdev bismuthdev bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo in the parameter name retrunJson in the Show method of the Terraform struct. The parameter name should be returnJson (with "return" spelled correctly). This typo could cause confusion for future developers who are trying to understand or modify this code, as it makes the code less readable and could lead to inconsistent naming conventions throughout the codebase.

Suggested change
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
params = append(params, "-no-color")
if retrunJson {
params = append(params, "-json")
}
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {
params = append(params, "-no-color")
if returnJson {
params = append(params, "-json")
}

Copy link
Contributor

@bismuthdev bismuthdev bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a typo in the parameter name of the Show method implementation in the Terraform struct. The parameter is named retrunJson instead of returnJson, which is confusing and inconsistent with the intended meaning. This typo should be fixed to improve code readability and maintainability.

Suggested change
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
params = append(params, "-no-color")
if retrunJson {
params = append(params, "-json")
}
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {
params = append(params, "-no-color")
if returnJson {
params = append(params, "-json")
}

Copy link
Contributor

@bismuthdev bismuthdev bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo in the parameter name retrunJson in the Show method of the Terraform struct. The parameter should be named returnJson instead. This typo could cause confusion for future developers who might expect the parameter to be named correctly. The typo is consistent throughout the method, so the code will still function correctly, but it's a readability and maintainability issue.

Suggested change
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
params = append(params, "-no-color")
if retrunJson {
params = append(params, "-json")
}
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {
params = append(params, "-no-color")
if returnJson {
params = append(params, "-json")
}

Copy link
Contributor

@bismuthdev bismuthdev bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo in the parameter name in the Show method implementation in libs/execution/tf.go. The parameter is named retrunJson instead of returnJson. This is inconsistent with the correct spelling that would be expected and could cause confusion for developers working with this code. The typo appears in both the function signature and where the parameter is used in the conditional statement.

Suggested change
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
params = append(params, "-no-color")
if retrunJson {
params = append(params, "-json")
}
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {
params = append(params, "-no-color")
if returnJson {
params = append(params, "-json")
}

Copy link
Contributor

@bismuthdev bismuthdev bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter name retrunJson in the Show method of the Terraform struct is misspelled. It should be returnJson instead. This typo could cause confusion for developers maintaining the code and makes the codebase less consistent.

Suggested change
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
params = append(params, "-no-color")
if retrunJson {
params = append(params, "-json")
}
func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {
params = append(params, "-no-color")
if returnJson {
params = append(params, "-json")
}

params = append(params, planArtefactFilePath)
stdout, stderr, _, err := tf.runTerraformCommand("show", false, envs, nil, params...)
if err != nil {
return "", "", err
Expand Down
Loading