Skip to content

chore: Upgrade golangci-lint to v2.8.0#5365

Merged
yhakbar merged 17 commits intomainfrom
chore/adding-more-linting
Jan 22, 2026
Merged

chore: Upgrade golangci-lint to v2.8.0#5365
yhakbar merged 17 commits intomainfrom
chore/adding-more-linting

Conversation

@yhakbar
Copy link
Copy Markdown
Collaborator

@yhakbar yhakbar commented Jan 15, 2026

Description

Upgrades golangci-lint to v2.8.0.

Addresses lints that were failing as a consequence in prealloc and wsl_v5.

TODOs

Read the Gruntwork contribution guidelines.

  • I authored this code entirely myself
  • I am submitting code based on open source software (e.g. MIT, MPL-2.0, Apache)]
  • I am adding or upgrading a dependency or adapted code and confirm it has a compatible open source license
  • Update the docs.
  • Run the relevant tests successfully, including pre-commit checks.
  • Include release notes. If this PR is backward incompatible, include a migration guide.

Release Notes (draft)

Added / Removed / Updated [X].

Migration Guide

Summary by CodeRabbit

  • New Features

    • Added a Makefile target to run linters with automatic fixes.
    • Added a parse-depth guard that surfaces a clear error when config nesting is excessive.
  • Chores

    • Bumped golangci-lint to v2.8.0 and re-enabled several linters.
  • Bug Fixes

    • Filter-affected flag now falls back to the current directory when no workdir is detected.
    • Adjusted logging to avoid unintended Terraform stdout forwarding.
    • Improved handling of external filters for list/find commands.
  • Tests

    • Expanded and hardened unit/integration tests and assertions.

✏️ Tip: You can customize this high-level summary in your review settings.

@vercel
Copy link
Copy Markdown

vercel bot commented Jan 15, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
terragrunt-docs Ready Ready Preview, Comment Jan 22, 2026 8:33pm

Request Review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Jan 15, 2026

📝 Walkthrough

Walkthrough

Widespread refactor: added a Makefile lint-fix target and tooling bump; converted many value receivers/parameters to pointers (parsing context, dependencies, AWS config, codegen, hooks, URL args); added parse-depth guard; replaced many slice/appends with capacity-aware allocations and slices.Concat; minor test/formatting and linter directive changes.

Changes

