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
merged 14 commits into from
Aug 11, 2025
Merged

support PLANOUT #2077

Show file tree
Hide file tree
Changes from 5 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

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
130 changes: 77 additions & 53 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

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

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

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

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

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

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 All @@ -219,6 +219,11 @@ func (d DiggerExecutor) Plan() (*iac_utils.IacSummary, bool, bool, string, strin
},
}
}

hasPlanStep := lo.ContainsBy(planSteps, func(step scheduler.Step) bool {
return step.Action == "plan"
})

for _, step := range planSteps {
slog.Info("Running step", "action", step.Action)
if step.Action == "init" {
Expand All @@ -234,46 +239,20 @@ 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

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

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)
plan, planSummary, isEmptyPlan, err = d.postProcessPlan(plan)
if err != nil {
slog.Error("error publishing comment", "error", err)
slog.Debug("error post processing plan",
"error", err,
"plan", plan,
"planSummary", planSummary,
"isEmptyPlan", isEmptyPlan,
)
return nil, false, false, "", "", fmt.Errorf("error post processing plan: %v", err) //nolint:wrapcheck // err
}
}
if step.Action == "run" {
Expand All @@ -297,8 +276,64 @@ func (d DiggerExecutor) Plan() (*iac_utils.IacSummary, bool, bool, string, strin
}
}
}

if !hasPlanStep {
var err error
plan, planSummary, isEmptyPlan, err = d.postProcessPlan(plan)
if err != nil {
slog.Debug("error post processing plan",
"error", err,
"plan", plan,
"planSummary", planSummary,
"isEmptyPlan", isEmptyPlan,
)
return nil, false, false, "", "", fmt.Errorf("error post processing plan: %v", err) //nolint:wrapcheck // err
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

When there's no explicit plan step (hasPlanStep is false), the code still tries to post-process the plan by calling d.postProcessPlan(plan). However, the postProcessPlan function attempts to read and process a plan file at d.PlanPathProvider.LocalPlanFilePath() which wouldn't exist if no plan step was executed.

This will lead to errors when trying to read a non-existent file in the postProcessPlan function, specifically at line 317 where it calls os.ReadFile(d.PlanPathProvider.LocalPlanFilePath()). The error is properly caught and returned, but it's an unnecessary error condition that could be avoided by simply not attempting to post-process when there's no plan step.

Suggested change
if !hasPlanStep {
var err error
plan, planSummary, isEmptyPlan, err = d.postProcessPlan(plan)
if err != nil {
slog.Debug("error post processing plan",
"error", err,
"plan", plan,
"planSummary", planSummary,
"isEmptyPlan", isEmptyPlan,
)
return nil, false, false, "", "", fmt.Errorf("error post processing plan: %v", err) //nolint:wrapcheck // err
}
}
if !hasPlanStep {
// Skip post-processing if there was no plan step, as there won't be a plan file to process
slog.Debug("No plan step found, skipping post-processing")
}


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

func (d DiggerExecutor) postProcessPlan(stdout string) (string, *iac_utils.IacSummary, bool, error) {
showArgs := make([]string, 0)
terraformPlanOutput, _, _ := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the postProcessPlan method, there's no check to ensure that the plan file exists before trying to read it. This can lead to errors if the plan file doesn't exist, as the Show method will attempt to read a non-existent file.

The fix adds a check using os.Stat to verify the file exists before proceeding with the Show command. If the file doesn't exist, it returns an appropriate error message that includes the path that was checked, making debugging easier.

Suggested change
showArgs := make([]string, 0)
terraformPlanOutput, _, _ := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
showArgs := make([]string, 0)
// Check if plan file exists before trying to read it
if _, err := os.Stat(d.PlanPathProvider.LocalPlanFilePath()); os.IsNotExist(err) {
return "", nil, false, fmt.Errorf("plan file does not exist at %s: %v", d.PlanPathProvider.LocalPlanFilePath(), err)
}
terraformPlanOutput, _, _ := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)

Copy link
Contributor

Choose a reason for hiding this comment

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

In the postProcessPlan method, errors from the Show command are also ignored. If the Show command fails, the error is silently ignored and the method continues processing with potentially empty or invalid plan output. This could lead to incorrect plan summaries or other downstream issues.

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, _, _ := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
showArgs := make([]string, 0)
terraformPlanOutput, stderr, err := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
if err != nil {
return "", nil, false, fmt.Errorf("error showing plan: %v, stderr: %s", err, stderr)
}


isEmptyPlan, planSummary, err := d.IacUtils.GetSummaryFromPlanJson(terraformPlanOutput)
if err != nil {
return "", nil, 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, 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, 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, fmt.Errorf("error storing artifact file: %v", err)
}
}

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's an inconsistent error handling pattern in the postProcessPlan function. At line 332-334, there's an if err != nil check that logs an error but doesn't return it. This is problematic because:

  1. The err variable being checked was already handled earlier in the function (at lines 301-304) and would have been returned if it was non-nil.
  2. By this point in the code, err should always be nil unless there's a logic error elsewhere.
  3. The error message "error publishing comment" doesn't match any operation being performed in this section of code.

This appears to be a leftover error check that should be removed, as it's checking an error that has already been handled and is using an error message that doesn't match the current operation.

Suggested change
// TODO: move this function to iacUtils interface and implement for pulumi
cleanedUpPlan := cleanupTerraformPlan(stdout)
if err != nil {
slog.Error("error publishing comment", "error", err)
}
// TODO: move this function to iacUtils interface and implement for pulumi
cleanedUpPlan := cleanupTerraformPlan(stdout)

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a bug in the postProcessPlan method where there's an error check at line 332-334 that doesn't make sense. The err variable being checked is not defined in the local scope at this point - the last err variable was defined and handled at line 323-327 for the StorePlanFile operation.

This error check is meaningless and should be removed as it's checking a variable that's already been handled earlier in the function. The code will never log an error here because if there was an error with StorePlanFile, the function would have already returned with an error.

Suggested change
// TODO: move this function to iacUtils interface and implement for pulumi
cleanedUpPlan := cleanupTerraformPlan(stdout)
if err != nil {
slog.Error("error publishing comment", "error", err)
}
// TODO: move this function to iacUtils interface and implement for pulumi
cleanedUpPlan := cleanupTerraformPlan(stdout)

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a bug in the postProcessPlan function where there's an if err != nil check at line 339 that logs an error but doesn't return it. This is problematic because:

  1. The err variable being checked is from the previous StorePlanFile call at line 330, which already has its own error handling that returns the error.
  2. If there was an error in StorePlanFile, the function would have already returned before reaching this check.
  3. This means the error check is redundant and misleading - it's checking a variable that would never contain an error at this point in the code.

This could lead to confusion during debugging and code maintenance. The error check should be removed or commented out to clarify that it's not needed.

Suggested change
// TODO: move this function to iacUtils interface and implement for pulumi
cleanedUpPlan := cleanupTerraformPlan(stdout)
if err != nil {
slog.Error("error publishing comment", "error", err)
}
// TODO: move this function to iacUtils interface and implement for pulumi
cleanedUpPlan := cleanupTerraformPlan(stdout)
// This err check is incorrect - err is from StorePlanFile above and has already been handled
// If there was an error, we would have already returned

Copy link
Contributor

Choose a reason for hiding this comment

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

In the postProcessPlan method in libs/execution/execution.go, there is an if err != nil check on line 340-342 after the cleanupTerraformPlan call. However, cleanupTerraformPlan doesn't return an error - it only returns a string. This means the error check is checking the previous error variable from line 330, which is unrelated to the cleanupTerraformPlan function call.

This is a bug because the error check is misplaced and is checking an error from a previous operation (the PlanStorage.StorePlanFile call) rather than being related to the cleanupTerraformPlan function. This could lead to confusing log messages where errors from storing the plan file are reported as "error publishing comment".

Suggested change
// TODO: move this function to iacUtils interface and implement for pulumi
cleanedUpPlan := cleanupTerraformPlan(stdout)
if err != nil {
slog.Error("error publishing comment", "error", err)
}
// TODO: move this function to iacUtils interface and implement for pulumi
cleanedUpPlan := cleanupTerraformPlan(stdout)

return cleanedUpPlan, planSummary, isEmptyPlan, nil
}

func reportError(r reporting.Reporter, stderr string) {
Expand Down Expand Up @@ -483,25 +518,14 @@ func (d DiggerExecutor) Destroy() (bool, error) {
return true, nil
}

func cleanupTerraformOutput(nonEmptyOutput bool, planError error, stdout string, stderr string, regexStr *string) string {
var errorStr string

func cleanupTerraformOutput(stdout string, regexStr *string) string {
// removes output of terraform -version command that terraform-exec executes on every run
i := strings.Index(stdout, "Initializing the backend...")
if i != -1 {
stdout = stdout[i:]
}
endPos := len(stdout)

if planError != nil {
if stderr != "" {
errorStr = stderr
} else if stdout != "" {
errorStr = stdout
}
return errorStr
}

delimiters := []string{
"Terraform will perform the following actions:",
"OpenTofu will perform the following actions:",
Expand Down Expand Up @@ -535,12 +559,12 @@ func cleanupTerraformOutput(nonEmptyOutput bool, planError error, stdout string,
}

func cleanupTerraformApply(nonEmptyPlan bool, planError error, stdout string, stderr string) string {
return cleanupTerraformOutput(nonEmptyPlan, planError, stdout, stderr, nil)
return cleanupTerraformOutput(stdout, nil)
}

func cleanupTerraformPlan(nonEmptyPlan bool, planError error, stdout string, stderr string) string {
func cleanupTerraformPlan(stdout string) string {
regex := `───────────.+`
return cleanupTerraformOutput(nonEmptyPlan, planError, stdout, stderr, &regex)
return cleanupTerraformOutput(stdout, &regex)
}

func (d DiggerExecutor) projectId() string {
Expand Down
4 changes: 2 additions & 2 deletions libs/execution/execution_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ Terraform will perform the following actions:

Plan: 2 to add, 0 to change, 0 to destroy.
`
res := cleanupTerraformPlan(true, nil, stdout, "")
res := cleanupTerraformPlan(stdout)
index := strings.Index(stdout, "Terraform will perform the following actions:")
assert.Equal(t, stdout[index:], res)
}
Expand Down Expand Up @@ -256,7 +256,7 @@ Plan: 9 to add, 0 to change, 0 to destroy.
Changes to Outputs:
+ api_url = (known after apply)
`
res := cleanupTerraformPlan(true, nil, stdout, "")
res := cleanupTerraformPlan(stdout)
index := strings.Index(stdout, "OpenTofu will perform the following actions:")
assert.Equal(t, stdout[index:], res)
}
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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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