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 all 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
2 changes: 1 addition & 1 deletion 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
19 changes: 19 additions & 0 deletions docs/ce/howto/custom-commands.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,22 @@ If your custom command writes into a file path defined in the `$DIGGER_OUT` env
![](/images/custom-command-output-infracost.png)

The value of `$DIGGER_OUT` defaults to `$RUNNER_TEMP/digger-out.log`; you can change that if needed by setting the env var explicitly.

## Overriding plan commands

You can add extra arguments to the plan command by setting the `extra_args` key in the `steps` section of the `plan` command.

However in some cases if you wish to override the plan command entirely you can do it by excluding the plan in the steps and having your command specified in the run like so:

```

workflows:
default:
plan:
steps:
- init
# exclude plan entierly and use custom command
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax: Typo: 'entierly' should be 'entirely'

- run: terraform plan -input=false -refresh -no-color -out $DIGGER_PLANFILE
```

Note that you need to use the -out flag to write the output to the $DIGGER_PLANFILE env variable, since this will be used in postprocessing steps by digger.
137 changes: 83 additions & 54 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,22 @@ 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)
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)
var err error
var stdout, stderr string
isEmptyPlan, stdout, stderr, err = d.TerraformExecutor.Plan(planArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), d.PlanStage.FilterRegex)
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)
}
return nil, false, false, "", "", fmt.Errorf("error executing plan: %v, stdout: %v, stderr: %v", err, stdout, stderr)
}

// TODO: move this function to iacUtils interface and implement for pulumi
plan = cleanupTerraformPlan(!isEmptyPlan, err, stdout, stderr)
plan, terraformPlanOutputJsonString, planSummary, isEmptyPlan, err = d.postProcessPlan(stdout)
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 +278,67 @@ func (d DiggerExecutor) Plan() (*iac_utils.IacSummary, bool, bool, string, strin
}
}
}

if !hasPlanStep {
rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
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 inconsistency in how the Show method is called in the codebase. In the RetrievePlanJson method (line 194), it always passes true for the JSON format parameter, but in the Plan method when there's no plan step (line 282), it passes false.

This inconsistency could lead to unexpected behavior because:

  1. The postProcessPlan method (called right after this) expects JSON output as it calls GetSummaryFromPlanJson on line 309
  2. The Show method in tf.go uses this parameter to determine whether to add the -json flag to the terraform command

By changing the parameter to true in the Plan method, we ensure consistent behavior across the codebase and make sure the output is always in JSON format when needed for further processing.

Suggested change
rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), 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.

There's an inconsistency in how the Show method is called in the execution.go file. When there's no plan step defined (line 282), Show is called with returnJson=false, but in the postProcessPlan method (line 304), it's called with returnJson=true.

This inconsistency can lead to incorrect plan processing when there's no explicit plan step defined. The postProcessPlan method expects JSON output to parse with GetSummaryFromPlanJson, but when there's no plan step, it receives non-JSON output, which would cause parsing errors.

The fix is to make both calls consistent by setting returnJson=true in both places.

Suggested change
rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), 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.

The code in the Plan method checks for hasPlanStep and calls Show with returnJson=false when there's no plan step. This is inconsistent with all other Show calls in the codebase which use returnJson=true.

This inconsistency could cause issues because:

  1. The postProcessPlan method expects JSON format to parse the plan correctly
  2. All implementations of Show in different executors (Terraform, OpenTofu, Pulumi, Terragrunt) handle the returnJson parameter by adding the -json flag
  3. Without the JSON format, the subsequent parsing in postProcessPlan will likely fail

The bug was introduced in the new code that handles the case when there's no plan step. The fix changes the returnJson parameter to true to maintain consistency with other Show calls.

Suggested change
rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)

if err != nil {
return nil, false, false, "", "", fmt.Errorf("error running terraform show: %v", err)
}
plan, terraformPlanOutputJsonString, planSummary, isEmptyPlan, err = d.postProcessPlan(rawPlan)
Comment on lines +283 to +287
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 an inconsistency in the code flow when !hasPlanStep. In this case, the code calls Show with returnJson=false to get a human-readable plan, but then immediately passes this non-JSON output to postProcessPlan. Inside postProcessPlan, it makes another call to Show with returnJson=true to get the JSON version for parsing with GetSummaryFromPlanJson.

While this doesn't cause a functional bug (since postProcessPlan correctly gets the JSON format itself), the code is confusing and could lead to maintenance issues. I've added comments to clarify the intended behavior and make it clear that the first call gets the human-readable format, while postProcessPlan will separately get the JSON format.

Suggested change
rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
if err != nil {
return nil, false, false, "", "", fmt.Errorf("error running terraform show: %v", err)
}
plan, terraformPlanOutputJsonString, planSummary, isEmptyPlan, err = d.postProcessPlan(rawPlan)
// First get the raw plan output (human readable format)
rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
if err != nil {
return nil, false, false, "", "", fmt.Errorf("error running terraform show: %v", err)
}
// Pass the raw plan to postProcessPlan which will get the JSON version separately
plan, terraformPlanOutputJsonString, planSummary, isEmptyPlan, err = d.postProcessPlan(rawPlan)

Comment on lines +283 to +287
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, when hasPlanStep is false, the code calls Show with returnJson=false, but then passes the result to postProcessPlan. This is inconsistent because postProcessPlan doesn't actually use the passed stdout parameter for JSON processing - it makes its own call to Show with returnJson=true to get the JSON data.

While this doesn't cause errors (since the non-JSON output is only used for display purposes via cleanupTerraformPlan), it's confusing and could lead to maintenance issues. I've added comments to clarify the intended behavior.

Suggested change
rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
if err != nil {
return nil, false, false, "", "", fmt.Errorf("error running terraform show: %v", err)
}
plan, terraformPlanOutputJsonString, planSummary, isEmptyPlan, err = d.postProcessPlan(rawPlan)
// First get the raw plan output (non-JSON format)
rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
if err != nil {
return nil, false, false, "", "", fmt.Errorf("error running terraform show: %v", err)
}
// Pass the raw plan to postProcessPlan which will get the JSON version internally
plan, terraformPlanOutputJsonString, planSummary, isEmptyPlan, err = d.postProcessPlan(rawPlan)

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
}
}
Comment on lines +282 to +297
Copy link
Contributor

Choose a reason for hiding this comment

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

When a custom command is used instead of the built-in plan step, there's no validation that the custom command actually generated a valid plan file. If the custom command fails to create the plan file, the code will attempt to read a non-existent file, resulting in cryptic errors like "error running terraform show" instead of clearly indicating that the custom command failed to generate the plan file.

The fix adds a check to verify that the plan file exists before attempting to show it, providing a more helpful error message that points to the likely cause of the problem (the custom command failing to generate the plan file).

Suggested change
if !hasPlanStep {
rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
if err != nil {
return nil, false, false, "", "", fmt.Errorf("error running terraform show: %v", err)
}
plan, terraformPlanOutputJsonString, planSummary, isEmptyPlan, err = d.postProcessPlan(rawPlan)
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 {
// Check if the plan file exists before attempting to show it
if _, err := os.Stat(d.PlanPathProvider.LocalPlanFilePath()); os.IsNotExist(err) {
return nil, false, false, "", "", fmt.Errorf("plan file not found at %s - custom command may have failed to generate it", d.PlanPathProvider.LocalPlanFilePath())
}
rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
if err != nil {
return nil, false, false, "", "", fmt.Errorf("error running terraform show: %v", err)
}
plan, terraformPlanOutputJsonString, planSummary, isEmptyPlan, err = d.postProcessPlan(rawPlan)
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
}
}


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, string, *iac_utils.IacSummary, bool, error) {
showArgs := make([]string, 0)
terraformPlanJsonOutputString, _, err := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
if err != nil {
return "", "", nil, false, fmt.Errorf("error running terraform show: %v", err)
}
Comment on lines +305 to +308
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 of DiggerExecutor, there is a potential nil pointer dereference when accessing d.PlanPathProvider. The method directly calls d.PlanPathProvider.LocalPlanFilePath() without first checking if d.PlanPathProvider is nil. If d.PlanPathProvider is nil, this would cause a nil pointer dereference and crash the application.

The fix adds a nil check before accessing the PlanPathProvider to prevent the potential crash.

Suggested change
terraformPlanJsonOutputString, _, err := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
if err != nil {
return "", "", nil, false, fmt.Errorf("error running terraform show: %v", err)
}
if d.PlanPathProvider == nil {
return "", "", nil, false, fmt.Errorf("plan path provider is nil")
}
terraformPlanJsonOutputString, _, err := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
if err != nil {
return "", "", nil, false, fmt.Errorf("error running terraform show: %v", err)
}

Comment on lines +304 to +308
Copy link
Contributor

Choose a reason for hiding this comment

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

The postProcessPlan method doesn't check if the plan file exists before attempting to run terraform show on it. Additionally, it ignores the stderr output from the Show command, which could contain valuable diagnostic information in case of failure.

The fix:

  1. Adds a check to verify the plan file exists before attempting to run terraform show
  2. Captures and logs the stderr output from the Show command when an error occurs
  3. Uses structured logging with slog.Error to provide better context for debugging

This change improves error handling and provides more diagnostic information when the terraform show command fails, making it easier to troubleshoot issues.

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


isEmptyPlan, planSummary, err := d.IacUtils.GetSummaryFromPlanJson(terraformPlanJsonOutputString)
if err != nil {
return "", "", nil, false, fmt.Errorf("error checking for empty plan: %v", err)
}
Comment on lines +310 to +313
Copy link
Contributor

Choose a reason for hiding this comment

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

The postProcessPlan method in DiggerExecutor calls d.IacUtils.GetSummaryFromPlanJson() without checking if d.IacUtils is nil. This could lead to a nil pointer dereference if the IacUtils field is not initialized when the method is called.

The fix adds a nil check before attempting to use d.IacUtils. If it's nil, the method returns default values that allow execution to continue safely.

Suggested change
isEmptyPlan, planSummary, err := d.IacUtils.GetSummaryFromPlanJson(terraformPlanJsonOutputString)
if err != nil {
return "", "", nil, false, fmt.Errorf("error checking for empty plan: %v", err)
}
if d.IacUtils == nil {
return "", terraformPlanJsonOutputString, &iac_utils.IacSummary{}, true, nil
}
isEmptyPlan, planSummary, err := d.IacUtils.GetSummaryFromPlanJson(terraformPlanJsonOutputString)
if err != nil {
return "", "", nil, false, fmt.Errorf("error checking for empty plan: %v", err)
}

Comment on lines +305 to +313
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's a potential issue with error handling when processing Terraform plan JSON.

The method calls Show with returnJson=true and then passes the result to GetSummaryFromPlanJson. If the plan has an error or is not valid JSON, GetSummaryFromPlanJson will return an error, but the current implementation will cause the entire planning process to fail with a generic error message.

Looking at the terraform_test.go file, there's a test case TestPlanOutputInvalidJsonFailsGracefully which confirms that GetSummaryFromPlanJson properly handles invalid JSON by returning an error, but the calling code in postProcessPlan doesn't handle this gracefully.

The fix modifies the error handling to:

  1. Log the error and the problematic JSON for debugging
  2. Return a more informative error message
  3. Return default values that allow the process to continue rather than failing completely

This makes the system more robust when dealing with unexpected plan output formats.

Suggested change
terraformPlanJsonOutputString, _, err := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
if err != nil {
return "", "", nil, false, fmt.Errorf("error running terraform show: %v", err)
}
isEmptyPlan, planSummary, err := d.IacUtils.GetSummaryFromPlanJson(terraformPlanJsonOutputString)
if err != nil {
return "", "", nil, false, fmt.Errorf("error checking for empty plan: %v", err)
}
terraformPlanJsonOutputString, _, err := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
if err != nil {
return "", "", nil, false, fmt.Errorf("error running terraform show: %v", err)
}
isEmptyPlan, planSummary, err := d.IacUtils.GetSummaryFromPlanJson(terraformPlanJsonOutputString)
if err != nil {
// If we can't parse the JSON, log the error but don't fail the entire operation
slog.Error("error parsing plan JSON", "error", err, "json", terraformPlanJsonOutputString)
return "", terraformPlanJsonOutputString, &iac_utils.IacSummary{}, true, 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()
}
Comment on lines +315 to +322
Copy link
Contributor

Choose a reason for hiding this comment

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

Another potential nil pointer dereference exists in the postProcessPlan method when creating the non-empty plan file. The code accesses d.PlanPathProvider without checking if it's nil. If d.PlanPathProvider is nil, this would cause a nil pointer dereference when trying to call LocalPlanFilePath() and StoredPlanFilePath().

The fix adds a nil check to ensure d.PlanPathProvider is not nil before attempting to access its methods.

Suggested change
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 !isEmptyPlan && d.PlanPathProvider != nil {
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()
}

Comment on lines +315 to +322
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, a file is created with os.Create when the plan is not empty, and the file handle is closed using defer file.Close(). However, if any of the subsequent operations in the function fail (like reading the plan file or storing it), the function will return early with an error, but the deferred close will only execute when the function completes normally.

This pattern can lead to file handle leaks if there are multiple error paths after the file is created. The fix is to close the file immediately after creation since we don't actually write anything to it - we're just creating an empty marker file. This ensures the file handle is properly closed regardless of any subsequent errors.

Suggested change
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 !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)
}
// Close file immediately after creation since we don't write to it
// This ensures the file handle is closed even if subsequent operations fail
file.Close()
}