Cohort / File(s) Summary
Build & Lint
Makefile, mise.toml, .golangci.yml
Added run-lint-fix Make target; bumped golangci-lint version; re-enabled previously disabled linters.
Parsing / Config API
pkg/config/parsing_context.go, pkg/config/errors.go, pkg/config/dependency.go, pkg/config/include.go
ParsingContext methods -> pointer receivers; added ParseDepth/MaxParseDepth and WithIncrementedDepth; new MaxParseDepthError; many Dependency methods changed to pointer receivers and DeepMerge signature updated.
Codegen & GenerateConfig
internal/codegen/generate.go, internal/codegen/generate_test.go, internal/runner/run/run.go, internal/remotestate/config.go
WriteToFile now accepts *GenerateConfig; call sites updated to pass pointers.
Discovery / CLI find & list
internal/discovery/constructor.go, internal/discovery/discovery.go, internal/cli/commands/find/find.go, internal/cli/commands/find/cli.go, internal/cli/commands/list/*
NewForDiscoveryCommand now accepts *DiscoveryCommandOptions (nil-check added); find/list External flag now appends {./**}... to filter queries.
Hooks & TFLint
internal/runner/run/hook.go, internal/tflint/tflint.go, internal/runner/run/download_source.go
Hook params switched to *runcfg.Hook; iteration moved to index/pointer style; hook error aggregation uses slice + strings.Join; tflint arg construction preallocated; includeInCopy uses slices.Concat.
AWS SDK & callers
internal/awshelper/config.go, internal/remotestate/backend/s3/client.go, test/integration_aws_test.go, internal/remotestate/*
Many helpers changed from aws.Config to *aws.Config; callers updated to pass pointers; related call sites adjusted.
TF getters & provider models
internal/tf/getter.go, internal/tf/getter_test.go, internal/tf/cache/models/provider.go
Several functions now take *url.URL; ResolveRelativeReferences receiver changed to pointer and returns a cloned pointer.
Slice preallocation & concat (widespread)
many internal/*, pkg/*, test/* (examples: internal/clihelper/*, internal/filter/*, internal/runner/*, pkg/config/*, test/integration_*)
Replaced nil/empty slice literals with make(..., 0, cap) and swapped some append(..., ...) usages for slices.Concat(...) to preallocate capacity.
String builders & concat optimizations
pkg/log/format/placeholders/placeholder.go, internal/runner/runnerpool/runner.go, internal/tf/getproviders/lock_test.go, test/integration_filter_test.go, test/integration_hcl_filter_test.go
Replaced repeated string concatenation with strings.Builder in loops for performance.
CLI flags & logging behavior
internal/cli/flags/global/flags.go, internal/cli/flags/shared/filter.go, internal/cli/commands/list/list.go
Adjusted TF stdout forwarding logic for log levels; added fallback workDir "." for filter handling; discovery call now passes pointer options.
Tests & assertions
many internal/*_test.go, test/*
Tests updated for new signatures, preallocated slices, use of assert.ErrorAs/regexp.MustCompile, added unmarshalling checks and unit-directory detection tweaks.
Lint suppressions
internal/cli/commands/catalog/tui/*.go, internal/util/shell.go
Added //nolint:gocritic annotations to multiple methods and some ProcessExecutionError methods.
Minor formatting & micro-optimizations
assorted internal/*, pkg/*, test/*
Numerous blank-line edits, index-based loops, small builders, and allocation tweaks without behavioral change.

Sequence Diagram(s)

(Skipped)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • ThisGuyCodes
  • denis256
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is incomplete. While it explains the upgrade and mentions addressing lint failures, the Release Notes section contains only placeholder text and lacks specific details about the changes. Complete the Release Notes with a concrete one-line summary of changes (e.g., 'Upgraded golangci-lint to v2.8.0 and addressed prealloc and wsl_v5 linter issues') and clarify any backward compatibility concerns.
Docstring Coverage ⚠️ Warning Docstring coverage is 54.87% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and concisely describes the main change: upgrading golangci-lint from v2.4.0 to v2.8.0, which is the primary modification shown in mise.toml.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

@yhakbar yhakbar changed the title chore/adding more linting chore: Adding more linting Jan 15, 2026
@yhakbar yhakbar force-pushed the chore/adding-more-linting branch from 10c0767 to 045ad8d Compare January 15, 2026 21:20
@yhakbar yhakbar force-pushed the chore/adding-more-linting branch from 045ad8d to eca3ac3 Compare January 21, 2026 21:44
@yhakbar yhakbar changed the title chore: Adding more linting chore: Upgrade golangci-lint to v2.8.0 Jan 21, 2026
@yhakbar yhakbar marked this pull request as ready for review January 21, 2026 22:24
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/discovery/discovery_test.go (1)

169-179: The test name creates semantic ambiguity about external component handling. Line 177 asserts an external dependency edge even though the case is named "discovery without external dependencies." If the intent is "exclude external components from returned results but keep them in dependency edges," consider renaming the test (e.g., "discovery excluding external components from results") or adding a clarifying comment to make the distinction from the "with dependencies" case explicit.

🤖 Fix all issues with AI agents
In `@internal/cli/flags/global/flags.go`:
- Around line 102-106: Setter for the flag currently always sets
opts.ForwardTFStdout = true regardless of val; change it so ForwardTFStdout is
only enabled when val is true (e.g., inside the Setter in flags.go, after
l.Formatter().SetDisabledOutput(val) add a conditional: if val {
opts.ForwardTFStdout = true } ), so providing --log-disable=false does not flip
ForwardTFStdout. Keep the rest of the Setter unchanged.

In `@Makefile`:
- Around line 42-45: Add the run-lint-fix target to the Makefile's .PHONY list
so the make target always runs instead of being blocked by a same-named file;
update the .PHONY declaration to include run-lint-fix alongside existing phony
targets (reference the run-lint-fix target and the .PHONY declaration in the
Makefile).
🧹 Nitpick comments (9)
pkg/config/cty_helpers.go (1)

145-148: Consider preallocating here for consistency.

This follows the same pattern as wrapStaticValueToStringSliceAsFuncImpl (line 161), which was updated to preallocate. For consistency and the same allocation efficiency benefit:

♻️ Suggested change
-			outCtyVals := []cty.Value{}
+			outCtyVals := make([]cty.Value, 0, len(outVals))
 			for _, val := range outVals {
 				outCtyVals = append(outCtyVals, cty.StringVal(val))
 			}
internal/worktrees/worktrees_test.go (1)

472-483: Factor out repeated unit-directory setup helper.
The unit-directory creation and terragrunt.hcl write logic is duplicated across multiple setupFilesystem blocks. A small helper would make the test setup more concise and easier to maintain.

♻️ Suggested refactor
 func TestExpandWithUnitDirectoryDetection(t *testing.T) {
 	t.Parallel()
+
+	createUnitDir := func(tmpDir, name string) error {
+		unitDir := filepath.Join(tmpDir, name)
+		if err := os.MkdirAll(unitDir, 0755); err != nil {
+			return err
+		}
+		terragruntFile := filepath.Join(unitDir, "terragrunt.hcl")
+		return os.WriteFile(terragruntFile, []byte("# terragrunt config"), 0644)
+	}
 
 	tests := []struct {
 		name               string
 		setupFilesystem    func(tmpDir string) error
@@
 		{
 			name: "removed file in unit directory creates path filter",
 			setupFilesystem: func(tmpDir string) error {
 				// Create unit directory with terragrunt.hcl
-				unitDir := filepath.Join(tmpDir, "unit1")
-				if err := os.MkdirAll(unitDir, 0755); err != nil {
-					return err
-				}
-
-				terragruntFile := filepath.Join(unitDir, "terragrunt.hcl")
-
-				return os.WriteFile(terragruntFile, []byte("# terragrunt config"), 0644)
+				return createUnitDir(tmpDir, "unit1")
 			},
@@
 		{
 			name: "added file in unit directory creates path filter",
 			setupFilesystem: func(tmpDir string) error {
 				// Create unit directory with terragrunt.hcl
-				unitDir := filepath.Join(tmpDir, "unit1")
-				if err := os.MkdirAll(unitDir, 0755); err != nil {
-					return err
-				}
-
-				terragruntFile := filepath.Join(unitDir, "terragrunt.hcl")
-
-				return os.WriteFile(terragruntFile, []byte("# terragrunt config"), 0644)
+				return createUnitDir(tmpDir, "unit1")
 			},
@@
 		{
 			name: "changed file in unit directory creates path filter",
 			setupFilesystem: func(tmpDir string) error {
 				// Create unit directory with terragrunt.hcl
-				unitDir := filepath.Join(tmpDir, "unit1")
-				if err := os.MkdirAll(unitDir, 0755); err != nil {
-					return err
-				}
-
-				terragruntFile := filepath.Join(unitDir, "terragrunt.hcl")
-
-				return os.WriteFile(terragruntFile, []byte("# terragrunt config"), 0644)
+				return createUnitDir(tmpDir, "unit1")
 			},
@@
 		{
 			name: "mixed scenarios with multiple units and non-units",
 			setupFilesystem: func(tmpDir string) error {
 				// Create unit1 directory
-				unit1Dir := filepath.Join(tmpDir, "unit1")
-				if err := os.MkdirAll(unit1Dir, 0755); err != nil {
-					return err
-				}
-
-				terragruntFile1 := filepath.Join(unit1Dir, "terragrunt.hcl")
-				if err := os.WriteFile(terragruntFile1, []byte("# terragrunt config"), 0644); err != nil {
+				if err := createUnitDir(tmpDir, "unit1"); err != nil {
 					return err
 				}
@@
 				// Create unit2 directory
-				unit2Dir := filepath.Join(tmpDir, "unit2")
-				if err := os.MkdirAll(unit2Dir, 0755); err != nil {
-					return err
-				}
-
-				terragruntFile2 := filepath.Join(unit2Dir, "terragrunt.hcl")
-				if err := os.WriteFile(terragruntFile2, []byte("# terragrunt config"), 0644); err != nil {
+				if err := createUnitDir(tmpDir, "unit2"); err != nil {
 					return err
 				}

Also applies to: 510-521, 548-559, 586-614

pkg/log/format/placeholders/placeholder.go (1)

67-81: Consider returning the builder output directly.
str is always empty, so the extra concatenation adds an allocation without benefit.

♻️ Proposed simplification
 func (phs Placeholders) Format(data *options.Data) (string, error) {
-	var str string
-
-	var strSb69 strings.Builder
+	var strSb69 strings.Builder
 
 	for _, ph := range phs {
 		s, err := ph.Format(data)
 		if err != nil {
 			return "", err
 		}
 
 		strSb69.WriteString(s)
 	}
 
-	str += strSb69.String()
-
-	return str, nil
+	return strSb69.String(), nil
 }
internal/tf/getproviders/lock_test.go (1)

36-48: Rename the builder variable for readability.
documentSb35 reads like an auto-generated temp name and makes the test harder to scan. Consider a clearer name like documentBuilder or docBuilder.

♻️ Suggested rename
-	var documentSb35 strings.Builder
+	var documentBuilder strings.Builder
@@
-		documentSb35.WriteString(fmt.Sprintf("%s %s\n", sha, packageName))
+		documentBuilder.WriteString(fmt.Sprintf("%s %s\n", sha, packageName))
@@
-	document += documentSb35.String()
+	document += documentBuilder.String()
internal/runner/runnerpool/runner.go (1)

729-741: Prefer a descriptive builder name.
outStrSb729 isn’t meaningful; a clearer name improves readability in this hot path.

♻️ Suggested rename
-	var outStrSb729 strings.Builder
+	var outStrBuilder strings.Builder
@@
-		outStrSb729.WriteString(fmt.Sprintf("- Unit %s\n", unitPath))
+		outStrBuilder.WriteString(fmt.Sprintf("- Unit %s\n", unitPath))
@@
-	outStr += outStrSb729.String()
+	outStr += outStrBuilder.String()
pkg/config/config_helpers_test.go (1)

23-36: Consider using errors.As instead of manual unwrap loop.

The standard library's errors.As function already handles error chain traversal and type checking. This would be more idiomatic and handle edge cases like pointer vs value receivers correctly.

♻️ Suggested refactor using errors.As
-// assertErrorType checks that the error chain contains an error of the same type as expectedErr.
-func assertErrorType(t *testing.T, expectedErr, actualErr error) bool {
-	t.Helper()
-
-	expectedType := reflect.TypeOf(expectedErr)
-
-	for err := actualErr; err != nil; err = errors.Unwrap(err) {
-		if reflect.TypeOf(err) == expectedType {
-			return true
-		}
-	}
-
-	return assert.Fail(t, "error type mismatch", "expected error of type %T in chain, but got %T", expectedErr, actualErr)
-}
+// assertErrorType checks that the error chain contains an error of the same type as expectedErr.
+func assertErrorType(t *testing.T, expectedErr, actualErr error) bool {
+	t.Helper()
+
+	// Create a pointer to the expected error type for errors.As
+	expectedType := reflect.TypeOf(expectedErr)
+	target := reflect.New(expectedType).Interface()
+
+	if errors.As(actualErr, target) {
+		return true
+	}
+
+	return assert.Fail(t, "error type mismatch", "expected error of type %T in chain, but got %T", expectedErr, actualErr)
+}

Note: The current implementation works correctly for the test cases shown, but errors.As is the canonical way to check for error types in a chain and handles more edge cases.

test/integration_filter_test.go (1)

551-558: Variable naming could be improved.

The variable name cmdSb551 appears to include a line number suffix, which is unusual and will become misleading if code is refactored. Consider a more descriptive name.

♻️ Suggested naming improvement
-			var cmdSb551 strings.Builder
+			var filterArgsBuilder strings.Builder

 			for _, filter := range tc.filterQueries {
-				cmdSb551.WriteString(" --filter " + filter)
+				filterArgsBuilder.WriteString(" --filter " + filter)
 			}

-			cmd += cmdSb551.String()
+			cmd += filterArgsBuilder.String()
test/integration_hcl_filter_test.go (2)

152-160: Variable naming and pattern could be improved.

The variable name filterStrSb152 includes a line number suffix which is unusual. Additionally, the pattern initializes filterStr to empty string and then only appends the builder result, which could be simplified.

♻️ Suggested improvements
 			// Build filter arguments
-			filterStr := ""
-
-			var filterStrSb152 strings.Builder
-
+			var filterArgs strings.Builder
 			for _, filter := range tc.filterArgs {
-				filterStrSb152.WriteString(fmt.Sprintf(" --filter '%s'", filter))
+				filterArgs.WriteString(fmt.Sprintf(" --filter '%s'", filter))
 			}

-			filterStr += filterStrSb152.String()
-
 			cmd := fmt.Sprintf(
 				"terragrunt hcl fmt %s --check --working-dir %s",
-				filterStr,
+				filterArgs.String(),
 				rootPath,
 			)

262-270: Same pattern improvements apply here.

This is the same pattern with odd variable naming (filterStrSb256) that could be simplified.

♻️ Suggested improvements
 			// Build filter arguments with proper quoting
-			filterStr := ""
-
-			var filterStrSb256 strings.Builder
-
+			var filterArgs strings.Builder
 			for _, filter := range tc.filterArgs {
-				filterStrSb256.WriteString(fmt.Sprintf(" --filter '%s'", filter))
+				filterArgs.WriteString(fmt.Sprintf(" --filter '%s'", filter))
 			}

-			filterStr += filterStrSb256.String()
-
-			cmd := fmt.Sprintf("terragrunt hcl validate%s --working-dir %s", filterStr, rootPath)
+			cmd := fmt.Sprintf("terragrunt hcl validate%s --working-dir %s", filterArgs.String(), rootPath)

Comment on lines 102 to +106
Setter: func(val bool) error {
l.Formatter().SetDisabledOutput(val)

opts.ForwardTFStdout = true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Guard stdout forwarding to only when logging is disabled.

Right now ForwardTFStdout is set to true even when --log-disable=false is explicitly provided, which changes behavior for callers that set the flag to false. Prefer only toggling when val is true (or leave unchanged when false).

🔧 Proposed fix
-			Setter: func(val bool) error {
-				l.Formatter().SetDisabledOutput(val)
-
-				opts.ForwardTFStdout = true
-
-				return nil
-			},
+			Setter: func(val bool) error {
+				l.Formatter().SetDisabledOutput(val)
+				if val {
+					opts.ForwardTFStdout = true
+				}
+				return nil
+			},
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Setter: func(val bool) error {
l.Formatter().SetDisabledOutput(val)
opts.ForwardTFStdout = true
Setter: func(val bool) error {
l.Formatter().SetDisabledOutput(val)
if val {
opts.ForwardTFStdout = true
}
return nil
},
🤖 Prompt for AI Agents
In `@internal/cli/flags/global/flags.go` around lines 102 - 106, Setter for the
flag currently always sets opts.ForwardTFStdout = true regardless of val; change
it so ForwardTFStdout is only enabled when val is true (e.g., inside the Setter
in flags.go, after l.Formatter().SetDisabledOutput(val) add a conditional: if
val { opts.ForwardTFStdout = true } ), so providing --log-disable=false does not
flip ForwardTFStdout. Keep the rest of the Setter unchanged.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
internal/tf/getter_test.go (1)

94-99: Rebind loop variable before parallel subtests.

With t.Parallel() and taking &tc.moduleURL, all subtests can capture the last tc value. Rebind tc inside the loop to avoid flakiness.

🛠️ Proposed fix
 for _, tc := range testCases {
+	tc := tc
 	t.Run(tc.name, func(t *testing.T) {
 		t.Parallel()
 
 		downloadURL, err := tf.GetDownloadURLFromHeader(&tc.moduleURL, tc.terraformGet)
internal/tf/getter.go (2)

282-288: Guard against nil URL input.

The signature now accepts *url.URL, but the function dereferences it unconditionally. Add a nil check to avoid panics in external callers.

🛠️ Proposed fix
 func GetTerraformGetHeader(ctx context.Context, logger log.Logger, url *url.URL) (string, error) {
+	if url == nil {
+		return "", errors.New(ModuleDownloadErr{sourceURL: "", details: "nil module URL"})
+	}
 	body, header, err := httpGETAndGetResponse(ctx, logger, url)

319-331: Handle nil moduleURL before ResolveReference.

With pointer params, moduleURL.ResolveReference(...) will panic if moduleURL is nil. Consider an early guard (or a more specific guard only when terraformGet is relative).

🛠️ Proposed fix
 func GetDownloadURLFromHeader(moduleURL *url.URL, terraformGet string) (string, error) {
+	if moduleURL == nil {
+		return "", errors.New(errors.New("module URL is nil"))
+	}
 	// If url from X-Terrafrom-Get Header seems to be a relative url,
🤖 Fix all issues with AI agents
In `@internal/discovery/constructor.go`:
- Around line 42-43: Add a nil check at the start of NewForDiscoveryCommand to
guard against opts being nil: if opts == nil return nil, errors.New(...) using
the internal/errors package; reference the DiscoveryCommandOptions pointer and
the NewForDiscoveryCommand function so the check happens before any dereference
(e.g., before calling NewDiscovery(opts.WorkingDir)), and return a clear error
created via internal/errors to fail fast.

In `@internal/runner/run/hook.go`:
- Around line 46-61: The aggregated error string `result` is built with `result
= fmt.Sprintf("%s\n%s", result, errorMessage)` which inserts a leading newline
when `result` is empty; change the logic in the loop in hook.go (where `result`,
`errorMessage`, `err` and `originalError`/`processError` are handled) to avoid a
leading `\n` — e.g., append the first `errorMessage` directly when `result` is
empty and only prepend `\n` for subsequent messages, or collect messages into a
slice and use `strings.Join` before assigning to `result`, so OnErrors matching
is not broken by a leading newline.

In `@internal/tf/getter.go`:
- Around line 358-360: The function httpGETAndGetResponse currently calls
getURL.String() without checking getURL for nil, which can panic; update
httpGETAndGetResponse to validate getURL at the top (e.g., if getURL == nil) and
return a clear error (and no panic) along with nil body/header, so callers
receive a predictable error; reference the function name httpGETAndGetResponse
and the parameter getURL when making the change and ensure the returned error
message clearly states that getURL was nil.
🧹 Nitpick comments (1)
internal/cli/app_test.go (1)

463-482: Add nil guards for clearer test failures.

If either option is nil, this helper will panic before assertions run. Adding explicit nil checks improves diagnostics without changing behavior.

💡 Proposed tweak
 func assertOptionsEqual(t *testing.T, expected *options.TerragruntOptions, actual *options.TerragruntOptions, msgAndArgs ...any) {
 	t.Helper()
+
+	require.NotNil(t, expected, msgAndArgs...)
+	require.NotNil(t, actual, msgAndArgs...)
 
 	// Normalize path separators for cross-platform compatibility
 	expectedConfigPath := filepath.ToSlash(expected.TerragruntConfigPath)
 	actualConfigPath := filepath.ToSlash(actual.TerragruntConfigPath)

denis256
denis256 previously approved these changes Jan 22, 2026
@yhakbar yhakbar force-pushed the chore/adding-more-linting branch from 7215802 to 13fc94c Compare January 22, 2026 18:57
@yhakbar yhakbar marked this pull request as ready for review January 22, 2026 19:49
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
internal/tf/getter.go (2)

282-288: Nil dereference in error path.

If url is nil, httpGETAndGetResponse returns an error (due to its nil check), but line 287 then calls url.String() which panics before the error can be propagated. Since this is an exported function, add a nil guard.

🛠️ Proposed fix
 func GetTerraformGetHeader(ctx context.Context, logger log.Logger, url *url.URL) (string, error) {
+	if url == nil {
+		return "", errors.New("GetTerraformGetHeader received nil url")
+	}
+
 	body, header, err := httpGETAndGetResponse(ctx, logger, url)
 	if err != nil {
 		details := "error receiving HTTP data"
 
 		return "", errors.New(ModuleDownloadErr{sourceURL: url.String(), details: details})
 	}

319-335: Missing nil check for moduleURL.

If moduleURL is nil and terraformGet is a relative path, line 330 will panic on moduleURL.ResolveReference(). As an exported function, consider adding a nil guard for consistency with the defensive pattern applied to httpGETAndGetResponse.

🛠️ Proposed fix
 func GetDownloadURLFromHeader(moduleURL *url.URL, terraformGet string) (string, error) {
+	if moduleURL == nil {
+		return "", errors.New("GetDownloadURLFromHeader received nil moduleURL")
+	}
+
 	// If url from X-Terrafrom-Get Header seems to be a relative url,
🤖 Fix all issues with AI agents
In `@internal/awshelper/config.go`:
- Around line 137-146: The closures assigned to cfg.Credentials in
getWebIdentityCredentialsFromIAMRoleOptions and
getSTSCredentialsFromIAMRoleOptions capture *cfg and cause recursive credential
resolution; fix by capturing a copy of the current config (e.g., baseCfg := cfg
or baseCfg := *cfg) before you set cfg.Credentials and pass that copy into those
helper calls (so sts.NewFromConfig or any STS client uses the original
credentials source, not the provider being installed). Update calls to
getWebIdentityCredentialsFromIAMRoleOptions and
getSTSCredentialsFromIAMRoleOptions to receive the copied/base config instead of
a pointer to the cfg that's being modified.

In `@pkg/config/parsing_context.go`:
- Around line 173-186: The boundary check in ParsingContext.WithIncrementedDepth
is off-by-one: change the conditional that currently uses ctx.ParseDepth >
MaxParseDepth to use >= so that attempting to increment when ParseDepth is
already at MaxParseDepth returns the MaxParseDepthError; update the check
located in WithIncrementedDepth (referencing ctx.ParseDepth, MaxParseDepth and
MaxParseDepthError) accordingly so the function prevents creating a clone that
would exceed the documented maximum depth.
🧹 Nitpick comments (4)
pkg/log/format/placeholders/placeholder.go (1)

67-82: Remove redundant string accumulation to avoid an extra copy.

str starts empty and is only appended once, so str += strSb69.String() adds an unnecessary allocation. You can return the builder’s string directly and simplify the variable name.

♻️ Proposed simplification
-	var str string
-
-	var strSb69 strings.Builder
+	var sb strings.Builder
 
 	for _, ph := range phs {
 		s, err := ph.Format(data)
 		if err != nil {
 			return "", err
 		}
 
-		strSb69.WriteString(s)
+		sb.WriteString(s)
 	}
 
-	str += strSb69.String()
-
-	return str, nil
+	return sb.String(), nil
internal/cli/commands/catalog/tui/keys.go (2)

83-92: Prefer pointer receivers over nolint suppression here.
Since newDelegateKeyMap() already returns a pointer, you can switch these to pointer receivers to avoid copies and drop the //nolint:gocritic. If you keep the suppression, consider adding a brief rationale to make it self-documenting.

♻️ Proposed change
-func (d delegateKeyMap) ShortHelp() []key.Binding { //nolint:gocritic
+func (d *delegateKeyMap) ShortHelp() []key.Binding {
 	return []key.Binding{
 		d.choose,
 		d.scaffold,
 	}
 }

-func (d delegateKeyMap) FullHelp() [][]key.Binding { //nolint:gocritic
+func (d *delegateKeyMap) FullHelp() [][]key.Binding {
 	return [][]key.Binding{
 		{
 			d.choose,
 			d.scaffold,
 		},
 	}
 }

146-161: Consider pointer receivers to avoid large receiver copies.
If the gocritic warning is about copying pagerKeyMap, switching to pointer receivers removes the lint suppression and avoids copying a large struct. This will require returning a pointer from newPagerKeyMap and updating call sites to pass *pagerKeyMap where a help.KeyMap is expected.

♻️ Proposed change (receivers)
-func (keys pagerKeyMap) ShortHelp() []key.Binding { //nolint:gocritic
+func (keys *pagerKeyMap) ShortHelp() []key.Binding {
 	return []key.Binding{
 		keys.Up,
 		keys.Down,
 		keys.Navigation,
 		keys.NavigationBack,
 		keys.Choose,
 		keys.Scaffold,
 		keys.Help,
 		keys.Quit,
 	}
 }

-func (keys pagerKeyMap) FullHelp() [][]key.Binding { //nolint:gocritic
+func (keys *pagerKeyMap) FullHelp() [][]key.Binding {
 	return [][]key.Binding{
 		{keys.Up, keys.Down, keys.PageDown, keys.PageUp},                   // first column
 		{keys.Navigation, keys.NavigationBack, keys.Choose, keys.Scaffold}, // second column
 		{keys.Help, keys.Quit, keys.ForceQuit},                             // third column
 	}
 }
♻️ Follow-up change (outside this hunk)
-func newPagerKeyMap() pagerKeyMap {
-	return pagerKeyMap{
+func newPagerKeyMap() *pagerKeyMap {
+	return &pagerKeyMap{
 		KeyMap: viewport.KeyMap{
 			// ...
 		},
 		// ...
 	}
 }
internal/awshelper/config.go (1)

278-356: *Add nil guards for aws.Config inputs to prevent panics.

These exported helpers now dereference *aws.Config directly; a nil cfg will panic. Consider returning a clear error when cfg is nil (and mirror the guard in the other helpers that dereference cfg).

✅ Minimal defensive check
func GetAWSCallerIdentity(ctx context.Context, cfg *aws.Config) (*sts.GetCallerIdentityOutput, error) {
+	if cfg == nil {
+		return nil, errors.New("aws config is nil")
+	}
 	stsClient := sts.NewFromConfig(*cfg)
 	return stsClient.GetCallerIdentity(ctx, &sts.GetCallerIdentityInput{})
}

Comment on lines +173 to +186
// WithIncrementedDepth returns a new ParsingContext with incremented parse depth.
// Returns an error if the maximum depth would be exceeded.
func (ctx *ParsingContext) WithIncrementedDepth() (*ParsingContext, error) {
if ctx.ParseDepth > MaxParseDepth {
return nil, errors.New(MaxParseDepthError{
Depth: ctx.ParseDepth,
Max: MaxParseDepth,
})
}

c := ctx.Clone()
c.ParseDepth = ctx.ParseDepth + 1

return c, nil
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Off-by-one in depth boundary check.

The condition ctx.ParseDepth > MaxParseDepth allows the depth to reach 1001 before erroring (when MaxParseDepth is 1000). If the intent is to allow exactly 1000 levels of recursion, the check should use >=:

 func (ctx *ParsingContext) WithIncrementedDepth() (*ParsingContext, error) {
-	if ctx.ParseDepth > MaxParseDepth {
+	if ctx.ParseDepth >= MaxParseDepth {
 		return nil, errors.New(MaxParseDepthError{
 			Depth: ctx.ParseDepth,
 			Max:   MaxParseDepth,
 		})
 	}

This is a minor discrepancy since 1000 vs 1001 doesn't materially affect stack overflow protection, but it would make the behavior match the documented constant value.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// WithIncrementedDepth returns a new ParsingContext with incremented parse depth.
// Returns an error if the maximum depth would be exceeded.
func (ctx *ParsingContext) WithIncrementedDepth() (*ParsingContext, error) {
if ctx.ParseDepth > MaxParseDepth {
return nil, errors.New(MaxParseDepthError{
Depth: ctx.ParseDepth,
Max: MaxParseDepth,
})
}
c := ctx.Clone()
c.ParseDepth = ctx.ParseDepth + 1
return c, nil
// WithIncrementedDepth returns a new ParsingContext with incremented parse depth.
// Returns an error if the maximum depth would be exceeded.
func (ctx *ParsingContext) WithIncrementedDepth() (*ParsingContext, error) {
if ctx.ParseDepth >= MaxParseDepth {
return nil, errors.New(MaxParseDepthError{
Depth: ctx.ParseDepth,
Max: MaxParseDepth,
})
}
c := ctx.Clone()
c.ParseDepth = ctx.ParseDepth + 1
return c, nil
}
🤖 Prompt for AI Agents
In `@pkg/config/parsing_context.go` around lines 173 - 186, The boundary check in
ParsingContext.WithIncrementedDepth is off-by-one: change the conditional that
currently uses ctx.ParseDepth > MaxParseDepth to use >= so that attempting to
increment when ParseDepth is already at MaxParseDepth returns the
MaxParseDepthError; update the check located in WithIncrementedDepth
(referencing ctx.ParseDepth, MaxParseDepth and MaxParseDepthError) accordingly
so the function prevents creating a clone that would exceed the documented
maximum depth.

denis256
denis256 previously approved these changes Jan 22, 2026
@yhakbar yhakbar merged commit 2e0e80d into main Jan 22, 2026
93 of 94 checks passed
@yhakbar yhakbar deleted the chore/adding-more-linting branch January 22, 2026 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants