Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughUpdates the CI lint workflow with OS-aware cache paths and matrix testing, adds tag-based filtering for Go builds to the Makefile, and makes various test formatting improvements and assertion enhancements across integration tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 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 (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✏️ Tip: You can disable this entire section by setting Comment |
yhakbar
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/lint.yml (1)
51-67: Cache keys contain hardcodedlinux-amd64suffix despite multi-OS matrix.The cache keys at lines 55, 61, and 67 include
-linux-amd64suffix, but this workflow now runs on bothubuntu-latestandmacos-latest. This will cause:
- macOS jobs to potentially restore Linux caches (architecture mismatch)
- Suboptimal cache utilization across different OS runners
The
${{ runner.os }}prefix helps, but the suffix should match the actual platform.🔧 Suggested fix
- name: Go Build Cache uses: actions/cache@v5 with: path: ${{ steps.go-cache-paths.outputs.go-build }} - key: ${{ runner.os }}-go-build-${{ hashFiles('**/go.sum') }}-linux-amd64 + key: ${{ runner.os }}-go-build-${{ hashFiles('**/go.sum') }} - name: Go Mod Cache uses: actions/cache@v5 with: path: ${{ steps.go-cache-paths.outputs.go-mod }} - key: ${{ runner.os }}-go-mod-${{ hashFiles('**/go.sum') }}-linux-amd64 + key: ${{ runner.os }}-go-mod-${{ hashFiles('**/go.sum') }} - name: golangci-lint Cache uses: actions/cache@v5 with: path: ${{ steps.go-cache-paths.outputs.golanci-lint-cache }} - key: ${{ runner.os }}-golangci-lint-${{ hashFiles('**/go.sum') }}-linux-amd64 + key: ${{ runner.os }}-golangci-lint-${{ hashFiles('**/go.sum') }}
🤖 Fix all issues with AI agents
In @.github/workflows/lint.yml:
- Around line 36-49: Rename the misspelled output variable "golanci-lint-cache"
to "golangci-lint-cache" wherever it is set and referenced (e.g., the echo
statements in the conditional blocks for RUNNER_OS and the later usage around
the other occurrence mentioned), ensuring all occurrences (including the other
line referenced) are updated to the correct spelling so the workflow output
variable name is consistent.
🧹 Nitpick comments (4)
Makefile (1)
43-49: Consider excluding vendor and hidden directories from tag scanning.The
grep -rh 'go:build' .command will scan all directories includingvendor/and.git/, which may include unintended build tags or slow down the scan. Consider adding exclusions:♻️ Suggested improvement
-LINT_TAGS := $(shell grep -rh 'go:build' . | \ +LINT_TAGS := $(shell grep -rh 'go:build' . --include='*.go' | \ + grep -v '^vendor/' | \ sed 's/.*go:build\s*//' | \ tr -cs '[:alnum:]_' '\n' | \ grep -vE '^($(IGNORE_TAGS))$$' | \ sed '/^$$/d' | \ sort -u | \ paste -sd, -)Alternatively, use
git ls-filesorfindwith exclusions to avoid scanning vendored dependencies.test/integration_gcp_test.go (1)
454-456: Adddefer gcsClient.Close()for consistent resource cleanup in test functions.The
gcsObjectAttrsfunction at lines 454-456 creates a GCS client but doesn't close it. WhiledoesGCSBucketObjectExistincludes the proper defer statement, the same pattern is missing ingcsObjectAttrs,validateGCSBucketExistsAndIsLabeled,createGCSBucket, anddeleteGCSBucket. Apply the defer statement consistently across all GCS client creations in this test file to ensure proper resource cleanup.Example fix
gcsClient, err := gcsbackend.NewClient(ctx, extGCSCfg) require.NoError(t, err, "Error creating GCS client") + + defer gcsClient.Close() bucket := gcsClient.Bucket(bucketName)test/integration_engine_test.go (1)
382-388: Consider error handling pattern.The
if err := os.MkdirAll(...); err != nil { require.NoError(t, err) }pattern at line 383-385 is redundant. Iferr != nil, callingrequire.NoError(t, err)is equivalent to just callingrequire.NoError(t, err)directly after the assignment.♻️ Suggested simplification
- if err := os.MkdirAll(engineDir, 0755); err != nil { - require.NoError(t, err) - } + err := os.MkdirAll(engineDir, 0755) + require.NoError(t, err)test/integration_aws_test.go (1)
2064-2069: Suppressed error masks all failures, not just missing encryption config.The
nolint:nilerrsuppression returnsnil, nilwhen GetBucketEncryption fails, masking network errors, permission issues, and service failures. Compare this to thebucketPolicyfunction in the same file (lines 2128-2144), which correctly propagates errors. The TODO comment in the code confirms this is known technical debt. Consider catching the specific "no encryption configured" error (NoSuchServerSideEncryptionConfiguration from the AWS SDK) and only suppressing that, while allowing actual API errors to propagate and fail the test appropriately.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/lint.ymlMakefileinternal/awshelper/config_test.gointernal/awshelper/policy_test.gointernal/remotestate/backend/s3/client_test.gotest/integration_aws_oidc_test.gotest/integration_aws_test.gotest/integration_engine_test.gotest/integration_gcp_test.gotest/integration_parse_test.gotest/integration_private_registry_test.gotest/integration_s3_encryption_test.gotest/integration_scaffold_ssh_test.gotest/integration_serial_aws_test.gotest/integration_serial_gcp_test.gotest/integration_tflint_test.gotest/integration_units_reading_test.gotf/getproviders/lock_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
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.
Files:
internal/awshelper/policy_test.gotest/integration_private_registry_test.gotest/integration_serial_gcp_test.gotest/integration_gcp_test.gotest/integration_aws_oidc_test.gotest/integration_tflint_test.gotest/integration_parse_test.gotest/integration_serial_aws_test.gotest/integration_s3_encryption_test.gotf/getproviders/lock_test.gotest/integration_engine_test.gointernal/awshelper/config_test.gointernal/remotestate/backend/s3/client_test.gotest/integration_units_reading_test.gotest/integration_aws_test.gotest/integration_scaffold_ssh_test.go
🧠 Learnings (5)
📚 Learning: 2025-06-18T22:17:16.375Z
Learnt from: wazy
Repo: gruntwork-io/terragrunt PR: 4445
File: tf/cliconfig/credentials.go:58-65
Timestamp: 2025-06-18T22:17:16.375Z
Learning: In the terragrunt codebase, the hostSuffixCredentialsFromEnv function in tf/cliconfig/credentials.go uses broad suffix matching intentionally. The user wants any host ending with a configured credential key to match, not just subdomain-specific matching. For example, abc.xyz.example.com should match credentials for example.com using strings.HasSuffix.
Applied to files:
test/integration_private_registry_test.go
📚 Learning: 2025-03-06T23:44:09.413Z
Learnt from: partcyborg
Repo: gruntwork-io/terragrunt PR: 3974
File: config/config_partial.go:448-456
Timestamp: 2025-03-06T23:44:09.413Z
Learning: The TerragruntConfig struct in config/config.go does contain an Engine field that's used to store engine configuration data.
Applied to files:
test/integration_engine_test.go
📚 Learning: 2025-11-03T04:40:01.000Z
Learnt from: ThisGuyCodes
Repo: gruntwork-io/terragrunt PR: 5041
File: test/fixtures/hclvalidate/valid/duplicate-attributes-in-required-providers/main.tf:2-7
Timestamp: 2025-11-03T04:40:01.000Z
Learning: In the terragrunt repository, test fixtures under test/fixtures/hclvalidate/valid/ are intentionally testing that INPUT validation succeeds even when Terraform code contains syntax errors or other issues unrelated to input validation (e.g., duplicate attributes, circular references, invalid locals). The "valid" designation means "valid for input validation purposes" not "syntactically valid Terraform/OpenTofu code".
Applied to files:
test/integration_engine_test.gotest/integration_scaffold_ssh_test.go
📚 Learning: 2025-02-10T13:36:19.542Z
Learnt from: levkohimins
Repo: gruntwork-io/terragrunt PR: 3723
File: cli/commands/stack/action.go:160-160
Timestamp: 2025-02-10T13:36:19.542Z
Learning: The project uses a custom error package `github.com/gruntwork-io/terragrunt/internal/errors` which provides similar functionality to `fmt.Errorf` but includes stack traces. Prefer using this package's error functions (e.g., `errors.Errorf`, `errors.New`) over the standard library's error handling.
Applied to files:
test/integration_aws_test.gotest/integration_scaffold_ssh_test.go
📚 Learning: 2025-08-19T16:05:54.723Z
Learnt from: Resonance1584
Repo: gruntwork-io/terragrunt PR: 4683
File: go.mod:86-90
Timestamp: 2025-08-19T16:05:54.723Z
Learning: When analyzing Go module dependencies for removal, always check for both direct imports and API usage across all Go files in the repository, not just a quick search. The github.com/mattn/go-zglob library is used for filesystem walking and glob expansion in multiple Terragrunt files including util/file.go, format commands, and AWS provider patch functionality.
Applied to files:
test/integration_scaffold_ssh_test.go
🧬 Code graph analysis (3)
test/integration_serial_gcp_test.go (1)
test/helpers/package.go (2)
CreateTmpTerragruntConfigContent(138-150)RemoveFile(971-979)
test/integration_engine_test.go (1)
test/helpers/package.go (2)
RunTerragruntCommandWithOutput(1085-1089)CopyAndFillMapPlaceholders(163-176)
test/integration_aws_test.go (4)
internal/awshelper/policy.go (1)
Statement(15-23)internal/remotestate/backend/s3/client.go (1)
SidEnforcedTLSPolicy(27-27)test/helpers/package.go (2)
DeleteS3Bucket(235-285)TerraformRemoteStateS3Region(61-61)internal/discovery/discovery.go (1)
String(340-342)
🔇 Additional comments (29)
tf/getproviders/lock_test.go (1)
40-40: LGTM!Minor formatting improvement that adds visual separation between error handling and the subsequent hash computation within the loop.
test/integration_scaffold_ssh_test.go (1)
115-118: LGTM! The new assertion correctly validates thatfind_in_parent_foldersis excluded whenEnableRootInclude: false.Minor note: Consider using
filepath.Join(tmpEnvPath, "terragrunt.hcl")for cross-platform path handling, especially given the PR's Windows-related fixes. That said, the current approach is consistent with existing patterns in this file (lines 61, 62, 94).test/integration_serial_gcp_test.go (2)
23-24: Formatting improvement enhances readability.The blank line addition appropriately separates the defer cleanup from the environment variable manipulation, improving visual organization of the test setup.
49-80: Whitespace additions improve test structure clarity.The blank lines effectively separate logical blocks (setup, execution, assertions), making the test flow easier to follow.
internal/awshelper/policy_test.go (1)
85-87: LGTM!The added blank line after
t.Parallel()improves consistency with the other test function in this file.test/integration_tflint_test.go (2)
93-110: LGTM!The added blank lines after
t.Cleanupblocks and between logical sections improve code readability by visually separating cleanup registration from test logic.
119-136: Consistent formatting pattern.The blank line additions after
t.Cleanupblocks throughout this file maintain consistency with the established pattern.internal/remotestate/backend/s3/client_test.go (3)
74-83: LGTM!The added blank lines around the goroutine creation improve readability by separating the loop increment, goroutine setup, and goroutine body.
186-189: LGTM!The blank line before
return nilprovides visual separation after therequire.Failfcall.
224-226: LGTM!The blank line after
t.Helper()is consistent with other helper functions in this file.test/integration_s3_encryption_test.go (2)
139-152: LGTM!The added blank lines after
require.NoError(t, err)improve readability by separating error validation from subsequent test logic.
183-204: LGTM!Consistent formatting pattern maintained—blank lines separate error checks from the processing of command outputs.
test/integration_serial_aws_test.go (4)
89-93: LGTM!The blank line after capturing the original environment variables improves readability before the
t.Setenvcalls.
126-136: LGTM!The added blank lines provide clear visual separation between the time parsing loop, sorting, and the subsequent scaling factor computation.
147-159: LGTM!The blank lines improve readability by separating the scaling factor loop from the scaled times computation and the final assertion loop.
170-181: LGTM!The blank line after the error check and before
os.Renameprovides cleaner visual separation of the copy and rename operations in the loop.test/integration_aws_oidc_test.go (1)
220-244: LGTM - Improved resource management and error handling.The addition of
defer resp.Body.Close()on line 223 ensures proper cleanup of the HTTP response body. The error handling for non-OK status codes and JSON unmarshalling is thorough and well-structured.internal/awshelper/config_test.go (1)
36-80: LGTM - Proper parallel test execution.Adding
t.Parallel()both at the test function level (line 37) and within each subtest (line 74) correctly enables parallel execution of test cases. This is the idiomatic Go pattern for table-driven tests with parallel execution.test/integration_units_reading_test.go (2)
174-182: LGTM - Formatting consistency.The blank line after the slice initialization improves readability and maintains consistent formatting across the test file.
316-318: LGTM - Simplified string construction.Using string concatenation instead of
fmt.Sprintffor simple path joining is cleaner and removes thefmtimport dependency.Makefile (1)
51-53: LGTM - Tag-aware linting.The
run-linttarget now correctly passes collected build tags to golangci-lint viaGOFLAGS, ensuring all code paths are linted regardless of build constraints.test/integration_gcp_test.go (1)
420-423: Good addition of resource cleanup.Adding
defer gcsClient.Close()ensures proper cleanup of the GCS client connection.test/integration_private_registry_test.go (2)
26-28: LGTM! Variable naming follows Go conventions.The rename from
registryUrltoregistryURLcorrectly follows Go's naming convention where acronyms like URL should be fully capitalized.
75-75: LGTM! Simplified string concatenation.Using direct string concatenation
"TF_TOKEN_"+hostinstead offmt.Sprintfis appropriate for simple string concatenation and removes the unnecessaryfmtimport dependency.test/integration_parse_test.go (1)
237-248: LGTM! Well-guarded assertions with improved formatting.The conditional assertions correctly guard against missing fixtures (index == -1), and the added blank lines improve readability between the assertion blocks.
test/integration_engine_test.go (1)
297-301: LGTM! Unused variable replaced with blank identifier.Using
_for the unusedstdoutreturn value is correct Go practice and eliminates unnecessary variable allocation.test/integration_aws_test.go (2)
621-623: LGTM! Helpful debug logging added.Adding the bucket name log statement aids in debugging test failures, especially useful when tests run in parallel with unique bucket names.
716-740: LGTM! Enhanced validation for access logging bucket security.The additional assertions verify that the access logging bucket has proper SSE configuration and enforces TLS via bucket policy. This improves test coverage for security-related settings.
.github/workflows/lint.yml (1)
8-16: LGTM! Multi-OS matrix strategy for comprehensive linting.The matrix strategy enables linting across Ubuntu and macOS, which helps catch OS-specific issues. The comment explaining why Windows is excluded (line ending requirements) provides good context.
yhakbar
force-pushed
the
chore/lint-everything-when-linting
branch
from
47c145c to
4701d8d
Compare
2 participants
Description
Ensures that we always lint across all available build flags when we lint.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.