Comment on lines +315 to +322
Copy link
Contributor

Choose a reason for hiding this comment

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

The postProcessPlan method creates a marker file when a non-empty plan is detected, but it doesn't actually write any content to the file. This means the file is created but remains empty, which could cause issues if other parts of the code check for the existence of content in this file rather than just the file's existence.

The fix adds a simple string write operation to ensure the file contains a marker indicating it's a non-empty plan. This makes the purpose of the file clearer and ensures that any code checking the file's content will work correctly.

Suggested change
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 !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()
// Write a marker to the file to indicate it's a non-empty plan
if _, err := file.WriteString("non-empty plan"); err != nil {
return "", "", nil, false, fmt.Errorf("unable to write to file: %v", err)
}
}

Comment on lines +315 to +322
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, when creating a file at nonEmptyPlanFilepath, the code doesn't check if the parent directory exists first. This can cause the os.Create() call to fail with an error like "no such file or directory" if any part of the directory path doesn't exist.

The fix adds a call to os.MkdirAll() to ensure the directory structure exists before attempting to create the file. This is a common pattern when creating files in potentially non-existent directories.

Suggested change
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 !isEmptyPlan {
nonEmptyPlanFilepath := strings.Replace(d.PlanPathProvider.LocalPlanFilePath(), d.PlanPathProvider.StoredPlanFilePath(), "isNonEmptyPlan.txt", 1)
// Ensure directory exists before creating file
dir := path.Dir(nonEmptyPlanFilepath)
if err := os.MkdirAll(dir, 0755); err != nil {
return "", "", nil, false, fmt.Errorf("unable to create directory structure: %v", err)
}
file, err := os.Create(nonEmptyPlanFilepath)
if err != nil {
return "", "", nil, false, fmt.Errorf("unable to create file: %v", err)
}
defer file.Close()
}

Comment on lines +315 to +322
Copy link
Contributor

Choose a reason for hiding this comment

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

The postProcessPlan method creates a file at nonEmptyPlanFilepath but doesn't ensure the parent directory exists. This can cause file creation to fail in certain directory structures where the parent directory doesn't exist yet.

The fix adds a call to os.MkdirAll() to create the parent directory structure before attempting to create the file. This ensures that the file creation will succeed even if the parent directories don't exist yet.

Suggested change
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 !isEmptyPlan {
nonEmptyPlanFilepath := strings.Replace(d.PlanPathProvider.LocalPlanFilePath(), d.PlanPathProvider.StoredPlanFilePath(), "isNonEmptyPlan.txt", 1)
// Ensure parent directory exists
dir := path.Dir(nonEmptyPlanFilepath)
if err := os.MkdirAll(dir, 0755); err != nil {
return "", "", nil, false, fmt.Errorf("unable to create directory: %v", err)
}
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)

}
}
Comment on lines +324 to +337
Copy link
Contributor

Choose a reason for hiding this comment

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

A third potential nil pointer dereference exists in the postProcessPlan method when storing the plan file. The code checks if d.PlanStorage is not nil, but doesn't check if d.PlanPathProvider is nil before accessing its methods. If d.PlanPathProvider is nil, this would cause a nil pointer dereference when trying to call LocalPlanFilePath(), ArtifactName(), and StoredPlanFilePath().

The fix adds a nil check to ensure both d.PlanStorage and d.PlanPathProvider are not nil before attempting to access their methods.

Suggested change
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)
}
}
if d.PlanStorage != nil && d.PlanPathProvider != 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)
}
}

Comment on lines +324 to +337
Copy link
Contributor

Choose a reason for hiding this comment

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

The current implementation of postProcessPlan has several issues with error handling when storing plan files:

  1. It uses fmt.Println for error logging instead of the structured logger (slog) that's used elsewhere in the codebase.
  2. It doesn't check if the plan file exists before attempting to read it, which could lead to confusing error messages.
  3. The error messages don't include important context like the file path or artifact name.

The fix:

  1. Adds a check to verify the plan file exists before attempting to read it
  2. Replaces fmt.Println with structured logging using slog.Error
  3. Includes more context in the error messages
  4. Fixes a formatting issue with an extra newline in the error handling
Suggested change
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)
}
}
if d.PlanStorage != nil {
// Check if the 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 path %s: %v", d.PlanPathProvider.LocalPlanFilePath(), err)
}
fileBytes, err := os.ReadFile(d.PlanPathProvider.LocalPlanFilePath())
if err != nil {
slog.Error("Error reading plan file", "error", err, "path", d.PlanPathProvider.LocalPlanFilePath())
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 {
slog.Error("Error storing plan file", "error", err, "artifact", d.PlanPathProvider.ArtifactName())
return "", "", nil, false, fmt.Errorf("error storing artifact file: %v", err)
}
}

Comment on lines +324 to +337
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 no check to verify if the plan file exists before attempting to read it. If the plan file doesn't exist, the code will call os.ReadFile on a non-existent file, which will result in an error.

The current error handling only catches the error after attempting to read the file, but it would be better to explicitly check if the file exists first. This provides a more specific error message and avoids unnecessary operations when the file is missing.

The fix adds an explicit check using os.Stat to verify the file exists before attempting to read it, and returns a more descriptive error message if the file is missing.

Suggested change
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)
}
}
if d.PlanStorage != nil {
// Check if the plan file exists before trying to read it
if _, err := os.Stat(d.PlanPathProvider.LocalPlanFilePath()); os.IsNotExist(err) {
fmt.Println("Plan file does not exist:", d.PlanPathProvider.LocalPlanFilePath())
return "", "", nil, false, fmt.Errorf("plan file does not exist: %v", err)
}
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)
return cleanedUpPlan, terraformPlanJsonOutputString, planSummary, isEmptyPlan, nil
}

func reportError(r reporting.Reporter, stderr string) {
Expand Down Expand Up @@ -483,25 +523,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 +564,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)
}
8 changes: 6 additions & 2 deletions libs/execution/opentofu.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,12 @@ 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) {
params = append(params, []string{"-no-color", "-json", planArtefactFilePath}...)
func (tf OpenTofu) 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.runOpentofuCommand("show", false, envs, nil, params...)
if err != nil {
return "", "", err
Expand Down
7 changes: 5 additions & 2 deletions libs/execution/pulumi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,13 @@ 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, returnJson bool) (string, string, error) {
pl.selectStack()
// TODO figure out how to avoid running a second plan (preview) here
params = append(params, []string{"--json"}...)
if returnJson {
params = append(params, []string{"--json"}...)
}

stdout, stderr, statusCode, err := pl.runPululmiCommand("preview", false, envs, params...)
if err != nil && statusCode != 2 {
return "", "", err
Expand Down
8 changes: 6 additions & 2 deletions libs/execution/terragrunt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,12 @@ 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) {
params = append(params, []string{"-no-color", "-json", planArtefactFilePath}...)
func (terragrunt Terragrunt) 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, exitCode, err := terragrunt.runTerragruntCommand("show", false, envs, nil, params...)
if exitCode != 0 {
logCommandFail(exitCode, err)
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, 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