chore: migrated to golangci-lint v2 #4333
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough""" WalkthroughThis change upgrades the golangci-lint tool and its configuration files to version 2, significantly expands and restructures the enabled linters, and modernizes related settings and exclusions. Additionally, it updates all test code to use the test framework's context ( Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant Context as t.Context()/b.Context()
participant Target as Function Under Test
Test->>Context: Obtain test/benchmark context
Test->>Target: Call function with Context
Target->>Target: Execute logic using provided Context
Target-->>Test: Return result/errors
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (14)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (12)
cli/commands/run/cli_test.go (1)
39-39:⚠️ Potential issueFix the remaining linting error
There's a golangci-lint "copyloopvar" error reported for this line that hasn't been addressed. Since Go 1.22+, loop variables are no longer shared across iterations, so the explicit variable copy is unnecessary.
- tc := tcThis change is needed to resolve the pipeline failure:
[error] 39-39: golangci-lint copyloopvar: The copy of the 'for' variable "tc" can be deleted (Go 1.22+)
🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 39-39: golangci-lint copyloopvar: The copy of the 'for' variable "tc" can be deleted (Go 1.22+)
internal/services/catalog/module/repo_test.go (1)
14-86: 💡 Verification agent🧩 Analysis chain
Address linting issues with function length
The pipeline is reporting a linting error: "golangci-lint funlen: multiple functions are too long (exceeding 60 lines)." Consider refactoring long functions like
TestFindModulesinto smaller, more focused test functions.
🏁 Script executed:
#!/bin/bash # Find exact function length to better understand the linting error echo "Function length of TestFindModules:" cat internal/services/catalog/module/repo_test.go | awk 'BEGIN {count=0; in_func=0} /^func TestFindModules/ {in_func=1} in_func==1 {count++} /^}/ && in_func==1 {in_func=0} END {print count}'Length of output: 276
Refactor
TestFindModulesto Comply withfunlen(60-line) LimitThe
TestFindModulesfunction is currently 71 lines long—exceeding the golangci-lintfunlenthreshold of 60 lines. Please break it into smaller, more focused tests or helpers to satisfy the linter:• File:
internal/services/catalog/module/repo_test.go, functionTestFindModules(starts at line 14)
• Consider extracting setup/teardown logic (e.g., the.gitrename) into a helper function
• Split the single large test into multiple subtests or separate test functions for different scenarios (e.g., happy-path vs. edge cases)
• Move data-building (themoduleDataslice) into a constructor/helper to reduce inline boilerplate🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 14-86: golangci-lint funlen: multiple functions are too long (exceeding 60 lines).
cli/commands/common/runall/runall_test.go (1)
27-27: 🛠️ Refactor suggestionRemove debug print statement
The linter is flagging line 27 with "golangci-lint forbidigo: use of
fmt.Printlnforbidden by pattern". This appears to be a debug print statement that should be removed from the test.- fmt.Println(err, errors.Unwrap(err))🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 27-27: golangci-lint forbidigo: use of
fmt.Printlnforbidden by pattern.internal/cas/integration_test.go (1)
15-99: 💡 Verification agent🧩 Analysis chain
Function is too long according to the linter
The
TestIntegration_CloneAndReusefunction exceeds the golangci-lint function length limit (77 > 60 lines).Consider splitting this into smaller test functions by moving each test case (t.Run block) to a separate helper function:
🏁 Script executed:
#!/bin/bash # Verify the function length error rg -A 1 funlen .golangci.ymlLength of output: 30
🏁 Script executed:
#!/bin/bash # Locate golangci-lint configuration and check for funlen settings set -euo pipefail echo "Searching for golangci-lint config files..." cfg_files=$(fd -HI -t f -i 'golangci.*') if [ -z "$cfg_files" ]; then echo "No golangci-lint config file found in the repository." exit 0 fi echo "Found config files:" echo "$cfg_files" echo for file in $cfg_files; do echo "---- $file ----" echo "First 200 lines of $file:" sed -n '1,200p' "$file" echo echo "Looking for 'funlen' in $file:" grep -Rin 'funlen' "$file" || echo " funlen not configured in $file" echo doneLength of output: 6927
Split long test function to satisfy funlen (>60 lines)
The
TestIntegration_CloneAndReuseininternal/cas/integration_test.gois 77 lines, exceeding the 60-line funlen limit configured in.strict.golangci.yml. Extract eacht.Runblock into its own top-level test to bring each function under the threshold:• internal/cas/integration_test.go
– TestIntegration_CloneSameRepoUsesStore
– TestIntegration_CloneNonexistentBranchFailsGracefully
– TestIntegration_CloneInvalidRepositoryFailsGracefullyExample skeleton:
func TestIntegration_CloneSameRepoUsesStore(t *testing.T) { t.Parallel() // …first-t.Run logic… } func TestIntegration_CloneNonexistentBranchFailsGracefully(t *testing.T) { t.Parallel() // …second-t.Run logic… } func TestIntegration_CloneInvalidRepositoryFailsGracefully(t *testing.T) { t.Parallel() // …third-t.Run logic… }This refactor will reduce each test below the 60-line limit.
🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 15-15: golangci-lint funlen: function is too long (77 > 60).
test/integration_debug_test.go (1)
233-504:⚠️ Potential issueFix unchecked type assertions
The linter is reporting unchecked type assertions in this section of code. The forcetypeassert rule requires all type assertions to have their success checked.
Search for patterns like
x.(map[string]any)in this code section and update them to check the assertion result:-value := someMap.(map[string]any) +value, ok := someMap.(map[string]any) +if !ok { + t.Fatalf("expected map[string]any, got %T", someMap) +}You can verify specific instances with:
#!/bin/bash # Find unchecked type assertions in the file rg "\.\(map\[string\]any\)" test/integration_debug_test.go🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 233-504: golangci-lint forcetypeassert: multiple type assertions must be checked.
internal/services/catalog/catalog_test.go (1)
46-46:⚠️ Potential issueFix dynamic error creation to comply with err113 linter
The pipeline is failing with a golangci-lint err113 error which requires using wrapped static errors instead of dynamic errors created with fmt.Errorf().
Implement this change to fix the linter error:
- return nil, fmt.Errorf("unexpected repoURL in mock newRepoFunc: %s", repoURL) + return nil, errors.Errorf("unexpected repoURL in mock newRepoFunc: %s", repoURL)Similar changes are needed for the other dynamic error creations at lines 109 and 162.
Also applies to: 109-109, 162-162
🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 46-162: golangci-lint err113: do not define dynamic errors, use wrapped static errors instead.
tf/getter_test.go (1)
37-102: 💡 Verification agent🧩 Analysis chain
Function length exceeds linter limits
The pipeline failure indicates this function is too long (64 lines > 60 line limit).
Consider refactoring to reduce length, possibly by extracting common test setup logic into helper functions, or requesting an exemption if this is an acceptable case of a longer test function.
🏁 Script executed:
#!/bin/bash # Check the actual function length against the linter limit echo "Function length is $(expr 102 - 37 + 1) lines, linter limit is 60 lines"Length of output: 137
TestGetDownloadURLFromHeader exceeds maximum function length
The test spans 66 lines (lines 37–102), exceeding the linter’s 60-line limit.• File: tf/getter_test.go (lines 37–102)
Consider extracting common setup or assertion logic into helper functions (e.g., a
runDownloadURLTesthelper or moving thetestCasesslice to a separate function/variable) to reduce the length, or request an exception if this level of detail is warranted.🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 37-37: golangci-lint funlen: function is too long (64 > 60).
cli/commands/list/list_test.go (1)
178-467: 💡 Verification agent🧩 Analysis chain
Consider addressing the linter warning about long functions
The pipeline is reporting that multiple functions are too long (exceeding 60 lines). While this issue pre-dates your changes, consider refactoring these test functions into smaller, more focused units in a future PR.
🏁 Script executed:
#!/bin/bash # Find the specific functions that are reported as too long rg -n "func Test.*DAGSorting.*\(" cli/commands/list/list_test.go | awk '{print $1, $2}'Length of output: 224
Refactor lengthy test functions for readability
The following test functions in cli/commands/list/list_test.go each exceed 60 lines and trigger the linter warning. While this predates your changes, consider splitting them into smaller, focused helpers (e.g., setup, execution, and assertions) in a future PR:
• Line 178: TestDAGSortingSimpleDependencies
• Line 255: TestDAGSortingReversedDependencies
• Line 350: TestDAGSortingComplexDependenciesNo code changes are required for this PR, but extracting common setup and assertion logic will improve maintainability going forward.
internal/cas/benchmark_test.go (1)
26-41: 💡 Verification agent🧩 Analysis chain
Consider using integer range in for loops.
Go 1.22+ supports using integer ranges in for loops. Consider updating these loops for better readability.
For example:
- for i := 0; i < b.N; i++ { + for i := range b.N {This applies to multiple for loops in the file.
Also applies to: 63-77
🌐 Web query:
When was Go 1.22 released and what new features does it include for integer ranges in for loops?💡 Result:
Go 1.22 Release Date
Go 1.22 was officially released on February 6, 2024[3][4][10].
New Features for Integer Ranges in
forLoopsGo 1.22 introduced significant enhancements to how
forloops work, especially regarding integer ranges:1. Range Over Integers
Starting with Go 1.22, you can use the
rangekeyword to loop directly over an integer value. This allows you to write concise loops like:for i := range 10 { fmt.Println(i) }This loop iterates from
i = 0up toi = 9, producing the values in increasing order. If the value is less than or equal to zero, the loop body is not executed at all. This construct is functionally equivalent to the classic:for i := 0; i < 10; i++ { // body }but is shorter and less error-prone[1][3][4][7][9][10].
2. Per-Iteration Loop Variables
Go 1.22 changed the semantics of variables declared in a
forloop. Now, in bothfor rangeand the traditionalforloop with declaration (:=), a new variable is created for each iteration. This prevents the common "closure capture" bug, where all goroutines accidentally share and mutate a single loop variable. With this change, each iteration has an independent copy of the loop variable, making code safer and more predictable[1][3][4][6][7][10].3. Preview: Range-Over-Function Iterators
Go 1.22 also includes a preview (opt-in via
GOEXPERIMENT=rangefunc) of a new language feature allowingfor rangeto iterate over functions that yield values. This is a preview and not yet part of the stable language, but shows ongoing development in Go's iteration capabilities[1][10].Summary Table: Integer Range in Go 1.22
Classic Loop Syntax Go 1.22 Integer Range Behavior for i := 0; i < 10; i++ {for i := range 10 {i = 0 to 9 (No change for n ≤ 0) for i := range n {(n ≤ 0)Loop does not run References
- Go 1.22 Release Notes[1]
- Official Go Blog Release Announcement[3]
- Applied Go and ByteSizeGo feature overviews[4][10]
- Interactive explanations and examples[7]
- Language specification update on integer ranges[9]
In summary:
Go 1.22, released on February 6, 2024, introduced the ability to range directly over integers inforloops and changed loop variable scoping to create new variables per iteration, addressing longstanding issues and making loop constructs more expressive and reliable[1][3][4][7][9][10].Citations:
- 1: https://tip.golang.org/doc/go1.22
- 2: https://go.dev/doc/devel/release
- 3: https://go.dev/blog/go1.22
- 4: https://appliedgo.com/blog/the-last-article-about-go-1-22-you-need-to-read
- 5: https://www.youtube.com/watch?v=cBB4Mgn6V5E
- 6: https://go101.org/blog/2024-03-01-for-loop-semantic-changes-in-go-1.22.html
- 7: https://antonz.org/go-1-22/
- 8: https://sig-synopsys.my.site.com/community/s/article/When-will-golang-1-22-be-supported
- 9: https://boldlygo.tech/archive/2024-07-03-iteration-over-integers/
- 10: https://www.bytesizego.com/blog/everything-you-need-go-122
🏁 Script executed:
#!/bin/bash # Check the Go version specified in the module if [ -f go.mod ]; then grep -E "^go [0-9]+\.[0-9]+" -n go.mod else echo "No go.mod found in the repository root." fiLength of output: 70
Update benchmark loops to use integer‐range syntax
The module is on Go 1.24 (see go.mod), so you can leverage the Go 1.22+ feature to range directly over an integer. This replaces
for i := 0; i < b.N; i++with the more concise and safer:- for i := 0; i < b.N; i++ { + for i := range b.N {This change applies here:
- internal/cas/benchmark_test.go: lines 26–41
- internal/cas/benchmark_test.go: lines 63–77
🧰 Tools
🪛 GitHub Actions: Strict Lint
[warning] 26-182: golangci-lint intrange: for loops can be changed to use an integer range (Go 1.22+).
configstack/module_test.go (1)
332-1281: 💡 Verification agent❓ Verification inconclusive
Address golangci-lint error handling issue.
The linter reports an err113 error: "do not define dynamic errors, use wrapped static errors instead." Consider defining error variables at package level and using them throughout the code.
🏁 Script executed:
#!/bin/bash # Find examples of how errors are handled in the codebase for guidance rg "var \w+Err = errors\.New\(" --type goLength of output: 43
Extract dynamic errors to static package-level variables
The golangci-lint err113 failure is caused by calls to
errors.Newinside test functions (e.g.,expectedErrA := errors.New("…")). To fix:
- In
configstack/module_test.go, add a top-of-file block of package-level error vars:var ( ErrExpectedModuleA = errors.New("Expected error for module a") ErrExpectedModuleB = errors.New("Expected error for module b") ErrExpectedModuleC = errors.New("Expected error for module c") // …add for each “Expected error for module X” )- In each test, replace the inline call with the static var:
- expectedErrA := errors.New("Expected error for module a") + expectedErrA := ErrExpectedModuleA- Repeat for
Expected error for module b,module c, etc.Alternatively, if you’d rather keep dynamic errors in tests, add a nolint directive in this file or configure golangci-lint to skip err113 for
_test.gofiles.🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 332-1281: golangci-lint err113: do not define dynamic errors, use wrapped static errors instead.
[error] 1100-1172: golangci-lint funlen: multiple functions are too long (exceeding 60 lines).
test/integration_test.go (2)
3802-3802:⚠️ Potential issueFix dogsled linter warning by naming unused return values or using fewer underscores
This line is triggering the "dogsled" linter warning because it uses 3 blank identifiers in a row. Consider either naming these variables or refactoring the code to use a helper function that only returns the error.
-_, _, _, err := testRunAllPlan(t, "--json-out-dir "+tmpDir, "") +// Option 1: Name the unused return values +stdout, stderr, planOutput, err := testRunAllPlan(t, "--json-out-dir "+tmpDir, "") +// If unused, add _ prefix to indicate they're intentionally unused +_stdout, _stderr, _planOutput, err := testRunAllPlan(t, "--json-out-dir "+tmpDir, "") +// Option 2: Create a helper function that only returns the error +err := testRunAllPlanIgnoreOutput(t, "--json-out-dir "+tmpDir, "")🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 3802-3802: golangci-lint dogsled: declaration has 3 blank identifiers.
1405-2504:⚠️ Potential issueAdd type assertion error checking for multiple type assertions
The code in this range contains multiple unchecked type assertions. According to the golangci-lint forcetypeassert rule, you should check the success of type assertions to avoid potential panics.
For example, look for patterns like
value.(Type)and replace them with:if value, ok := value.(Type); ok { // use value safely } else { // handle error case }This will make the code more robust by preventing runtime panics when type assertions fail.
🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 1405-2504: golangci-lint forcetypeassert: multiple type assertions must be checked.
🧹 Nitpick comments (13)
internal/cli/app.go (2)
192-217: Consider addressing the cyclomatic complexity warning.The pipeline reports that the
setupAutocompletefunction has a cyclomatic complexity of 12, exceeding the maximum of 10. While this PR makes a good improvement to the switch statement, you could further refactor this function to reduce its complexity.You could extract the flag checking logic into a separate function:
func (app *App) setupAutocomplete(arguments []string) error { var ( isAutocompleteInstall bool isAutocompleteUninstall bool ) if app.AutocompleteInstallFlag == "" { app.AutocompleteInstallFlag = defaultAutocompleteInstallFlag } if app.AutocompleteUninstallFlag == "" { app.AutocompleteUninstallFlag = defaultAutocompleteUninstallFlag } if app.AutocompleteInstaller == nil { app.AutocompleteInstaller = &autocompleteInstaller{} } - for _, arg := range arguments { - switch arg { - case "-" + app.AutocompleteInstallFlag, "--" + app.AutocompleteInstallFlag: - isAutocompleteInstall = true - case "-" + app.AutocompleteUninstallFlag, "--" + app.AutocompleteUninstallFlag: - isAutocompleteUninstall = true - } - } + isAutocompleteInstall, isAutocompleteUninstall = app.checkAutocompleteFlags(arguments) // Remaining function code... } +func (app *App) checkAutocompleteFlags(arguments []string) (install bool, uninstall bool) { + for _, arg := range arguments { + switch arg { + case "-" + app.AutocompleteInstallFlag, "--" + app.AutocompleteInstallFlag: + install = true + case "-" + app.AutocompleteUninstallFlag, "--" + app.AutocompleteUninstallFlag: + uninstall = true + } + } + return +}🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 192-192: golangci-lint cyclop: calculated cyclomatic complexity for function setupAutocomplete is 12, max is 10.
173-237: Consider addressing the wrapcheck warning.The pipeline indicates that errors returned from external packages are not being wrapped in the
NewRootCommandmethod. It's a good practice to wrap errors with context before returning them to aid in debugging.// For error returns like this: err := app.AutocompleteInstaller.Install(app.Name) - return NewExitError(err, 0) + return NewExitError(errors.Wrap(err, "failed to install autocomplete"), 0) err := app.AutocompleteInstaller.Uninstall(app.Name) - return NewExitError(err, 0) + return NewExitError(errors.Wrap(err, "failed to uninstall autocomplete"), 0)🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 192-192: golangci-lint cyclop: calculated cyclomatic complexity for function setupAutocomplete is 12, max is 10.
[warning] 173-237: golangci-lint wrapcheck: error returned from external package is unwrapped; should be wrapped.
cli/commands/find/find.go (2)
19-118: Consider refactoring to reduce cyclomatic complexity.The
Runfunction has a cyclomatic complexity of 18, which exceeds the maximum of 10 set by golangci-lint. Consider breaking it down into smaller, more focused functions to improve maintainability and testability.For example, you could extract the logic for different modes into separate functions:
func Run(ctx context.Context, opts *Options) error { d := configureDiscovery(opts) cfgs, err := discoverConfigurations(ctx, d, opts) if err != nil { return err } cfgs, err = processByMode(ctx, opts, cfgs) if err != nil { return err } foundCfgs, err := convertToFoundConfigs(ctx, cfgs, opts) if err != nil { return err } return outputByFormat(opts, foundCfgs) } func configureDiscovery(opts *Options) *discovery.Discovery { // Discovery configuration logic } func processByMode(ctx context.Context, opts *Options, cfgs discovery.DiscoveredConfigs) (discovery.DiscoveredConfigs, error) { switch opts.Mode { case ModeNormal: return cfgs.Sort(), nil case ModeDAG: // DAG mode logic default: return nil, errors.New("invalid mode: " + opts.Mode) } } func outputByFormat(opts *Options, foundCfgs FoundConfigs) error { switch opts.Format { case FormatText: return outputText(opts, foundCfgs) case FormatJSON: return outputJSON(opts, foundCfgs) default: return errors.New("invalid format: " + opts.Format) } }Can this refactoring be considered in a future PR? The current PR focuses on fixing linter errors, but this could improve code maintainability.
🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 19-131: golangci-lint cyclop: calculated cyclomatic complexity for functions Run and discoveredToFound are 18 and 16 respectively, max is 10.
131-187: Consider refactoring to reduce cyclomatic complexity.The
discoveredToFoundfunction has a cyclomatic complexity of 16, which exceeds the maximum of 10 set by golangci-lint. Consider extracting parts of the logic into helper functions.For example, you could extract the code that processes a single config:
func discoveredToFound(configs discovery.DiscoveredConfigs, opts *Options) (FoundConfigs, error) { foundCfgs := make(FoundConfigs, 0, len(configs)) errs := []error{} for _, config := range configs { foundCfg, err := processConfig(config, opts) if err != nil { errs = append(errs, err) continue } if foundCfg != nil { foundCfgs = append(foundCfgs, foundCfg) } } return foundCfgs, errors.Join(errs...) } func processConfig(config discovery.DiscoveredConfig, opts *Options) (*FoundConfig, error) { if config.External && !opts.External { return nil, nil } if opts.QueueConstructAs != "" { if config.Parsed != nil && config.Parsed.Exclude != nil { if config.Parsed.Exclude.IsActionListed(opts.QueueConstructAs) { return nil, nil } } } relPath, err := filepath.Rel(opts.WorkingDir, config.Path) if err != nil { return nil, errors.New(err) } foundCfg := &FoundConfig{ Type: config.Type, Path: relPath, } if opts.Exclude && config.Parsed != nil && config.Parsed.Exclude != nil { foundCfg.Exclude = config.Parsed.Exclude.Clone() } if !opts.Dependencies || len(config.Dependencies) == 0 { return foundCfg, nil } foundCfg.Dependencies = make([]string, len(config.Dependencies)) for i, dep := range config.Dependencies { relDepPath, err := filepath.Rel(opts.WorkingDir, dep.Path) if err != nil { return foundCfg, errors.New(err) } foundCfg.Dependencies[i] = relDepPath } return foundCfg, nil }cli/commands/scaffold/scaffold_test.go (1)
19-94: Consider breaking down the test function.The test function is 72 lines long, exceeding the golangci-lint limit of 60 lines. Consider breaking it down into smaller, focused test functions or extracting setup logic into helper functions.
For example:
func TestDefaultTemplateVariables(t *testing.T) { t.Parallel() vars := setupTemplateVariables() workDir, templateDir, outputDir := setupDirectories(t) setupTemplateFiles(t, templateDir) boilerplateOpts := setupBoilerplateOptions(outputDir, templateDir, vars) emptyDep := variables.Dependency{} err := templates.ProcessTemplate(boilerplateOpts, boilerplateOpts, emptyDep) require.NoError(t, err) verifyTemplateOutput(t, outputDir) } func setupTemplateVariables() map[string]any { vars := map[string]any{} // Set up required and optional variables // ... return vars } func setupDirectories(t *testing.T) (workDir, templateDir, outputDir string) { // Setup directories // ... return workDir, templateDir, outputDir } func setupTemplateFiles(t *testing.T, templateDir string) { // Write template files // ... } func setupBoilerplateOptions(outputDir, templateDir string, vars map[string]any) *boilerplateoptions.BoilerplateOptions { // Setup boilerplate options // ... return boilerplateOpts } func verifyTemplateOutput(t *testing.T, outputDir string) { // Read and verify generated content // ... }🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 19-19: golangci-lint funlen: function is too long (72 > 60).
internal/cas/benchmark_test.go (2)
127-189: Consider refactoring to reduce cyclomatic complexity.The
BenchmarkGitOperationsfunction has a cyclomatic complexity of 11, exceeding the maximum of 10 set by golangci-lint. Consider extracting the benchmark sub-tests into separate functions.You could refactor by using sub-benchmark helpers:
func BenchmarkGitOperations(b *testing.B) { // Setup a git repository for testing repoDir := b.TempDir() git := cas.NewGitRunner().WithWorkDir(repoDir) ctx := b.Context() if err := git.Clone(ctx, "https://github.com/gruntwork-io/terragrunt.git", false, 1, "main"); err != nil { b.Fatal(err) } benchmarkLsRemote(b, ctx) benchmarkLsTree(b, ctx, git) benchmarkCatFile(b, ctx, git) } func benchmarkLsRemote(b *testing.B, ctx context.Context) { b.Run("ls-remote", func(b *testing.B) { git := cas.NewGitRunner() // No workDir needed for ls-remote b.ResetTimer() for i := 0; i < b.N; i++ { _, err := git.LsRemote(ctx, "https://github.com/gruntwork-io/terragrunt.git", "HEAD") if err != nil { b.Fatal(err) } } }) } func benchmarkLsTree(b *testing.B, ctx context.Context, git *cas.GitRunner) { b.Run("ls-tree", func(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { _, err := git.LsTree(ctx, "HEAD", ".") if err != nil { b.Fatal(err) } } }) } func benchmarkCatFile(b *testing.B, ctx context.Context, git *cas.GitRunner) { b.Run("cat-file", func(b *testing.B) { // First get a valid hash tree, err := git.LsTree(ctx, "HEAD", ".") if err != nil { b.Fatal(err) } if len(tree.Entries()) == 0 { b.Fatal("no entries in tree") } hash := tree.Entries()[0].Hash tmpFile := b.TempDir() + "/cat-file" tmp, err := os.Create(tmpFile) if err != nil { b.Fatal(err) } defer os.Remove(tmpFile) defer tmp.Close() b.ResetTimer() for i := 0; i < b.N; i++ { err := git.CatFile(ctx, hash, tmp) if err != nil { b.Fatal(err) } } }) }🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 127-127: golangci-lint cyclop: calculated cyclomatic complexity for function BenchmarkGitOperations is 11, max is 10.
16-80: Consider refactoring long benchmark function.The
BenchmarkClonefunction is 61 lines long, slightly exceeding the golangci-lint limit of 60 lines. Consider extracting helper functions to improve readability.For example:
func BenchmarkClone(b *testing.B) { // Use a small, public repository for consistent results repo := "https://github.com/gruntwork-io/terragrunt.git" l := log.New() benchmarkFreshClone(b, repo, l) benchmarkCloneWithExistingStore(b, repo, l) } func benchmarkFreshClone(b *testing.B, repo string, l log.Logger) { b.Run("fresh clone", func(b *testing.B) { tempDir := b.TempDir() b.ResetTimer() for i := 0; i < b.N; i++ { storePath := filepath.Join(tempDir, "store", strconv.Itoa(i)) targetPath := filepath.Join(tempDir, "repo", strconv.Itoa(i)) c, err := cas.New(cas.Options{ StorePath: storePath, }) if err != nil { b.Fatal(err) } if err := c.Clone(b.Context(), &l, &cas.CloneOptions{ Dir: targetPath, }, repo); err != nil { b.Fatal(err) } } }) } func benchmarkCloneWithExistingStore(b *testing.B, repo string, l log.Logger) { b.Run("clone with existing store", func(b *testing.B) { // Implementation... }) }🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 16-16: golangci-lint funlen: function is too long (61 > 60).
configstack/module_test.go (1)
1100-1172: Consider refactoring long functions for better maintainability.The linter reports that several functions exceed the recommended length of 60 lines. Consider breaking down these functions into smaller, more focused helper functions.
🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 1100-1172: golangci-lint funlen: multiple functions are too long (exceeding 60 lines).
cli/commands/hcl/format/format_test.go (1)
52-270: Update loop variable handling for Go 1.22+ compatibility.The linter reports a copyloopvar issue: "The copy of the 'for' variable can be deleted (Go 1.22+)." In Go 1.22+, the language fixed the loop variable capture issue in goroutines, making explicit variable copies unnecessary.
Apply this pattern where the loop variable is captured:
for _, dir := range dirs { - // Capture range variable into for block so it doesn't change while looping - dir := dir t.Run(dir, func(t *testing.T) { t.Parallel() // Rest of the code... }) }🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 52-270: golangci-lint copyloopvar: The copy of the 'for' variable can be deleted (Go 1.22+).
cli/commands/list/list.go (1)
22-534: Consider refactoring complex functions to reduce cyclomatic complexity.Several functions (Run, discoveredToListed, renderLong, generateDAGTree) have a cyclomatic complexity greater than 10, making them harder to test and maintain. Consider breaking these functions into smaller, more focused ones.
For example, in the
Runfunction, you could extract separate functions for:
- Configuring the discovery options
- Processing discovered configurations
- Handling different output formats
🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 22-534: golangci-lint cyclop: multiple functions (Run, discoveredToListed, renderLong, generateDAGTree) have cyclomatic complexity > 10.
test/integration_test.go (1)
295-624: Consider refactoring long functions to improve readabilityMultiple functions in this range exceed the recommended 60-line limit set by the funlen linter. Consider breaking down large test functions into smaller helper functions or table-driven tests to improve readability and maintainability.
For example, the
TestLogCustomFormatOutputfunction could benefit from extracting the test case setup and validation logic into separate helper functions.🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 295-624: golangci-lint funlen: multiple functions are too long (exceeding 60 lines).
.golangci.yml (2)
14-57: Expand enabled lint rules
This PR enables a broad set of new linters. While comprehensive coverage is beneficial, consider rolling out such a large change in phases to reduce noise and help the team adapt to new warnings.
61-66: Clarify disabled linter rationale
You’ve disabledrecvcheckdue to an unresolved issue. It would be helpful to add a short comment explaining the specific problem or link to an issue ticket for future reference.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (68)
.github/workflows/strict-lint.yml(1 hunks).golangci.yml(1 hunks).strict.golangci.yml(1 hunks)Makefile(1 hunks)cli/commands/common/runall/runall_test.go(1 hunks)cli/commands/dag/graph/cli_test.go(1 hunks)cli/commands/find/find.go(1 hunks)cli/commands/find/find_test.go(1 hunks)cli/commands/hcl/format/format_test.go(7 hunks)cli/commands/list/list.go(1 hunks)cli/commands/list/list_test.go(5 hunks)cli/commands/render/render_test.go(5 hunks)cli/commands/run/cli_test.go(1 hunks)cli/commands/run/download_source_test.go(3 hunks)cli/commands/run/run.go(1 hunks)cli/commands/run/run_test.go(1 hunks)cli/commands/scaffold/scaffold_test.go(1 hunks)cli/flags/flag_test.go(1 hunks)cli/help_test.go(1 hunks)config/cache_test.go(1 hunks)config/catalog_test.go(1 hunks)config/config.go(1 hunks)config/config_as_cty_test.go(2 hunks)config/config_helpers_test.go(23 hunks)config/config_partial_test.go(15 hunks)config/config_test.go(40 hunks)config/dependency_test.go(1 hunks)config/locals_test.go(4 hunks)config/stack_test.go(3 hunks)configstack/module_test.go(27 hunks)configstack/stack_test.go(26 hunks)configstack/test_helpers_test.go(3 hunks)internal/cache/cache_test.go(3 hunks)internal/cas/benchmark_test.go(4 hunks)internal/cas/cas_test.go(3 hunks)internal/cas/getter_test.go(3 hunks)internal/cas/git_test.go(3 hunks)internal/cas/integration_test.go(5 hunks)internal/cas/race_test.go(1 hunks)internal/cas/tree_test.go(1 hunks)internal/cli/app.go(1 hunks)internal/cli/command_test.go(1 hunks)internal/discovery/discovery_test.go(3 hunks)internal/providercache/provider_cache_test.go(1 hunks)internal/remotestate/backend/s3/client_test.go(6 hunks)internal/services/catalog/catalog_test.go(6 hunks)internal/services/catalog/module/repo_test.go(1 hunks)internal/strict/control_test.go(1 hunks)mise.toml(1 hunks)shell/run_cmd_output_test.go(2 hunks)shell/run_cmd_test.go(4 hunks)shell/run_cmd_unix_test.go(1 hunks)shell/run_cmd_windows_test.go(1 hunks)telemetry/meter_test.go(1 hunks)telemetry/tracer_test.go(1 hunks)test/benchmarks/helpers/helpers.go(1 hunks)test/helpers/package.go(2 hunks)test/integration_catalog_test.go(5 hunks)test/integration_debug_test.go(1 hunks)test/integration_feature_flags_test.go(1 hunks)test/integration_gcp_test.go(5 hunks)test/integration_parse_test.go(1 hunks)test/integration_s3_encryption_test.go(5 hunks)test/integration_serial_test.go(3 hunks)test/integration_test.go(5 hunks)tf/getproviders/lock_test.go(1 hunks)tf/getter_test.go(2 hunks)tf/run_cmd_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
**/*.go: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
internal/cache/cache_test.gocli/commands/dag/graph/cli_test.gotest/benchmarks/helpers/helpers.gocli/help_test.gotest/integration_debug_test.gotelemetry/tracer_test.goshell/run_cmd_windows_test.gocli/commands/scaffold/scaffold_test.gointernal/cas/race_test.gointernal/providercache/provider_cache_test.gocli/commands/common/runall/runall_test.gointernal/cli/command_test.goconfig/dependency_test.goshell/run_cmd_unix_test.gointernal/cas/integration_test.gocli/flags/flag_test.goconfig/stack_test.gocli/commands/render/render_test.goconfig/cache_test.gotf/getproviders/lock_test.gotest/integration_parse_test.goconfig/locals_test.gotf/getter_test.goshell/run_cmd_output_test.gointernal/services/catalog/module/repo_test.goconfig/catalog_test.gointernal/cas/cas_test.gocli/commands/run/cli_test.goconfig/config_partial_test.gotest/integration_catalog_test.gocli/commands/find/find_test.gointernal/strict/control_test.gointernal/cas/getter_test.goconfigstack/stack_test.gotelemetry/meter_test.goconfig/config_as_cty_test.gocli/commands/run/run_test.goconfigstack/test_helpers_test.goshell/run_cmd_test.gointernal/discovery/discovery_test.gocli/commands/list/list_test.gotest/integration_s3_encryption_test.gotf/run_cmd_test.gointernal/services/catalog/catalog_test.gocli/commands/run/run.gotest/integration_feature_flags_test.goconfig/config_helpers_test.gointernal/remotestate/backend/s3/client_test.goconfigstack/module_test.gointernal/cas/benchmark_test.gocli/commands/hcl/format/format_test.gotest/integration_serial_test.gointernal/cas/git_test.gocli/commands/run/download_source_test.gointernal/cli/app.gointernal/cas/tree_test.gocli/commands/list/list.gocli/commands/find/find.gotest/integration_test.gotest/integration_gcp_test.gotest/helpers/package.goconfig/config.goconfig/config_test.go
🧬 Code Graph Analysis (18)
cli/commands/scaffold/scaffold_test.go (1)
config/config.go (2)
ReadTerragruntConfig(1137-1145)DefaultParserOptions(100-112)
config/dependency_test.go (1)
config/parsing_context.go (1)
NewParsingContext(54-62)
internal/cas/integration_test.go (1)
internal/cas/cas.go (1)
CloneOptions(32-44)
config/stack_test.go (1)
config/parsing_context.go (1)
NewParsingContext(54-62)
cli/commands/render/render_test.go (1)
cli/commands/render/render.go (1)
Run(23-31)
tf/getproviders/lock_test.go (1)
tf/getproviders/lock.go (1)
UpdateLockfile(26-56)
test/integration_parse_test.go (1)
config/parsing_context.go (1)
NewParsingContext(54-62)
config/locals_test.go (1)
config/parsing_context.go (1)
NewParsingContext(54-62)
config/catalog_test.go (1)
config/catalog.go (1)
ReadCatalogConfig(80-99)
internal/cas/cas_test.go (1)
internal/cas/cas.go (1)
CloneOptions(32-44)
config/config_partial_test.go (2)
config/parsing_context.go (1)
NewParsingContext(54-62)config/config_partial.go (4)
DependenciesBlock(30-30)TerragruntFlags(34-34)DependencyBlock(31-31)TerraformSource(33-33)
config/config_as_cty_test.go (1)
config/parsing_context.go (1)
NewParsingContext(54-62)
cli/commands/run/run_test.go (1)
cli/commands/run/run.go (1)
RunTerraformWithRetry(478-501)
cli/commands/run/run.go (1)
util/file.go (1)
FileExists(57-60)
config/config_helpers_test.go (1)
config/parsing_context.go (1)
NewParsingContext(54-62)
configstack/module_test.go (1)
options/options.go (1)
DefaultParallelism(39-39)
internal/cas/benchmark_test.go (1)
internal/cas/cas.go (1)
CloneOptions(32-44)
cli/commands/list/list.go (1)
options/options.go (1)
TerragruntOptions(92-304)
🪛 GitHub Actions: Strict Lint
test/integration_debug_test.go
[error] 233-504: golangci-lint forcetypeassert: multiple type assertions must be checked.
cli/commands/scaffold/scaffold_test.go
[error] 19-19: golangci-lint funlen: function is too long (72 > 60).
internal/providercache/provider_cache_test.go
[error] 43-43: golangci-lint funlen: function is too long (124 > 60).
cli/commands/common/runall/runall_test.go
[error] 27-27: golangci-lint forbidigo: use of fmt.Println forbidden by pattern.
internal/cli/command_test.go
[error] 15-15: golangci-lint funlen: function is too long (176 > 60).
internal/cas/integration_test.go
[error] 15-15: golangci-lint funlen: function is too long (77 > 60).
cli/flags/flag_test.go
[error] 76-76: golangci-lint funlen: function is too long (105 > 60).
config/stack_test.go
[error] 60-60: golangci-lint funlen: function is too long (41 > 40 statements).
tf/getter_test.go
[error] 37-37: golangci-lint funlen: function is too long (64 > 60).
internal/services/catalog/module/repo_test.go
[error] 14-86: golangci-lint funlen: multiple functions are too long (exceeding 60 lines).
config/catalog_test.go
[error] 15-15: golangci-lint funlen: function is too long (128 > 60).
internal/cas/cas_test.go
[error] 13-13: golangci-lint funlen: function is too long (71 > 60).
cli/commands/run/cli_test.go
[error] 39-39: golangci-lint copyloopvar: The copy of the 'for' variable "tc" can be deleted (Go 1.22+)
internal/strict/control_test.go
[error] 122-283: golangci-lint funlen: multiple functions are too long (exceeding 60 lines).
[error] 35-92: golangci-lint gochecknoglobals: multiple global variables defined.
configstack/stack_test.go
[error] 281-281: golangci-lint funlen: function is too long (82 > 60).
config/config_as_cty_test.go
[error] 261-261: golangci-lint cyclop: calculated cyclomatic complexity for function terragruntConfigStructFieldToMapKey is 31, max is 10.
[error] 20-20: golangci-lint funlen: function is too long (145 > 60).
cli/commands/run/run_test.go
[error] 19-501: golangci-lint funlen: multiple functions are too long (exceeding 60 lines).
[error] 449-449: golangci-lint gochecknoglobals: defaultLogLevel is a global variable.
internal/discovery/discovery_test.go
[error] 14-403: golangci-lint funlen: multiple functions are too long (exceeding 60 lines).
cli/commands/list/list_test.go
[error] 17-350: golangci-lint funlen: multiple functions are too long (exceeding 60 lines).
internal/services/catalog/catalog_test.go
[error] 46-162: golangci-lint err113: do not define dynamic errors, use wrapped static errors instead.
cli/commands/run/run.go
[error] 298-298: golangci-lint cyclop: calculated cyclomatic complexity for function runTerragruntWithConfig is 16, max is 10.
[error] 188-196: golangci-lint err113: do not define dynamic errors, use wrapped static errors instead.
[error] 79-79: golangci-lint gochecknoglobals: sourceChangeLocks is a global variable.
config/config_helpers_test.go
[error] 892-930: golangci-lint forcetypeassert: multiple type assertions must be checked.
[error] 959-987: golangci-lint forcetypeassert: multiple type assertions must be checked.
[error] 1225-1234: golangci-lint forcetypeassert: multiple type assertions must be checked.
[error] 147-147: golangci-lint funlen: function is too long (71 > 60).
[error] 231-231: golangci-lint funlen: function is too long (99 > 60).
configstack/module_test.go
[error] 332-1281: golangci-lint err113: do not define dynamic errors, use wrapped static errors instead.
[error] 1100-1172: golangci-lint funlen: multiple functions are too long (exceeding 60 lines).
internal/cas/benchmark_test.go
[error] 127-127: golangci-lint cyclop: calculated cyclomatic complexity for function BenchmarkGitOperations is 11, max is 10.
[error] 16-16: golangci-lint funlen: function is too long (61 > 60).
[warning] 26-182: golangci-lint intrange: for loops can be changed to use an integer range (Go 1.22+).
cli/commands/hcl/format/format_test.go
[error] 52-270: golangci-lint copyloopvar: The copy of the 'for' variable can be deleted (Go 1.22+).
test/integration_serial_test.go
[error] 35-623: golangci-lint funlen: multiple functions are too long (exceeding 60 lines).
internal/cas/git_test.go
[error] 137-137: golangci-lint funlen: function is too long (64 > 60).
internal/cli/app.go
[error] 192-192: golangci-lint cyclop: calculated cyclomatic complexity for function setupAutocomplete is 12, max is 10.
[error] 29-29: golangci-lint gochecknoglobals: defaultLogLevel is a global variable.
[warning] 173-237: golangci-lint wrapcheck: error returned from external package is unwrapped; should be wrapped.
internal/cas/tree_test.go
[error] 13-134: golangci-lint funlen: multiple functions are too long (exceeding 60 lines).
cli/commands/list/list.go
[error] 22-534: golangci-lint cyclop: multiple functions (Run, discoveredToListed, renderLong, generateDAGTree) have cyclomatic complexity > 10.
cli/commands/find/find.go
[error] 19-131: golangci-lint cyclop: calculated cyclomatic complexity for functions Run and discoveredToFound are 18 and 16 respectively, max is 10.
test/integration_test.go
[error] 3802-3802: golangci-lint dogsled: declaration has 3 blank identifiers.
[error] 1405-2504: golangci-lint forcetypeassert: multiple type assertions must be checked.
[error] 295-624: golangci-lint funlen: multiple functions are too long (exceeding 60 lines).
test/helpers/package.go
[error] 539-952: golangci-lint funlen: multiple functions are too long (exceeding 60 lines).
[warning] 716-716: golangci-lint wrapcheck: error returned from external package is unwrapped; should be wrapped.
config/config.go
[error] 1252-1252: golangci-lint cyclop: calculated cyclomatic complexity for function ParseConfig is 16, max is 10.
[error] 95-114: golangci-lint gochecknoglobals: multiple global variables defined.
[error] 1388-1388: golangci-lint gochecknoglobals: iamRoleCache is a global variable.
config/config_test.go
[error] 1306-1423: golangci-lint funlen: multiple functions are too long (exceeding 60 lines).
[error] 1558-1558: golangci-lint funlen: function is too long (55 > 40 statements).
[error] 15-15: golangci-lint funlen: function is too long (98 > 60).
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (114)
internal/cli/app.go (1)
211-215: Good refactoring of the switch statement!This refactor is cleaner and more idiomatic Go, directly switching on the argument value rather than using boolean expressions in case clauses. It makes the code more readable and maintainable while preserving the same functionality.
mise.toml (1)
4-4: Version update for golangci-lint looks good.Updating to golangci-lint v2.1.6 is appropriate for this migration and aligns with the PR objective.
cli/commands/find/find_test.go (1)
436-436: Good improvement to context management.Replacing
context.Background()witht.Context()is a best practice for tests as it ties the context lifecycle to the test framework. This ensures proper context cancellation when tests complete or time out.telemetry/meter_test.go (1)
20-20: Good improvement to context management.Using
t.Context()instead ofcontext.Background()is a best practice for tests, as it properly ties the context lifecycle to the test framework. This ensures proper context propagation and cancellation handling during testing.telemetry/tracer_test.go (1)
19-19: Good improvement using test context!This change improves the test by using the test's own context (
t.Context()) instead of an explicitly created background context. This approach is better because:
- It ties the context to the test's lifecycle (auto-cancellation when the test completes)
- It follows best practices for Go testing
- It's consistent with similar changes throughout the codebase as part of the golangci-lint v2 migration
.github/workflows/strict-lint.yml (1)
48-48: Better descriptive step nameThe renaming from "Lint" to "Strict Lint" improves clarity by making the step name match the workflow name and better describe the specific type of linting being performed. This change enhances the readability of workflow logs and status reports.
cli/commands/run/cli_test.go (1)
46-46: Good improvement using test context!This change properly uses the test's own context (
t.Context()) instead of an explicitly created background context, which is a better practice for testing in Go. This ties the context to the test's lifecycle and ensures proper resource cleanup.cli/help_test.go (1)
80-80: Good improvement using test context!This change improves the test by using the test's own context (
t.Context()) instead of an explicitly created background context. This approach:
- Properly ties the context to the test's lifecycle
- Ensures resources are cleaned up when the test completes
- Follows Go testing best practices
- Maintains consistency with similar changes throughout the codebase
internal/cas/race_test.go (1)
52-52: Good improvement using test context instead of context.TODO()Using
t.Context()instead of a detached context is a great practice as it properly ties the context lifecycle to the test execution. This ensures proper resource cleanup when the test completes or times out.internal/services/catalog/module/repo_test.go (1)
60-60: Properly using test contextGood change to use
t.Context()instead of a standalone context. This ensures proper context cancellation when the test completes.shell/run_cmd_test.go (2)
21-21: Good context management improvementExcellent change to use
t.Context()instead of context.Background() for all shell command executions. This ensures proper context propagation and cancellation when the test completes.Also applies to: 24-24, 40-40, 53-53
78-78: Proper context initializationGood refactoring to use the test context as the base for the cache context. This ensures proper lifecycle management.
cli/commands/common/runall/runall_test.go (1)
22-22: Good improvement using test contextUsing
t.Context()instead of context.Background() ensures proper context propagation and cancellation when tests complete.internal/cas/integration_test.go (1)
31-31: Good improvement on context usage!Replacing context.TODO() with t.Context() aligns with best practices for Go testing. This ensures the context is properly canceled when the test completes, preventing potential resource leaks.
Also applies to: 46-46, 72-72, 91-91, 104-104
test/integration_debug_test.go (1)
58-58: Good improvement on context usage!Replacing context.Background() with t.Context() aligns with best practices for Go testing. This ensures the context is properly canceled when the test completes, preventing potential resource leaks.
internal/cache/cache_test.go (1)
25-25: Good improvement on context usage!Replacing context.Background() with t.Context() aligns with best practices for Go testing. This ensures the context is properly canceled when the test completes, preventing potential resource leaks.
Also applies to: 55-55, 74-74
tf/getproviders/lock_test.go (1)
163-163: Good improvement on context usage!Replacing context.Background() with t.Context() aligns with best practices for Go testing. This ensures the context is properly canceled when the test completes, preventing potential resource leaks.
internal/cli/command_test.go (1)
181-181: Good improvement to test context usage.Replacing a manual background context with the test's context (
t.Context()) is a better practice as it ties the context lifecycle to the test lifecycle. This ensures proper context cancellation when tests complete or time out, and aligns with Go testing best practices.config/cache_test.go (1)
29-29: Well-aligned test context usage.Using
t.Context()for cache operations properly integrates with the test lifecycle rather than using a detached background context. This ensures that context cancellation is respected if the test times out or completes, which is a good practice for Go tests.config/config_as_cty_test.go (1)
232-232: Good standardization of context usage in tests.Using
t.Context()when creating parsing contexts properly integrates with the test lifecycle, ensuring that context deadlines and cancellations are properly propagated. The change is consistent with the pattern applied across the codebase and follows Go testing best practices.Also applies to: 251-251
cli/commands/run/run_test.go (1)
266-266: Appropriate test context propagation.Using
t.Context()when callingRunTerraformWithRetryensures that the test's cancellation signals are properly propagated to the function under test. This is especially important for this function since it contains retry logic with context-based cancellation checks.test/benchmarks/helpers/helpers.go (1)
37-37: Good change to use benchmark contextReplacing the implicit context.Background() with b.Context() ensures better context propagation during benchmarks, aligning with Go best practices for context handling. This change properly integrates with the benchmark lifecycle for cancellation and timeout propagation.
config/locals_test.go (1)
25-25: Good improvement to context management in testsConsistently using t.Context() instead of context.Background() when creating parsing contexts ensures proper propagation of test lifecycle, including cancellation signals and deadlines. This change aligns with Go best practices for context management in tests.
Also applies to: 70-70, 109-109, 129-129
shell/run_cmd_windows_test.go (1)
28-28: Good context management updateUsing t.Context() as the parent context for cancellation properly integrates with the test lifecycle. This change ensures that any test timeout will properly propagate through the created context.
internal/services/catalog/catalog_test.go (1)
65-65: Good context propagation improvementsReplacing context.Background() with t.Context() in all Load() calls ensures proper test lifecycle integration and follows Go best practices for context management in tests.
Also applies to: 87-87, 113-113, 135-135, 166-166, 187-187
internal/providercache/provider_cache_test.go (1)
108-108: Good improvement: Using test context instead of background contextReplacing
context.Background()witht.Context()is a beneficial change as it properly ties the context lifecycle to the test's lifecycle. This ensures that test timeouts and cancellations are properly propagated to all operations using this context.internal/cas/tree_test.go (1)
251-251: Good improvement: Using test context instead of background contextReplacing
context.Background()witht.Context()is a good practice as it ensures that the context is tied to the test's lifecycle. This change improves context propagation and allows proper test timeout handling.cli/flags/flag_test.go (1)
157-158: Good improvement: Using test context instead of background contextUsing
t.Context()instead ofcontext.Background()properly integrates with the testing framework's context lifecycle. This ensures that any test timeouts or cancellations will propagate correctly to the logger and subsequent operations.config/dependency_test.go (1)
119-119: Good improvement: Using test context instead of background contextUsing
t.Context()instead ofcontext.Background()when creating the parsing context is a better practice as it ties the context lifecycle to the test's lifecycle. This ensures proper propagation of test timeouts and cancellations to all parsing operations.config/catalog_test.go (1)
134-134: LGTM - Good use of test contextThe change from
context.Background()tot.Context()is a good practice as it ties the context lifecycle to the test's lifecycle, which can help with proper resource cleanup and cancellation propagation.shell/run_cmd_unix_test.go (1)
31-31: LGTM - Using test context improves cancellation propagationReplacing
context.Background()witht.Context()ensures that cancellation and timeout signals from the test framework are properly propagated to this context, which is particularly important for command execution tests that need to be properly cleaned up.internal/strict/control_test.go (1)
356-357: LGTM - Standardized context usageUsing
t.Context()instead of a standalone background context ensures that the test's timeout and cancellation signals are properly propagated, while maintaining the same logger integration. This is consistent with the repository-wide standardization of context usage.internal/cas/cas_test.go (3)
29-31: LGTM - Consistent context usage patternUsing
t.Context()instead ofcontext.TODO()ensures that test timeouts and cancellation are properly propagated to the Clone operation, which is especially important for network operations that may hang.
54-57: LGTM - Proper context propagationConsistent use of the test context in this clone operation with branch specification, which maintains the standardized context pattern across the codebase.
76-79: LGTM - Improved context handlingReplacing
context.TODO()with the test's context is appropriate here, particularly for clone operations that preserve git files, which may involve more file system operations that benefit from proper context cancellation.config/config_partial_test.go (1)
25-25: Good practice: Using test-owned contextsAll context creation now uses
t.Context()instead ofcontext.Background(), which is a best practice as it properly ties context lifecycle to the test lifecycle. This ensures that cancellation signals and deadlines from the test framework are properly propagated to the code under test.Also applies to: 54-54, 74-74, 96-96, 115-115, 128-128, 147-147, 174-174, 198-198, 224-224, 251-251, 278-278, 305-305, 332-332, 355-355
shell/run_cmd_output_test.go (2)
66-66: Good practice: Using test-owned contextsUsing
t.Context()instead ofcontext.Background()properly ties the context lifecycle to the test lifecycle, ensuring cancellation signals and deadlines from the test framework propagate to command execution.
85-85: Improved test assertion for slice lengthUsing
assert.Len()instead ofassert.Equal()for checking slice length is more idiomatic and provides clearer error messages in case of failure.test/integration_catalog_test.go (2)
21-21: Good practice: Using test-owned contextsExplicitly creating and using
t.Context()variables for module repository operations is a good practice as it properly ties context lifecycle to the test lifecycle.Also applies to: 35-35, 50-50, 89-89
135-135: Good practice: Using test-owned contexts for config readingUsing
t.Context()forReadTerragruntConfigproperly ties context lifecycle to the test lifecycle, ensuring proper propagation of cancellation signals and deadlines.cli/commands/dag/graph/cli_test.go (1)
50-50: Good practice: Using benchmark-owned contextsUsing
b.Context()instead ofcontext.Background()for the benchmark properly ties the context lifecycle to the benchmark's lifecycle, ensuring cancellation signals and deadlines from the test framework are properly propagated.test/integration_feature_flags_test.go (1)
152-152: Improved type checking withassert.EqualThe change from
assert.EqualValuestoassert.Equalenforces strict type equality between expected and actual values, making the test more precise by preventing implicit type conversions.This change aligns with best practices for Go testing by ensuring that the types match exactly, not just the values, which can help catch subtle type-related issues.
tf/getter_test.go (1)
19-19: Improved context handling witht.Context()Replacing
context.Background()witht.Context()properly ties the context lifecycle to the test's lifecycle, ensuring better resource cleanup when the test ends.This is a best practice in Go testing as it ensures that any operations that depend on this context will be canceled if the test times out or completes.
Also applies to: 32-32
Makefile (1)
42-42: Updated golangci-lint to v2The installation target now correctly points to version 2.1.6 of golangci-lint, matching the PR objective of migrating to v2.
The updated import path with
/v2/follows Go module versioning conventions for major version changes. This change is central to the PR's purpose of upgrading the linting tool.test/integration_parse_test.go (1)
53-53: Improved context handling witht.Context()Replacing
context.Background()witht.Context()properly ties the context lifecycle to the test's lifecycle, ensuring better resource cleanup when the test ends.This change is consistent with the pattern applied across multiple test files in this PR and represents a best practice for context management in Go tests. The
NewParsingContextfunction uses this context to set up Terraform command hooks and parser options.cli/commands/render/render_test.go (1)
27-27: Great use of test context!Replacing
context.Background()witht.Context()is a best practice that ensures proper context propagation tied to the test lifecycle. This change improves test reliability by ensuring contexts get canceled when tests complete, preventing potential resource leaks.Also applies to: 49-49, 71-71, 92-92, 106-106
cli/commands/list/list_test.go (1)
70-70: Good improvement to context handlingUpdating the code to use
t.Context()instead ofcontext.Background()ensures proper context propagation tied to the test lifecycle, improving test reliability.Also applies to: 153-153, 235-235, 312-312, 421-421
internal/remotestate/backend/s3/client_test.go (1)
72-72: Proper context propagation improvementGreat change to use
t.Context()instead ofcontext.Background()across all AWS S3 client operations. This ensures proper context cancellation and deadline propagation tied to the test lifecycle.Also applies to: 87-87, 103-103, 121-121, 178-178, 206-206
test/integration_s3_encryption_test.go (1)
50-50: Improved context management in integration testsUsing
t.Context()instead ofcontext.Background()across all AWS client calls ensures proper context propagation tied to the test lifecycle. This is particularly important for integration tests that make actual AWS API calls.Also applies to: 76-76, 100-100, 144-144, 178-178
cli/commands/find/find.go (1)
275-275: Improved logical expression for better readability.The change from negating a disjunction to using the conjunction of negations makes the condition more readable while maintaining the same logical behavior.
cli/commands/scaffold/scaffold_test.go (1)
87-87: Improved context usage in test.Replacing
context.Background()witht.Context()is a good practice as it ties the context to the test's lifecycle, ensuring proper cancellation when the test ends.cli/commands/run/download_source_test.go (3)
257-257: Improved context usage in test.Replacing
context.Background()witht.Context()is a good practice as it ties the context to the test's lifecycle, ensuring proper cancellation when the test ends.
365-365: Improved context usage in test helper function.Consistently using
t.Context()in helper functions that are called from tests ensures proper context propagation and resource management.
406-406: Improved context usage in test setup.Updating the
PopulateTerraformVersioncall to uset.Context()ensures consistent context handling throughout the test.internal/cas/benchmark_test.go (4)
37-37: Improved context usage in benchmark.Replacing arbitrary contexts with
b.Context()is a good practice as it ties the context to the benchmark's lifecycle, ensuring proper cancellation when the benchmark ends.
56-56: Improved context usage in benchmark setup.Using the benchmark context for the initial clone operation maintains consistency in context handling.
73-73: Improved context usage in benchmark loop.Using the benchmark context within the benchmark loop ensures that each iteration has access to the proper context.
132-132: Improved context creation in benchmark.Creating a context from the benchmark rather than using
context.TODO()ensures proper context management throughout the benchmark.internal/cas/git_test.go (2)
17-17: Use of test context improves context propagationGood practice to use
t.Context()instead of directly creating a background context. This ensures proper test lifecycle management and supports context cancellation.
50-50: Test context usage in TestGitRunner_CloneConsistent usage of test context here is good practice. This change matches the pattern applied throughout the test files in this PR.
internal/discovery/discovery_test.go (2)
82-82: Use of test context in TestDiscoveryUsing
t.Context()correctly propagates the test context to theDiscovermethod.
261-261: Test context in TestDiscoveryWithDependenciesConsistent context usage here with other changes in the PR.
configstack/stack_test.go (1)
44-44: Use of test context in FindStackInSubfoldersGood practice to use
t.Context()to propagate the test lifecycle context.tf/run_cmd_test.go (2)
71-71: Use of test context in RunCommandWithOutputGood practice to use the test context from
t.Context()instead of creating a new background context.
90-90: Improved assertion style for length checkUsing
assert.Lenis more idiomatic and better communicates intent thanassert.Equalfor slice length comparisons. This change improves test readability and error messages.configstack/test_helpers_test.go (3)
36-36: Improved assertion readability with assert.LenThe change from
assert.Equal(t, len(expectedModules), len(actualModules))toassert.Len(t, actualModules, len(expectedModules))is a good improvement. Usingassert.Lenis more idiomatic in Go tests and provides clearer error messages when assertions fail.
81-81: Improved assertion readability with assert.LenSimilar to the previous change, using
assert.Lenhere improves test readability and follows Go testing best practices.
99-99: Improved assertion readability with assert.LenThe change to
assert.Lenhere is consistent with the other assertion improvements and makes the testing code more idiomatic.internal/cas/getter_test.go (3)
21-21: Using test context instead of background contextReplacing
context.Background()witht.Context()is a good improvement. This ties the context to the test lifecycle, allowing for proper propagation of cancellation signals and deadlines.
30-30: Using test context instead of background contextSame improvement as above - using the test's context instead of a generic background context.
110-111: Using test context in client.Get callProperly updated to use test context in the client.Get call, which ensures the operation respects test timeouts and cancellation.
test/integration_gcp_test.go (5)
404-404: Using test context instead of context.TODO()Replacing
context.TODO()witht.Context()in GCS bucket operations is a good practice. This ensures that GCS operations respect test timeouts and cancellation signals.
437-437: Using test context in gcsObjectAttrs functionUsing
t.Context()here ensures that object attribute retrieval operations properly integrate with the test lifecycle.
461-461: Using test context in assertGCSLabels functionCorrectly updated to use the test context for GCS label verification operations.
482-482: Using test context in createGCSBucket functionProperly updated to use test context when creating GCS buckets during tests.
507-507: Using test context in deleteGCSBucket functionUsing the test context for cleanup operations ensures proper integration with test lifecycle and respects test timeouts.
config/stack_test.go (4)
31-31: Using test context in NewParsingContextReplacing
context.Background()witht.Context()when creating a new parsing context is a good improvement. This ensures that the parsing operations properly respect test timeouts and cancellation.
104-104: Using test context in complex config testConsistent use of test context in the complex config test case.
162-162: Using test context in invalid syntax testProperly updated to use test context in the invalid syntax test case.
60-151:❓ Verification inconclusive
Consider refactoring test to address pipeline failure
The pipeline indicates that
TestParseTerragruntStackConfigComplexis slightly over the function length limit (41 statements vs limit of 40). Consider extracting assertion groups into helper functions to reduce function length.
🏁 Script executed:
#!/bin/bash # Verify the function length issue and count statements echo "Checking function length for TestParseTerragruntStackConfigComplex" ast-grep --pattern $'func TestParseTerragruntStackConfigComplex(t \*testing.T) { $$$ }' config/stack_test.go | wc -lLength of output: 264
Refactor TestParseTerragruntStackConfigComplex to satisfy function-length limit
CI is reporting that TestParseTerragruntStackConfigComplex in config/stack_test.go (lines 60–151) exceeds the 40-statement threshold. To both bring it under the limit and improve readability, extract each block of assertions into focused helper functions. For example:
func assertLocals(t *testing.T, cfg *StackConfig, expected map[string]string)func assertUnit(t *testing.T, u UnitConfig, name, source, path string, noStack bool, values map[string]interface{})func assertStack(t *testing.T, s StackConfig, name, source, path string, noStack bool, values map[string]interface{})Moving the locals, units and stacks checks into these helpers will reduce the number of statements in the main test and make it easier to maintain.
🧰 Tools
🪛 GitHub Actions: Strict Lint
[error] 60-60: golangci-lint funlen: function is too long (41 > 40 statements).
config/config_test.go (2)
42-42: Good standardization of context usage in testsThe change from
context.Background()tot.Context()across all test functions is a positive improvement. This ensures that test contexts are tied to the test lifecycle and will be properly cancelled when tests complete.This consistent update aligns with best practices for context propagation in tests and makes the codebase more maintainable.
Also applies to: 67-67, 93-93, 114-114, 139-139, 181-181, 214-214, 242-242, 262-262, 281-281, 300-300, 320-320, 342-342, 456-456, 498-498, 526-526, 563-563, 614-614, 663-663, 693-693, 718-718, 740-740, 1208-1208, 1227-1227, 1266-1266, 1414-1414
758-758: Improved assertion style for better clarityThe switch from
assert.EqualValuestoassert.Equalmakes the assertions more consistent throughout the test suite.Also applies to: 808-808
cli/commands/run/run.go (1)
652-652:❓ Verification inconclusive
Changed provider initialization logic condition
The condition for determining when provider initialization is needed has been modified from an OR condition to an AND condition for the plugins and providers paths. This change narrows the circumstances under which Terraform initialization will be triggered.
Original condition (would initialize if ANY path was missing):
return !util.FileExists(pluginsPath) || !util.FileExists(providersPath) || !util.FileExists(terraformLockPath)New condition (only initializes if BOTH plugin and provider paths are missing, OR if lock file is missing):
return (!util.FileExists(pluginsPath) && !util.FileExists(providersPath)) || !util.FileExists(terraformLockPath)This change may reduce unnecessary initializations but should be verified to ensure it doesn't prevent initialization when needed.
🏁 Script executed:
#!/bin/bash # Check the comment pattern in the code to understand the intent of this change rg -A 5 -B 5 "providersNeedInit" -- "*.go" # Look for any issues or discussions about terraform initialization behavior gh issue list --search "terraform init providers initialization" gh pr list --search "terraform init providers initialization"Length of output: 987
🏁 Script executed:
#!/bin/bash set -e # Locate the run.go file to confirm path find . -type f -name "run.go" # Search for occurrences of pluginsPath and providersPath across the repository rg -n "pluginsPath" . rg -n "providersPath" . # Show context around the FileExists condition to verify its use and intent rg -n -C 5 "util.FileExists" .Length of output: 48174
🏁 Script executed:
#!/bin/bash set -e # Search for Auto-Init in README.md to verify documentation of init conditions rg -n "Auto-Init" -n README.md # Search for references to plugins/providers in README to see expected behavior rg -n "plugins" -n README.md rg -n "providers" -n README.mdLength of output: 39
Verify provider initialization condition and documentation
The
providersNeedInitlogic incli/commands/run/run.gonow only triggersterraform initwhen both the legacy plugins directory and the new providers directory are missing, or when the lock file is absent. Please confirm that this change doesn’t inadvertently skip initialization in valid scenarios and that it’s documented appropriately.• File: cli/commands/run/run.go:652
• Diff:- return !util.FileExists(pluginsPath) || !util.FileExists(providersPath) || !util.FileExists(terraformLockPath) + return (!util.FileExists(pluginsPath) && !util.FileExists(providersPath)) || !util.FileExists(terraformLockPath)• Verify that in all supported Terraform versions, missing either
.terraform/pluginsor.terraform/providersstill triggers init if needed.
• If this behavior is intentional, updateREADME.md(or relevant docs) to describe the new Auto-Init conditions.config/config_helpers_test.go (2)
77-77: Standardized context usage in testsThe change from
context.Background()tot.Context()across all test functions is a consistent improvement that aligns with modern Go testing practices. This ensures test contexts are properly tied to the test lifecycle and will be cancelled appropriately when tests complete.Also applies to: 140-140, 207-207, 319-319, 368-368, 389-389, 486-486, 537-537, 581-581, 795-795, 882-882, 919-919, 953-953, 995-995, 1062-1062, 1093-1093, 1154-1154, 1201-1201, 1218-1218
550-550: Improved assertion style for better clarityChanged from using
assert.EqualValuesto the more idiomaticassert.Equalfor comparing slices, which improves consistency across the test suite.Also applies to: 594-594
configstack/module_test.go (1)
239-239: Improved context management by using test context.The change from
context.Background()tot.Context()ties the context lifecycle to the test lifecycle, enabling better cancellation propagation and deadline handling. This is a good practice that ensures if a test is canceled or times out, operations using that context will also be canceled.Also applies to: 259-259, 281-281, 302-302, 323-323, 345-345, 367-367, 389-389, 470-470, 512-512, 554-554, 597-597, 641-641, 684-684, 736-736, 779-779, 821-821, 866-866, 915-915, 960-960, 1003-1003, 1049-1049, 1092-1092, 1161-1161, 1247-1247, 1324-1324
cli/commands/hcl/format/format_test.go (1)
37-37: Improved context management by using test context.Replacing
context.Background()witht.Context()ties the context lifecycle to the test lifecycle, enabling better test control and resource cleanup. This change is consistent with the pattern applied across the codebase.Also applies to: 127-127, 153-153, 199-199, 245-245, 309-309, 332-332
cli/commands/list/list.go (1)
324-324: Improved clarity of color output condition.The logic has been refactored to be more readable while maintaining the same behavior. The new expression directly states the two conditions that must be met for color output: colors must not be disabled in the logger, and stdout must not be redirected.
test/integration_test.go (6)
209-209: Good improvement: Using test context instead of background contextReplacing
context.Background()witht.Context()is a good practice as it properly leverages the test context for better lifecycle management and cancellation propagation.
228-228: Good improvement: Using test context instead of background contextUsing
t.Context()instead ofcontext.Background()ensures proper context propagation and cleanup during tests.
245-245: Good improvement: Using test context instead of background contextConsistently using
t.Context()instead ofcontext.Background()maintains proper context handling across all test functions.
263-263: Good improvement: Using test context instead of background contextCorrectly using the test context instead of creating a new background context.
284-284: Good improvement: Using test context instead of background contextThis change completes the consistent migration from
context.Background()tot.Context()across all detailed exit code test functions.
4-35: Good: Removed unused context package importThe removal of the "context" package import is consistent with replacing
context.Background()witht.Context()across the test functions.test/integration_serial_test.go (3)
67-67: Good improvement to use test contextUsing
t.Context()instead ofcontext.Background()is a best practice as it ties the context lifecycle to the test, ensuring proper propagation of cancellation signals and deadlines.
111-111: Good improvement to use test context with cancellationUsing
t.Context()withWithCancelproperly ties the context lifecycle to the test while maintaining the cancellation functionality needed for this test.
254-254: More idiomatic assertionChanging from
assert.Equal(t, len(appDirs), len(entries))toassert.Len(t, entries, len(appDirs))is more idiomatic and provides clearer error messages on failure.test/helpers/package.go (2)
788-788: Good improvement to use test contextUsing
t.Context()instead ofcontext.Background()ensures that test timeouts and cancellation signals are properly propagated through the helper functions.
830-830: Good improvement to use test contextSimilar to the previous change, this ensures proper context propagation in the command execution helper.
config/config.go (1)
1430-1431: Improved error handling logicThe modified condition provides more precise control over error suppression. It now only suppresses errors when both conditions are met:
- The command matches (
render-jsonorrender), AND- The error is an attribute access error
This refinement ensures errors are properly reported in other scenarios.
.strict.golangci.yml (5)
1-13: Good upgrade to golangci-lint v2This updates the linter configuration to the new version 2 format and upgrades the Go version to 1.24. The output format now uses more descriptive settings that include linter names and issue lines for better diagnostics.
14-107: Comprehensive expansion of lintersThe list of enabled linters has been significantly expanded to include many additional checks that will improve code quality. This is a good modernization of the linting setup that will catch more potential issues.
113-154: Well-structured linter settingsThe settings have been reorganized under the new v2 format's "settings" key, maintaining the existing configurations while adapting to the new structure.
155-173: Good addition of exclusion rulesAdding explicit exclusion rules for generated code and test files is a good practice that prevents noisy linting errors in areas where strict compliance isn't necessary or practical.
177-192: Good addition of formatter configurationAdding the goimports formatter with simplify option will help maintain consistent code formatting across the codebase, with appropriate exclusions for generated code and special directories.
.golangci.yml (9)
1-1: Upgrade to golangci-lint v2 configuration
The top-levelversion: "2"aligns with golangci-lint v2 requirements.
3-3: Verify Go version in CI
You've bumped the Go version to"1.24". Please confirm that your CI/CD images and documentation have been updated to use Go 1.24 to avoid mismatches during builds.
8-11: Switch to plain text output format
Changing fromcolored-line-numberto plaintextwith linter names and line numbers can improve CI log parsability. Ensure that this meets the team’s requirements for local development readability.
68-69: Validate dupl threshold
Theduplthreshold is set to120, which is significantly higher than the default. Please verify this value appropriately balances duplication detection and false positives for our codebase.
95-96: Review nakedret function length limit
Limiting naked returns to functions under20lines is strict and may flag many existing helpers. Double-check whether this aligns with your style guidelines or if a higher threshold would be more practical.
118-124: Verify exclusion path regexes
Thepaths:exclusions use patterns likethird_party$andbuiltin$. Confirm that these regex anchors properly match the intended directories (e.g.third_party/) and don’t inadvertently exclude similarly named files.
126-127: Enforce zero tolerance for issues
Setting bothmax-issues-per-linterandmax-same-issuesto0will reject any new lint findings. Ensure the team is prepared to address every warning immediately, or consider a non-zero baseline for incremental enforcement.
129-133: Enable goimports with simplification
Enablinggoimportsandgofmt.simplifyis a great step towards automating import ordering and code formatting. Looks good to me.
135-143: Mirror code exclusions for formatters
Applying the samegenerated: laxand path-based exclusions to formatters ensures consistency between linting and formatting. Approving.
| @@ -138,7 +137,7 @@ func TestGetRepoName(t *testing.T) { | |||
| func TestGitRunner_LsTree(t *testing.T) { | |||
| t.Parallel() | |||
|
|
|||
| ctx := context.Background() | |||
| ctx := t.Context() | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Test context usage in TestGitRunner_LsTree
Using the test context instead of a background context is appropriate and consistent with other changes.
Note that line 137 is flagged by golangci-lint: "function is too long (64 > 60)". Consider refactoring this test function into smaller sub-functions or subtests to satisfy the linter.
🏁 Script executed:
#!/bin/bash
# Count lines in the TestGitRunner_LsTree function
sed -n '/^func TestGitRunner_LsTree/,/^}/p' internal/cas/git_test.go | wc -lLength of output: 81
Refactor TestGitRunner_LsTree to satisfy linter constraints
Using t.Context() here is correct and aligns with the rest of the suite. However, the function body is 72 lines long—exceeding the 60-line limit enforced by golangci-lint. Please break it down into smaller units, for example:
- Extract common setup into a helper (e.g.
newTestGitRunner) - Split each scenario into a
t.Run("case name", func(t *testing.T) { … })subtest - Move assertion logic into dedicated assertion functions
File: internal/cas/git_test.go
Function: TestGitRunner_LsTree (lines 137–~208)
🤖 Prompt for AI Agents
In internal/cas/git_test.go around lines 137 to 208, the TestGitRunner_LsTree
function exceeds the 60-line limit set by golangci-lint, currently being 72
lines long. Refactor this function by extracting common setup code into a helper
function like newTestGitRunner, splitting different test scenarios into separate
subtests using t.Run with descriptive names, and moving assertion logic into
dedicated helper functions. This will reduce the main test function length and
improve readability while satisfying the linter constraints.
| @@ -445,7 +444,7 @@ exclude { | |||
| // Test discovery with exclude parsing | |||
| d := discovery.NewDiscovery(tmpDir).WithParseExclude() | |||
|
|
|||
| cfgs, err := d.Discover(context.Background(), tgOpts) | |||
| cfgs, err := d.Discover(t.Context(), tgOpts) | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Test context in TestDiscoveryWithExclude
Good change to use test context consistently.
The linter reports multiple functions in this file exceeding the 60-line limit. Consider refactoring the long test functions to improve readability and satisfy the linter requirements.
🏁 Script executed:
#!/bin/bash
# Find all test functions in this file that exceed 60 lines
awk '/^func Test/ {start=NR; name=$2} /^}/ {if (NR-start > 60) print name, NR-start}' internal/discovery/discovery_test.goLength of output: 247
Refactor lengthy tests in internal/discovery/discovery_test.go to meet the 60-line limit
Great job on consistently using the test context in TestDiscoveryWithExclude. The linter output confirms several test functions exceed the 60-line threshold—please break these up to improve readability and satisfy lint rules:
• internal/discovery/discovery_test.go
– TestDiscovery: 80 lines
– TestDiscoveryWithDependencies: 145 lines
– TestDiscoveredConfigsCycleCheck: 106 lines
– TestDiscoveryWithExclude: 78 lines
Recommendations:
- Split each oversized test into smaller subtests (t.Run) or helper functions.
- Consider table-driven tests for repetitive scenarios.
- Extract common setup/teardown code into shared fixtures.
🤖 Prompt for AI Agents
In internal/discovery/discovery_test.go around line 447 and throughout the file,
several test functions exceed the 60-line limit, making them hard to read and
maintain. Refactor these long tests by splitting them into smaller subtests
using t.Run or by extracting helper functions for repeated logic. Use
table-driven tests for repetitive cases and move common setup or teardown code
into shared fixtures to improve clarity and comply with linting rules.
| @@ -202,7 +201,7 @@ func TestResolveTerraformModulesNoPaths(t *testing.T) { | |||
| configPaths := []string{} | |||
| expected := configstack.TerraformModules{} | |||
| stack := configstack.NewStack(mockOptions) | |||
| actualModules, actualErr := stack.ResolveTerraformModules(context.Background(), configPaths) | |||
| actualModules, actualErr := stack.ResolveTerraformModules(t.Context(), configPaths) | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Test context propagation in ResolveTerraformModules calls
Consistent application of test context usage across all method calls. This ensures proper test lifecycle management and promotes clean context handling.
Note that a long function was identified by golangci-lint at line 281. Consider refactoring long test functions to improve readability and satisfy the linter.
Also applies to: 227-227, 250-250, 276-276, 361-361, 387-387, 413-413, 448-448, 483-483, 518-518, 550-550, 599-599, 642-642, 677-677, 719-719, 762-762, 812-812, 877-877, 937-937, 989-989, 1024-1024, 1083-1083, 1094-1094
🏁 Script executed:
#!/bin/bash
# Check the function at line 281
awk 'NR==281,/^}/ {count++} END {print "Function length:", count}' configstack/stack_test.go
# Find all the test functions in this file that exceed 60 lines
grep -n "^func Test" configstack/stack_test.go | while read line; do
line_num=$(echo $line | cut -d: -f1)
func_name=$(echo $line | cut -d: -f2 | awk '{print $2}')
end_line=$(awk "NR>$line_num && /^}/ {print NR; exit}" configstack/stack_test.go)
if [ ! -z "$end_line" ]; then
length=$((end_line - line_num))
if [ $length -gt 60 ]; then
echo "$func_name: $length lines"
fi
fi
doneLength of output: 14853
I’ll verify that every ResolveTerraformModules invocation in configstack/stack_test.go uses t.Context() consistently.
🏁 Script executed:
#!/bin/bash
echo "All ResolveTerraformModules calls:"
grep -n "stack.ResolveTerraformModules" configstack/stack_test.goLength of output: 2272
Context propagation is already consistent; refactor the long test at line 281
All 27 calls to stack.ResolveTerraformModules in configstack/stack_test.go already pass t.Context(), so no changes are needed for context handling.
• The test function TestResolveTerraformModulesReadConfigFromParentConfig (configstack/stack_test.go:281–364) is 83 lines long—exceeding the 60-line lint threshold. Consider extracting setup logic or assertions into helper functions or subtests to improve readability and satisfy golangci-lint.
🤖 Prompt for AI Agents
In configstack/stack_test.go at line 281, the test function
TestResolveTerraformModulesReadConfigFromParentConfig is 83 lines long,
exceeding the 60-line limit set by golangci-lint. Refactor this test by
extracting setup code, repeated logic, or assertion blocks into separate helper
functions or subtests to reduce the main test function length and improve
readability while maintaining all existing calls to
stack.ResolveTerraformModules with t.Context().
Description
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Chores
Refactor