add terraform plan-diff command #1144
Conversation
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
golangci-lint found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
main_plan_diff_integration_test.go (5)
1-126: Good test design for the new plan-diff commandThe integration test is well structured, effectively testing the functionality of the new Terraform plan-diff command while handling process exit codes appropriately. The mocking of
os.Exitis a good approach that allows for testing commands that would normally terminate the process.Some improvements that would make this test even more robust:
- Consider adding a test case for when plans are identical (should return exit code 0)
- Add validation for the actual output content to verify the diff is correct
88-89: Use filepath.Join instead of path.Join for cross-platform compatibilityThe
pathpackage uses forward slashes which may cause issues on Windows. Since this is for file paths, usefilepath.Joinwhich handles platform-specific path separators.- origPlanFile := path.Join(tmpDir, "orig.plan") - newPlanFile := path.Join(tmpDir, "new.plan") + origPlanFile := filepath.Join(tmpDir, "orig.plan") + newPlanFile := filepath.Join(tmpDir, "new.plan")Don't forget to add the import:
import ( "os" - "path" + "path/filepath" "runtime" "testing"
82-86: Add verification that plan files were created successfullyThe test assumes the plan files were created successfully but doesn't verify it. Adding a simple check would make the test more robust.
tmpDir, err := os.MkdirTemp("", "atmos-plan-diff-test") if err != nil { t.Fatalf("failed to create temporary directory: %v", err) } defer os.RemoveAll(tmpDir)
110-115: Consider capturing and validating the command outputThe test verifies the exit code but doesn't validate the actual diff output. Adding validation would ensure the command is producing the expected output format and content.
os.Args = []string{"atmos", "terraform", "plan-diff", "component-1", "-s", "nonprod", "--orig=" + origPlanFile, "--new=" + newPlanFile} + // Capture stdout to verify output + oldStdout := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + exitCode = runMainWithExitCode() + + // Restore stdout and read captured output + w.Close() + os.Stdout = oldStdout + var buf bytes.Buffer + io.Copy(&buf, r) + output := buf.String() + + // Verify output contains expected diff content + if !strings.Contains(output, "foo=new-value") { + t.Errorf("Expected diff output to show variable change, got: %s", output) + } + t.Logf("After plan-diff, exit code: %d", exitCode)Don't forget to add the necessary imports:
import ( + "bytes" + "io" "os" "path" "runtime" + "strings" "testing"
56-58: Add comment explaining Windows-specific behaviorThe Windows-specific handling is important but could benefit from a more detailed explanation of why this is necessary.
// On Windows, don't wait for done as it might deadlock if runtime.GOOS != "windows" { - <-done // Wait for main to finish on non-Windows platforms + <-done // Wait for main to finish on non-Windows platforms + // Windows has different goroutine handling that can cause deadlocks in this scenario }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main_plan_diff_integration_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
main_plan_diff_integration_test.go (1)
120-121: Consider replacing static sleep with a robust check.A 500ms sleep may not always suffice, especially on congested systems or slow environments. Consider a loop-based wait or a file lock mechanism for Windows-specific file timing issues.
- time.Sleep(500 * time.Millisecond) // E.g., poll until file operations complete or a max timeout is reached + for i := 0; i < 10; i++ { + if checkAllFilesAreClosedOrReady() { + break + } + time.Sleep(100 * time.Millisecond) + }internal/exec/terraform_plan_diff.go (1)
149-160: Avoid forced exit within library code.Calling
u.OsExit(2)here precludes upstream callers from handling differences in a customized way. Consider returning an error or a sentinel value to let callers decide how to exit or continue.- u.OsExit(2) - return nil // This line will never be reached + return errors.New("terraform plans differ -- exit code 2 recommended")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/terraform_plan_diff.go(1 hunks)main_plan_diff_integration_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
main_plan_diff_integration_test.go (2)
13-27: Well-structured approach to capturing exit codes.This technique helps keep the test process under control by avoiding an actual
os.Exit, ensuring integration tests can robustly verify different exit conditions. Great work!
54-67:❓ Verification inconclusive
Request concurrency consideration.
Modifying global state like
runtime.GOOSchecks or interceptingu.OsExitcan be risky if tests run in parallel. Confirm no concurrency issues arise or mark this test as serial if needed.Please verify with your CI or test runner that these tests do not run concurrently, or otherwise provide a concurrency-safe approach (e.g., using sync primitives).
🏁 Script executed:
#!/bin/bash # Attempt to locate any concurrency or parallel annotation in the test suite rg -A 3 't\.Parallel\(\)'Length of output: 27
Concurrency Check: Verify Serial Execution of Affected Tests
The search for
t.Parallel()didn’t return any output, so we can’t be sure that these tests aren’t running concurrently. Please manually verify with your CI or test runner that tests—especially those manipulating global state (likeruntime.GOOSand interceptingu.OsExit)—aren’t executed in parallel. If there’s a risk of concurrent execution, consider marking this test as serial or introducing explicit synchronization.internal/exec/terraform_plan_diff.go (1)
33-45: Good use of a temporary directory.You are properly isolating intermediate files and cleaning them up after use, preserving the host environment’s cleanliness.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/exec/terraform_plan_diff.go (3)
31-69: Consider returning errors instead of callingos.Exit.Forcing process termination at line 159 with
u.OsExit(2)can surprise callers and complicate testing. It may be more flexible to propagate an error indicating a plan diff was detected, so the calling code can handle that gracefully.
72-98: Double-check handling of--origand--newflags.The flag parsing logic looks correct. As a small preventive measure, consider validating that each flag’s argument does not begin with
--to avoid flag collisions (e.g.,--orig --new something).
115-165: Avoid overshadowing stdout for diff messages.The diff is printed to
os.Stdoutbefore callingu.OsExit(2). If other logs or data are being printed concurrently, it might be interleaved. Consider capturing output separately and printing in a controlled manner.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/terraform_plan_diff.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
internal/exec/terraform_plan_diff.go (5)
internal/exec/terraform_plan_diff_preparation.go (2) (2)
err(18-18)prepareNewPlanFile(15-34)internal/exec/terraform_plan_diff_comparison.go (8) (8)
sortMapKeys(11-30)diff(54-54)diff(85-85)diff(121-121)diff(140-140)diff(295-295)diff(323-323)generatePlanDiff(53-76)pkg/errors/terraform_errors.go (1) (1)
ErrPlanHasDiff(6-6)pkg/utils/log_utils.go (1) (1)
OsExit(23-23)internal/exec/constants.go (1) (1)
planFileMode(18-18)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
internal/exec/terraform_plan_diff.go (1)
167-191: Revisit potential large plan file outputs.This logic calls
runTerraformShow, which can generate large JSON outputs. Ensure there’s enough buffer capacity or streaming approach to avoid choking the pipe if Terraform outputs large data.
mcalhoun
force-pushed
the
feature/add-plan-diff-command
branch
from
1ab52fe to
9f9402a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
internal/exec/terraform_plan_diff.go (6)
40-43: Consider adding test coverage for error handling when creating the temporary directory.
This block is not covered by tests according to the static analysis and is critical for robust error scenarios.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-43: internal/exec/terraform_plan_diff.go#L42-L43
Added lines #L42 - L43 were not covered by tests
63-65: Add negative test scenarios to exerciseprepareNewPlanFilefailures.
IfprepareNewPlanFilereturns an error, these lines remain untested. Consider adding coverage to confirm correct handling of initialization failures.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 64-65: internal/exec/terraform_plan_diff.go#L64-L65
Added lines #L64 - L65 were not covered by tests
124-127: Add coverage for the error branch during retrieval of the new plan JSON.
Simulate a situation in whichgetTerraformPlanJSONfails, ensuring proper error propagation is tested.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 124-127: internal/exec/terraform_plan_diff.go#L124-L127
Added lines #L124 - L127 were not covered by tests
130-139: Test invalid or malformed plan JSON scenarios.
These lines that parse JSON remain untested for error conditions. Include coverage for corrupted or empty JSON to solidify error handling.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 130-134: internal/exec/terraform_plan_diff.go#L130-L134
Added lines #L130 - L134 were not covered by tests
[warning] 136-139: internal/exec/terraform_plan_diff.go#L136-L139
Added lines #L136 - L139 were not covered by tests
208-234: Expand testing to cover plan file copying logic.
Simulate a scenario where the source file is missing or the destination is unwritable, ensuring each step of the copy flow is validated.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 208-234: internal/exec/terraform_plan_diff.go#L208-L234
Added lines #L208 - L234 were not covered by tests
299-310: Increase test coverage forrunTerraformInitlogic.
Verifying the-reconfigureflag scenario on different platforms ensures consistent initialization.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 299-299: internal/exec/terraform_plan_diff.go#L299
Added line #L299 was not covered by tests
[warning] 310-310: internal/exec/terraform_plan_diff.go#L310
Added line #L310 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/terraform_plan_diff.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
internal/exec/terraform_plan_diff.go (5)
internal/exec/terraform_plan_diff_preparation.go (2) (2)
err(18-18)prepareNewPlanFile(15-34)internal/exec/terraform_plan_diff_comparison.go (8) (8)
sortMapKeys(11-30)diff(54-54)diff(85-85)diff(121-121)diff(140-140)diff(295-295)diff(323-323)generatePlanDiff(53-76)pkg/errors/terraform_errors.go (1) (1)
ErrPlanHasDiff(6-6)pkg/utils/log_utils.go (1) (1)
OsExit(23-23)internal/exec/constants.go (1) (1)
planFileMode(18-18)
🪛 GitHub Check: codecov/patch
internal/exec/terraform_plan_diff.go
[warning] 42-43: internal/exec/terraform_plan_diff.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 64-65: internal/exec/terraform_plan_diff.go#L64-L65
Added lines #L64 - L65 were not covered by tests
[warning] 124-127: internal/exec/terraform_plan_diff.go#L124-L127
Added lines #L124 - L127 were not covered by tests
[warning] 130-134: internal/exec/terraform_plan_diff.go#L130-L134
Added lines #L130 - L134 were not covered by tests
[warning] 136-139: internal/exec/terraform_plan_diff.go#L136-L139
Added lines #L136 - L139 were not covered by tests
[warning] 142-161: internal/exec/terraform_plan_diff.go#L142-L161
Added lines #L142 - L161 were not covered by tests
[warning] 163-164: internal/exec/terraform_plan_diff.go#L163-L164
Added lines #L163 - L164 were not covered by tests
[warning] 175-181: internal/exec/terraform_plan_diff.go#L175-L181
Added lines #L175 - L181 were not covered by tests
[warning] 184-187: internal/exec/terraform_plan_diff.go#L184-L187
Added lines #L184 - L187 were not covered by tests
[warning] 190-190: internal/exec/terraform_plan_diff.go#L190
Added line #L190 was not covered by tests
[warning] 208-234: internal/exec/terraform_plan_diff.go#L208-L234
Added lines #L208 - L234 were not covered by tests
[warning] 237-240: internal/exec/terraform_plan_diff.go#L237-L240
Added lines #L237 - L240 were not covered by tests
[warning] 243-243: internal/exec/terraform_plan_diff.go#L243
Added line #L243 was not covered by tests
[warning] 247-252: internal/exec/terraform_plan_diff.go#L247-L252
Added lines #L247 - L252 were not covered by tests
[warning] 255-282: internal/exec/terraform_plan_diff.go#L255-L282
Added lines #L255 - L282 were not covered by tests
[warning] 284-284: internal/exec/terraform_plan_diff.go#L284
Added line #L284 was not covered by tests
[warning] 299-299: internal/exec/terraform_plan_diff.go#L299
Added line #L299 was not covered by tests
[warning] 310-310: internal/exec/terraform_plan_diff.go#L310
Added line #L310 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
internal/exec/terraform_plan_diff.go (1)
247-285: Capturestderroutput for enhanced debugging.
Terraform may generate errors onstderr, so capturing onlystdoutcan hamper troubleshooting.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 247-252: internal/exec/terraform_plan_diff.go#L247-L252
Added lines #L247 - L252 were not covered by tests
[warning] 255-282: internal/exec/terraform_plan_diff.go#L255-L282
Added lines #L255 - L282 were not covered by tests
[warning] 284-284: internal/exec/terraform_plan_diff.go#L284
Added line #L284 was not covered by tests
what
Create a new command that shows what has changed between two plan files
why
Helps validate that an "approved plan" hasn't changed prior to deployment
example output
Windows-Specific Logic
This PR includes some error handling to deal with two distinct Windows-specific issues that caused intermittent test failures:
Process Termination & Goroutine Synchronization
Windows handles process termination differently than Unix systems. When intercepting os.Exit() calls in tests, Windows may not properly schedule the goroutine to completion, causing deadlocks when waiting for both exit codes and goroutine completion signals.
This specialized Windows path ensures we only wait for the exit code without expecting the goroutine to signal completion, avoiding deadlocks specific to Windows process termination behavior.
File System Timing Issues
Windows file systems sometimes experience timing inconsistencies where file handles aren't fully released or contents aren't completely flushed before subsequent operations start.
This small delay before the on-the-fly plan-diff ensures previous Terraform operations fully complete their file I/O before starting the next test phase, preventing race conditions in file access.
These changes significantly improve test reliability on Windows environments while preserving the original test behavior on Unix platforms.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation