Skip to content

Commit 46d13a3

Browse files
ostermanclaude
andauthored
fix: restore config import override behavior while maintaining Windows fix (#1489)
* fix: remove redundant MergeInConfig call that broke Windows tests The tempViper.MergeInConfig() call was attempting to merge the config into itself, which was causing YAML template functions to return null values on Windows. This bug was introduced in PR #1447 and caused test failures in: - TestYamlFuncTerraformOutput - TestProcessCustomYamlTags The fix removes this unnecessary call since we're already processing the config correctly by marshaling tempViper to YAML and then merging it into the main Viper instance. * fix: restore config import override behavior while maintaining Windows fix - Re-merge original config content after processing imports to ensure main config takes precedence - This fixes TestInitCliConfig/valid_import_custom_override while keeping Windows tests passing - Imports now correctly serve as base configuration that can be overridden by the main config The fix ensures proper config precedence: imports are processed first as a base layer, then the main config is applied on top as an override layer. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: clarify config merging comments for better understanding - Changed "original config" to "current config file's content" to be more precise - Clarified that we're re-applying the current file's settings on top of imported configs - Makes it clearer that the file being processed (which contains the imports) takes precedence 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * test: add comprehensive tests for config import override behavior - Added TestMergeConfig_ImportOverrideBehavior to verify main config overrides imports - Added TestMergeConfig_ImportDeepMerge to document section replacement behavior - Added TestMergeConfig_ProcessImportsWithInvalidYAML for error handling coverage - These tests increase coverage of the config merging logic and document expected behavior The tests confirm that: - Main config sections completely replace imported sections (not deep merge) - Invalid imports are logged but don't cause failures - The fix correctly prioritizes main config over imports 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * chore: add trim_trailing_whitespace to .editorconfig - Added trim_trailing_whitespace = true to the global [*] section - Added trim_trailing_whitespace = false for binary files to prevent corruption - Added explicit Go file configuration with tab indentation - This aligns .editorconfig with pre-commit hooks that already trim whitespace This ensures consistency between editor behavior and CI checks, reducing pre-commit hook failures caused by trailing whitespace. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: clarify config import precedence hierarchy in comments - Removed confusing "main config" terminology - Clearly explain that each config file's settings override its imports - Added concrete example: if A imports B, and B imports C, then B overrides C, and A overrides both - Updated function comment to explain the precedence hierarchy The comments now clearly convey that imports create a hierarchy where the importing file always takes precedence over imported files. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * test: improve config import tests with stronger assertions and godot compliance - Added periods to all test comments to satisfy godot linter - Strengthened command override assertion to verify replacement (not append): - Assert commands slice has exactly 1 element - Verify the single command is "main-command" from main config - Ensures imported commands were replaced, not merged - Replaced fragile empty string checks with v.IsSet() assertions: - Use assert.False(t, v.IsSet(...)) for absent keys - More reliable than checking for empty strings - Fixed all comment punctuation in test functions These changes make tests more robust and comply with Go linting standards. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * test: add tests to improve config merge coverage - Added TestMergeConfig_ReadFileError to test file read error handling - Tests the error path when os.ReadFile fails (lines 285-287) - Uses chmod to make file unreadable after initial load - Added TestMergeConfig_WithoutImports to test processImports=false path - Ensures code coverage when imports are not processed - Verifies config loads correctly without import processing These tests improve coverage of the mergeConfig function from ~45% to 83.3%. The remaining uncovered lines are edge cases difficult to simulate in tests. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: replace Windows-incompatible test with cross-platform coverage tests - Removed TestMergeConfig_ReadFileError which used chmod (fails on Windows) - Added TestMergeConfig_EmptyConfig to test edge case of empty config files - Added TestMergeConfig_ComplexImportHierarchy to test multi-level import chains - Tests A imports B, B imports C hierarchy - Verifies proper override precedence through import chains - Improves coverage of import processing paths These tests are cross-platform compatible and provide better coverage of the import processing logic without relying on OS-specific permissions. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor: break mergeConfig into smaller testable functions for better coverage - Extracted loadConfigFile() for loading config files into Viper - Extracted readConfigFileContent() for reading file contents - Extracted processConfigImportsAndReapply() for import processing logic - Extracted marshalViperToYAML() for YAML marshaling - Extracted mergeYAMLIntoViper() for merging YAML into Viper - Simplified mergeConfig() to orchestrate these smaller functions Added comprehensive tests for each new function: - 100% coverage for loadConfigFile, readConfigFileContent, mergeYAMLIntoViper - 85.7% coverage for processConfigImportsAndReapply - 80% coverage for marshalViperToYAML - 81.2% coverage for mergeConfig (up from 45%) This refactoring improves: - Code maintainability through smaller, focused functions - Test coverage from 67.6% to 68.0% overall - Makes error paths more testable - Keeps all existing functionality intact 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: add Windows file locking resilience for Terraform state operations - Add retry logic with exponential backoff for Windows file operations - Add small delays between Terraform workspace and output operations on Windows - Wrap os.Remove calls with retry logic to handle locked files - Use build tags to apply Windows-specific fixes only on Windows platform This addresses intermittent 'The process cannot access the file because another process has locked a portion of the file' errors when running multiple Terraform output operations in quick succession on Windows. * test: add comprehensive tests for Windows file locking resilience - Add platform-specific tests for retry logic on Windows - Add tests verifying no-op behavior on non-Windows platforms - Add integration tests for file operation scenarios - Test retry with exponential backoff timing - Test successful operations, retries, and failure scenarios Ensures the Windows file locking fixes are properly tested and maintain expected behavior across platforms. * fix: improve error wrapping in config load functions - Wrap preprocessAtmosYamlFunc error with 'preprocess YAML functions' context - Wrap tempViper.MergeConfig error with 'merge temp config' context - Wrap os.ReadFile error with file path context in readConfigFileContent - Handle viper.ConfigFileNotFoundError sentinel properly in loadConfigFile - Return sentinel unwrapped for type checking compatibility - Wrap other errors with descriptive context This ensures consistent error handling throughout the config loading process while preserving sentinel errors for proper error checking with errors.Is(). * fix: resolve test issues for better code quality - Fix variable shadowing in terraform_output_utils_windows_test.go - Renamed 'errors' slice to 'errList' to avoid shadowing errors package - Ensures errors.New() calls resolve correctly to the package - Use t.Skipf instead of t.Skip per project conventions - Updated terraform_output_utils_integration_test.go - Follows mandatory test skipping conventions requiring t.Skipf These fixes improve code clarity and maintain consistency with project guidelines. * refactor: reorganize config tests for better maintainability - Split large config_test.go (974 lines) into smaller, focused files - Create config_merge_test.go for merge-related tests (302 lines) - Create config_import_test.go for import-related tests (205 lines) - Reduce main config_test.go to 487 lines (~50% reduction) - Remove duplicate load_test.go that was causing compilation errors - Remove unused viper import from config_test.go This improves test organization, makes tests easier to find and maintain, and follows Go best practices of keeping test files focused and manageable. --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 3e2ed8a commit 46d13a3

12 files changed

Lines changed: 958 additions & 104 deletions

.editorconfig

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,31 @@
11
[*]
22
insert_final_newline = true
33
end_of_line = lf
4+
trim_trailing_whitespace = true
45

56
# Binary files (override to ensure no text-related rules are applied)
67
[*.{png,jpg,gif,svg,pdf,ai,eps,mp4}]
78
insert_final_newline = false
89
end_of_line = unset
910
indent_style = unset
1011
indent_size = unset
12+
trim_trailing_whitespace = false
1113

1214
# Override for machine generated binary HTML files in Screengrabs directory
1315
[website/src/components/Screengrabs/**/*.html]
1416
insert_final_newline = false
1517
end_of_line = unset
1618
indent_style = unset
1719
indent_size = unset
20+
trim_trailing_whitespace = false
1821

1922
# Override for machine generated binary files in tests/snapshots directory for golden snapshots
2023
[test/snapshots/**/*.golden]
2124
insert_final_newline = false
2225
end_of_line = unset
2326
indent_style = unset
2427
indent_size = unset
28+
trim_trailing_whitespace = false
2529

2630
# Override for Makefile
2731
[{Makefile,makefile,GNUmakefile}]
@@ -47,3 +51,8 @@ indent_size = 2
4751
[*.{tf,tfvars,tpl}]
4852
indent_style = space
4953
indent_size = 2
54+
55+
# Go files
56+
[*.go]
57+
indent_style = tab
58+
indent_size = 4

internal/exec/terraform_output_utils.go

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,9 @@ func execTerraformOutput(
252252
cfg.StackStr, stack,
253253
)
254254
}
255+
256+
// Add delay on Windows after workspace operations to prevent file locking
257+
windowsFileDelay()
255258
}
256259

257260
// Terraform output
@@ -260,10 +263,21 @@ func execTerraformOutput(
260263
cfg.ComponentStr, component,
261264
cfg.StackStr, stack,
262265
)
263-
outputMeta, err := tf.Output(ctx)
266+
267+
// Add small delay on Windows to prevent file locking issues
268+
windowsFileDelay()
269+
270+
// Wrap the output call with retry logic for Windows
271+
var outputMeta map[string]tfexec.OutputMeta
272+
err = retryOnWindows(func() error {
273+
var outputErr error
274+
outputMeta, outputErr = tf.Output(ctx)
275+
return outputErr
276+
})
264277
if err != nil {
265278
return nil, err
266279
}
280+
267281
log.Debug("Executed terraform output command",
268282
"command", fmt.Sprintf("terraform output %s -s %s", component, stack),
269283
cfg.ComponentStr, component,
Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
package exec
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"runtime"
7+
"testing"
8+
"time"
9+
10+
"github.com/stretchr/testify/assert"
11+
"github.com/stretchr/testify/require"
12+
)
13+
14+
func TestRetryOnWindows_FileOperations(t *testing.T) {
15+
// Create a temporary directory for testing.
16+
tmpDir, err := os.MkdirTemp("", "terraform_output_test")
17+
require.NoError(t, err)
18+
defer os.RemoveAll(tmpDir)
19+
20+
testFile := filepath.Join(tmpDir, "test.txt")
21+
22+
// Create a test file.
23+
err = os.WriteFile(testFile, []byte("test content"), 0o644)
24+
require.NoError(t, err)
25+
26+
// Test file deletion with retry logic.
27+
err = retryOnWindows(func() error {
28+
return os.Remove(testFile)
29+
})
30+
assert.NoError(t, err)
31+
32+
// Verify file was deleted.
33+
_, err = os.Stat(testFile)
34+
assert.True(t, os.IsNotExist(err), "File should be deleted")
35+
}
36+
37+
func TestRetryOnWindows_SimulatedLockingScenario(t *testing.T) {
38+
if runtime.GOOS != "windows" {
39+
t.Skipf("This test simulates Windows-specific file locking behavior")
40+
}
41+
42+
tmpDir, err := os.MkdirTemp("", "terraform_lock_test")
43+
require.NoError(t, err)
44+
defer os.RemoveAll(tmpDir)
45+
46+
testFile := filepath.Join(tmpDir, "state.tfstate")
47+
48+
// Create a test file.
49+
err = os.WriteFile(testFile, []byte("terraform state"), 0o644)
50+
require.NoError(t, err)
51+
52+
// Open the file to simulate it being locked.
53+
file, err := os.Open(testFile)
54+
require.NoError(t, err)
55+
56+
// Try to delete while file is open (this typically fails on Windows).
57+
deleteErr := os.Remove(testFile)
58+
59+
if deleteErr != nil {
60+
// File is locked, close it and try with retry logic.
61+
file.Close()
62+
63+
// Small delay to ensure handle is released.
64+
time.Sleep(50 * time.Millisecond)
65+
66+
// Now deletion with retry should work.
67+
err = retryOnWindows(func() error {
68+
return os.Remove(testFile)
69+
})
70+
assert.NoError(t, err, "Retry logic should handle file deletion after lock is released")
71+
} else {
72+
// If deletion succeeded immediately, just close the file.
73+
file.Close()
74+
}
75+
76+
// Verify file was deleted.
77+
_, err = os.Stat(testFile)
78+
assert.True(t, os.IsNotExist(err), "File should be deleted")
79+
}
80+
81+
func TestWindowsFileDelay_Timing(t *testing.T) {
82+
// Test that the delay function behaves correctly on the current platform.
83+
start := time.Now()
84+
windowsFileDelay()
85+
elapsed := time.Since(start)
86+
87+
if runtime.GOOS == "windows" {
88+
// On Windows, expect a delay.
89+
assert.GreaterOrEqual(t, elapsed.Milliseconds(), int64(90), "Expected at least 90ms delay on Windows")
90+
} else {
91+
// On other platforms, expect no significant delay.
92+
assert.Less(t, elapsed.Milliseconds(), int64(10), "Expected no significant delay on non-Windows")
93+
}
94+
}
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
//go:build !windows
2+
// +build !windows
3+
4+
package exec
5+
6+
// windowsFileDelay is a no-op on non-Windows platforms.
7+
func windowsFileDelay() {
8+
// No delay needed on Unix-like systems
9+
}
10+
11+
// retryOnWindows immediately executes the function on non-Windows platforms.
12+
func retryOnWindows(fn func() error) error {
13+
return fn()
14+
}
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
//go:build !windows
2+
// +build !windows
3+
4+
package exec
5+
6+
import (
7+
"errors"
8+
"testing"
9+
"time"
10+
11+
"github.com/stretchr/testify/assert"
12+
)
13+
14+
func TestWindowsFileDelay_NoOp(t *testing.T) {
15+
// Test that windowsFileDelay is a no-op on non-Windows platforms.
16+
start := time.Now()
17+
windowsFileDelay()
18+
elapsed := time.Since(start)
19+
20+
// Should have no significant delay on non-Windows platforms.
21+
assert.Less(t, elapsed.Milliseconds(), int64(10), "Expected no significant delay on non-Windows platforms")
22+
}
23+
24+
func TestRetryOnWindows_NoRetry(t *testing.T) {
25+
// Test that retryOnWindows executes immediately without retries on non-Windows.
26+
callCount := 0
27+
testErr := errors.New("test error")
28+
29+
// Test with error - should not retry on non-Windows.
30+
err := retryOnWindows(func() error {
31+
callCount++
32+
return testErr
33+
})
34+
35+
assert.Error(t, err)
36+
assert.Equal(t, testErr, err)
37+
assert.Equal(t, 1, callCount, "Function should be called exactly once on non-Windows platforms")
38+
}
39+
40+
func TestRetryOnWindows_SuccessNoRetry(t *testing.T) {
41+
// Test successful execution on non-Windows.
42+
callCount := 0
43+
44+
err := retryOnWindows(func() error {
45+
callCount++
46+
return nil
47+
})
48+
49+
assert.NoError(t, err)
50+
assert.Equal(t, 1, callCount, "Function should be called exactly once on non-Windows platforms")
51+
}
52+
53+
func TestRetryOnWindows_NoDelay(t *testing.T) {
54+
// Test that there's no delay even on error for non-Windows platforms.
55+
testErr := errors.New("test error")
56+
57+
start := time.Now()
58+
err := retryOnWindows(func() error {
59+
return testErr
60+
})
61+
elapsed := time.Since(start)
62+
63+
assert.Error(t, err)
64+
assert.Less(t, elapsed.Milliseconds(), int64(10), "Expected no delay on non-Windows platforms even with errors")
65+
}
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
//go:build windows
2+
// +build windows
3+
4+
package exec
5+
6+
import (
7+
"runtime"
8+
"time"
9+
)
10+
11+
// windowsFileDelay adds a small delay on Windows to allow file handles to be released.
12+
// This helps prevent "The process cannot access the file because another process has locked
13+
// a portion of the file" errors when multiple terraform operations run in quick succession.
14+
func windowsFileDelay() {
15+
if runtime.GOOS == "windows" {
16+
time.Sleep(100 * time.Millisecond)
17+
}
18+
}
19+
20+
// retryOnWindows wraps a function with retry logic for Windows file operations.
21+
// It will retry up to 3 times with increasing delays.
22+
func retryOnWindows(fn func() error) error {
23+
if runtime.GOOS != "windows" {
24+
return fn()
25+
}
26+
27+
var lastErr error
28+
delays := []time.Duration{100 * time.Millisecond, 200 * time.Millisecond, 500 * time.Millisecond}
29+
30+
for i := 0; i < len(delays); i++ {
31+
if err := fn(); err == nil {
32+
return nil
33+
} else {
34+
lastErr = err
35+
if i < len(delays)-1 {
36+
time.Sleep(delays[i])
37+
}
38+
}
39+
}
40+
41+
return lastErr
42+
}
Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
//go:build windows
2+
// +build windows
3+
4+
package exec
5+
6+
import (
7+
"errors"
8+
"testing"
9+
"time"
10+
11+
"github.com/stretchr/testify/assert"
12+
)
13+
14+
func TestWindowsFileDelay(t *testing.T) {
15+
// Test that windowsFileDelay introduces a delay on Windows.
16+
start := time.Now()
17+
windowsFileDelay()
18+
elapsed := time.Since(start)
19+
20+
// Should have at least 100ms delay on Windows.
21+
assert.GreaterOrEqual(t, elapsed.Milliseconds(), int64(90), "Expected at least 90ms delay on Windows")
22+
assert.LessOrEqual(t, elapsed.Milliseconds(), int64(150), "Expected no more than 150ms delay on Windows")
23+
}
24+
25+
func TestRetryOnWindows_Success(t *testing.T) {
26+
// Test successful execution on first try.
27+
callCount := 0
28+
err := retryOnWindows(func() error {
29+
callCount++
30+
return nil
31+
})
32+
33+
assert.NoError(t, err)
34+
assert.Equal(t, 1, callCount, "Function should be called exactly once on success")
35+
}
36+
37+
func TestRetryOnWindows_RetryThenSuccess(t *testing.T) {
38+
// Test retry logic - fail twice, then succeed.
39+
callCount := 0
40+
testErr := errors.New("temporary error")
41+
42+
start := time.Now()
43+
err := retryOnWindows(func() error {
44+
callCount++
45+
if callCount < 3 {
46+
return testErr
47+
}
48+
return nil
49+
})
50+
elapsed := time.Since(start)
51+
52+
assert.NoError(t, err)
53+
assert.Equal(t, 3, callCount, "Function should be called 3 times (2 failures + 1 success)")
54+
// Should have delays: 100ms after first failure, 200ms after second failure.
55+
assert.GreaterOrEqual(t, elapsed.Milliseconds(), int64(250), "Expected at least 250ms total delay for 2 retries")
56+
}
57+
58+
func TestRetryOnWindows_AllFailures(t *testing.T) {
59+
// Test when all retries fail.
60+
callCount := 0
61+
testErr := errors.New("persistent error")
62+
63+
start := time.Now()
64+
err := retryOnWindows(func() error {
65+
callCount++
66+
return testErr
67+
})
68+
elapsed := time.Since(start)
69+
70+
assert.Error(t, err)
71+
assert.Equal(t, testErr, err, "Should return the last error")
72+
assert.Equal(t, 3, callCount, "Function should be called 3 times (max retries)")
73+
// Should have delays: 100ms + 200ms = 300ms minimum.
74+
assert.GreaterOrEqual(t, elapsed.Milliseconds(), int64(250), "Expected at least 250ms total delay for all retries")
75+
}
76+
77+
func TestRetryOnWindows_DifferentErrors(t *testing.T) {
78+
// Test that the last error is returned when different errors occur.
79+
callCount := 0
80+
errList := []error{
81+
errors.New("first error"),
82+
errors.New("second error"),
83+
errors.New("final error"),
84+
}
85+
86+
err := retryOnWindows(func() error {
87+
err := errList[callCount]
88+
callCount++
89+
return err
90+
})
91+
92+
assert.Error(t, err)
93+
assert.Equal(t, errList[2], err, "Should return the last error")
94+
assert.Equal(t, 3, callCount, "Function should be called 3 times")
95+
}

0 commit comments

Comments
 (0)