feat: Implementation of backend commands: bootstrap, delete
#4070
Conversation
|
@yhakbar, thanks for the review! 90% of the changes are just ported code. I didn't delve deeply into what it does, as it would have taken even more time. Let's make improvements to the old code in another PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/remotestate/backend/gcs/client.go (3)
40-114: Consider refactoring the credentials logic
The large if-else chain handling different credentials/env vars is correct in logic but somewhat verbose. You could consider collapsing or structuring it with helper methods for improved readability.
116-165: Duplicate checks forProjectandLocation
Notice lines 124–126 (and 128–130) repeat the same validations again at lines 135–137 (and 139–142). Consolidating these checks in one place or extracting them into a helper function would improve maintainability.func (client *Client) validateProjectLocation() error { + if client.Project == "" { + return errors.New(MissingRequiredGCSRemoteStateConfig("project")) + } + if client.Location == "" { + return errors.New(MissingRequiredGCSRemoteStateConfig("location")) + } + return nil } func (client *Client) CreateGCSBucketIfNecessary(ctx context.Context, bucketName string, opts *options.TerragruntOptions) error { if client.DoesGCSBucketExist(ctx, bucketName) { return nil } - if client.Project == "" { - return errors.New(MissingRequiredGCSRemoteStateConfig("project")) - } - if client.Location == "" { - return errors.New(MissingRequiredGCSRemoteStateConfig("location")) - } + if err := client.validateProjectLocation(); err != nil { + return err + } ... - if client.Project == "" { - return errors.New(MissingRequiredGCSRemoteStateConfig("project")) - } - if client.Location == "" { - return errors.New(MissingRequiredGCSRemoteStateConfig("location")) - } + if err := client.validateProjectLocation(); err != nil { + return err + } ... }
372-399: Potential performance concerns on large buckets
Deleting objects one at a time in a loop works but may be slow for buckets with a very large number of objects. In the future, consider batching or parallel deletions for improved performance, though this is not strictly necessary if your use cases involve small or moderate bucket sizes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
awshelper/config.go(12 hunks)awshelper/config_test.go(2 hunks)cli/commands/backend/delete/delete.go(1 hunks)cli/commands/run/download_source_test.go(0 hunks)cli/commands/run/run.go(11 hunks)docs/_docs/04_reference/05-strict-mode.md(2 hunks)internal/cli/bool_flag_test.go(0 hunks)internal/cli/command_test.go(0 hunks)internal/cli/generic_flag_test.go(0 hunks)internal/cli/map_flag_test.go(0 hunks)internal/cli/slice_flag_test.go(0 hunks)internal/cli/sort_test.go(0 hunks)internal/remotestate/backend/gcs/client.go(1 hunks)internal/remotestate/backend/s3/errors.go(1 hunks)internal/remotestate/backend/s3/remote_state_config_test.go(1 hunks)internal/strict/controls/controls.go(3 hunks)test/integration_graph_test.go(0 hunks)test/integration_list_test.go(0 hunks)tf/cliconfig/config_test.go(0 hunks)tf/getproviders/lock_test.go(0 hunks)tf/getproviders/package_authentication_test.go(0 hunks)tf/source_test.go(0 hunks)
💤 Files with no reviewable changes (13)
- internal/cli/sort_test.go
- cli/commands/run/download_source_test.go
- internal/cli/slice_flag_test.go
- internal/cli/generic_flag_test.go
- internal/cli/command_test.go
- tf/getproviders/lock_test.go
- tf/cliconfig/config_test.go
- internal/cli/map_flag_test.go
- tf/getproviders/package_authentication_test.go
- test/integration_graph_test.go
- internal/cli/bool_flag_test.go
- tf/source_test.go
- test/integration_list_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cli/commands/backend/delete/delete.go
- docs/_docs/04_reference/05-strict-mode.md
- internal/remotestate/backend/s3/remote_state_config_test.go
- internal/remotestate/backend/s3/errors.go
- awshelper/config.go
🧰 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/strict/controls/controls.goawshelper/config_test.gocli/commands/run/run.gointernal/remotestate/backend/gcs/client.go
🧠 Learnings (1)
internal/remotestate/backend/gcs/client.go (2)
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4070
File: internal/remotestate/backend/gcs/client.go:274-284
Timestamp: 2025-03-24T14:42:18.517Z
Learning: The syntax `for retries := range maxRetriesWaitingForGcsBucket` is valid in the Terragrunt codebase. While not standard Go, the pattern `for x := range someInt` is an accepted idiom in this project.
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4070
File: internal/remotestate/backend/s3/client.go:609-620
Timestamp: 2025-03-24T14:48:52.988Z
Learning: The syntax `for retries := range maxRetriesWaitingForS3Bucket` is valid in the Terragrunt codebase. While not standard Go, the pattern `for x := range someInt` is an accepted idiom in this project, used in both GCS and S3 client implementations.
🧬 Code Definitions (3)
internal/strict/controls/controls.go (2)
internal/strict/controls/control.go (1)
Control(15-45)internal/strict/control.go (1)
Control(22-55)
cli/commands/run/run.go (6)
internal/remotestate/backend/s3/errors.go (6)
err(19-21)err(25-27)err(33-35)err(42-44)err(51-53)err(60-62)internal/remotestate/backend/gcs/client.go (1)
opts(42-42)internal/remotestate/backend/s3/client.go (1)
cfg(1349-1349)internal/remotestate/backend/gcs/backend.go (1)
BackendName(13-13)internal/remotestate/backend/s3/backend.go (1)
BackendName(14-14)internal/strict/controls/controls.go (1)
RequireExplicitBootstrap(45-45)
internal/remotestate/backend/gcs/client.go (3)
internal/remotestate/backend/gcs/remote_state_config.go (2)
ExtendedRemoteStateConfigGCS(24-32)RemoteStateConfigGCS(50-60)internal/remotestate/backend/gcs/config.go (2)
gcsConfig(55-55)Config(16-16)internal/remotestate/backend/gcs/errors.go (3)
err(13-15)MissingRequiredGCSRemoteStateConfig(5-5)MaxRetriesWaitingForGCSBucketExceeded(11-11)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: unessential
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (25)
internal/strict/controls/controls.go (4)
43-45: Well-documented constant definition.The new constant
RequireExplicitBootstrapis clearly documented and follows the naming convention used throughout the file.
66-72: Control definition appears complete and accurate.The control implementation includes all required fields with clear description, appropriate error message, and warning message. The control is properly categorized under
stageCategory, which is consistent with other similar controls in the file.
96-96: Control correctly added as a subcontrol.Adding
requireExplicitBootstrapControlas a subcontrol toDeprecatedConfigsmakes sense as it relates to a configuration behavior that will be deprecated in future versions.
100-100: Control correctly added to the main controls list.The control is properly added to the main
controlslist, ensuring it will be included in the strict mode controls returned by theNewfunction.awshelper/config_test.go (2)
41-46: Improved test structure for session validation.The refactoring of this test improves the flow by properly creating an AWS session first and then validating it, which better reflects the real-world usage pattern.
51-96: Well-structured table-driven tests for S3 public access block validation.The new test function is well-implemented using the table-driven testing pattern, which makes it easy to test multiple scenarios for public access block validation. The test cases cover important scenarios: nil response, legacy bucket configuration, and explicit false settings for public access blocks.
However, consider changing variable naming for consistency:
-for _, testCase := range testCases { +for _, tc := range testCases { t.Run(testCase.name, func(t *testing.T) { t.Parallel() - response, err := awshelper.ValidatePublicAccessBlock(testCase.response) + response, err := awshelper.ValidatePublicAccessBlock(tc.response) require.NoError(t, err) assert.False(t, response) }) }cli/commands/run/run.go (7)
230-230: Function call updated to match renamed exported function.The call to
GenerateConfighas been updated to match the new function signature with reordered parameters.
266-291: Function visibility and parameter order improved.The function has been exported (renamed from
generateConfigtoGenerateConfig) with clearer parameter names that follow consistent naming conventions (optsandcfg). This improves the API design and makes the function available for use in other packages.
517-517: Updated call to include context parameter.The function call to
remoteStateNeedsInithas been updated to include the context parameter, which follows good practices for context propagation.
523-523: Context propagation added to remote state initialization.The call to
RemoteState.Initnow passes the context parameter, which is important for proper cancellation and timeout handling.
560-590: Consistent parameter naming in functions.The function parameters have been renamed from
terragruntOptionstooptsto maintain consistent naming conventions throughout the codebase.
723-746: Enhanced remote state initialization with explicit bootstrap checks.This significant update to
remoteStateNeedsInitadds several important improvements:
- Context propagation for better cancellation and timeout handling
- Updated parameter naming for consistency
- Integration with the logging context
- Implementation of strict controls for explicit bootstrap requirement
The new code properly checks the
BackendBootstrapflag and evaluates theRequireExplicitBootstrapcontrol, providing better flexibility in how backend resources are initialized.
729-745: Improved null handling and early returns pattern.The updated control flow with early returns improves readability and avoids unnecessary nesting. The explicit handling of the nil remote state case at the beginning of the function follows best practices.
internal/remotestate/backend/gcs/client.go (12)
1-33: File initialization and imports look good
The package declaration, imports, and constant definitions are appropriate. The usage ofcloud.google.com/go/storageand other dependencies aligns with the intended GCS functionality.
34-39: Struct design is clear
TheClientstruct composition is straightforward, embedding the remote state config and the storage client. Logging is also injected cleanly. No issues spotted here.
167-183: Versioning check method looks solid
No issues spotted. The warning for non-versioned buckets is clear and beneficial for users.
185-200: Bucket creation with versioning is well-structured
The code first creates the bucket, waits for it to exist, then adds labels. This order is logical.
202-226: Label assignment is straightforward
LeveragingBucketAttrsToUpdate.SetLabelis a concise approach. No concerns here.
228-265: Conditional enabling of versioning
Your handling ofSkipBucketVersioningis aligned with user intentions. The logic forEnableBucketPolicyOnlyalso reads cleanly.
267-287: Retry loop syntax is approved in this codebase
We see thefor retries := range maxRetriesWaitingForGcsBucketpattern. As per [retrieved_learnings], this idiom is valid in Terragrunt. No changes needed.
289-309: Bucket existence check is acceptable
Although it checks attributes and then attempts to list objects, the comment clarifies its origin and known trade-offs. This is reasonable given the current APIs.
311-326: Deletion logic with retries
This matches the existingutil.DoWithRetryapproach elsewhere in Terragrunt. Clear and consistent.
328-340: Deleting GCS buckets
After deleting the bucket, the code waits until it’s fully removed. This approach prevents race conditions with subsequent operations.
342-357: Second retry loop for waiting on deletion
Similarly to bucket creation, thefor retries := range maxRetriesWaitingForGcsBucketusage is an accepted pattern in this project. Good job maintaining consistency.
359-370: Deletion of objects via a retry pattern
Methodically reattempting object deletions is robust for transient GCP errors. No concerns.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
test/integration_gcp_test.go (2)
127-127: Fix typo in function and variable names.There's a typo in the function name and its usage: "Buket" should be "Bucket".
-func doesGCSBuketObjectExist(t *testing.T, bucketName, prefix string) bool { +func doesGCSBucketObjectExist(t *testing.T, bucketName, prefix string) bool {And update the function calls:
-assert.True(t, doesGCSBuketObjectExist(t, gcsBucketName, objectName), "GCS bucket object %s must exist", objectName) +assert.True(t, doesGCSBucketObjectExist(t, gcsBucketName, objectName), "GCS bucket object %s must exist", objectName) -assert.False(t, doesGCSBuketObjectExist(t, gcsBucketName, objectName), "GCS bucket object %s must not exist", objectName) +assert.False(t, doesGCSBucketObjectExist(t, gcsBucketName, objectName), "GCS bucket object %s must not exist", objectName)Also applies to: 134-134, 295-295
308-309: Remember to close the client in other functions too.You've correctly deferred closing the client in
doesGCSBuketObjectExist, but this pattern isn't consistently applied in other functions that create GCS clients (likevalidateGCSBucketExistsAndIsLabeled,gcsObjectAttrs, etc.).Consider adding
defer gcsClient.Close()to all functions that create a GCS client to prevent potential resource leaks, especially in tests that might create many clients.test/integration_aws_test.go (3)
1456-1481: Fix typo in function name.There's a typo in the function name - it should be "Bucket" instead of "Buket".
-func doesS3BuketKeyExist(t *testing.T, awsRegion string, bucketName, key string) bool { +func doesS3BucketKeyExist(t *testing.T, awsRegion string, bucketName, key string) bool {Also, make sure to update all call sites to this function.
153-154: Update function name in function call.Update the function call to match the corrected function name.
-assert.True(t, doesS3BuketKeyExist(t, helpers.TerraformRemoteStateS3Region, s3BucketName, key), "S3 bucket key %s must exist", key) +assert.True(t, doesS3BucketKeyExist(t, helpers.TerraformRemoteStateS3Region, s3BucketName, key), "S3 bucket key %s must exist", key)
163-164: Update function name in function call.Update the function call to match the corrected function name.
-assert.False(t, doesS3BuketKeyExist(t, helpers.TerraformRemoteStateS3Region, s3BucketName, key), "S3 bucket key %s must not exist", key) +assert.False(t, doesS3BucketKeyExist(t, helpers.TerraformRemoteStateS3Region, s3BucketName, key), "S3 bucket key %s must not exist", key)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/integration_aws_test.go(16 hunks)test/integration_gcp_test.go(8 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.
test/integration_gcp_test.gotest/integration_aws_test.go
🧬 Code Definitions (2)
test/integration_gcp_test.go (4)
test/helpers/package.go (2)
CleanupTerraformFolder(718-725)CopyEnvironment(82-95)util/file.go (4)
err(103-103)err(663-665)err(672-674)JoinPath(472-474)pkg/log/log.go (2)
Error(42-44)Default(12-14)internal/remotestate/backend/gcs/backend.go (4)
bucketName(46-46)bucketName(88-88)bucketName(134-134)bucketName(153-153)
test/integration_aws_test.go (4)
internal/remotestate/backend/s3/backend.go (5)
_(16-16)bucketName(52-52)bucketName(79-79)bucketName(142-142)bucketName(171-171)internal/remotestate/backend/s3/client.go (17)
SidEnforcedTLSPolicy(27-27)client(111-153)client(155-253)client(256-298)client(309-405)client(408-424)client(427-503)client(505-526)client(528-554)client(556-584)client(604-623)client(626-637)client(661-741)client(744-782)client(788-801)client(803-811)client(813-853)internal/remotestate/backend/s3/config.go (1)
DefaultS3BucketAccessLoggingTargetPrefix(25-25)internal/remotestate/backend/s3/client_test.go (2)
description(130-130)CreateS3ClientForTest(27-43)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (14)
test/integration_gcp_test.go (6)
17-18: LGTM: New imports for improved package structure.The imports reflect a transition from using the older remote state implementation to a newer, more modular approach with dedicated backend packages. This aligns with the PR objectives of refactoring the remote state code.
35-35: LGTM: New test fixture constant.The new constant follows the existing naming convention pattern and correctly references the fixture path for bootstrap GCS backend tests.
99-136: Comprehensive test for backend delete functionality.The test properly validates that the
backend deletecommand correctly removes remote state objects. It confirms objects exist after apply and are gone after deletion, which is essential for verifying the correctness of this new command.
271-293: LGTM: Well-refactored bucket validation using new backend structure.The function has been properly updated to use the new
gcsbackendpackage instead of the older remote state implementation. This improves code organization and maintainability.
425-427: Improved error handling in cleanup functions.Changing from failing the test to logging errors during bucket cleanup is a good approach. This prevents cleanup errors from masking actual test failures, which is important for test reliability.
Also applies to: 431-433
38-97:✅ Verification successful
Well-structured test for GCS backend bootstrapping.
This test function effectively validates the three critical scenarios for GCS backend bootstrapping:
- Without bootstrap flag (expected to fail)
- With bootstrap flag
- Using the new backend bootstrap command
The table-driven test approach is a good practice for testing multiple scenarios with different inputs and expected outputs.
Note that this test uses experimental flags like
--experiment cli-redesignand--strict-control require-explicit-bootstrap. Ensure that these flags are intentional and documented in the release notes, especially if they'll become required in future versions.
🏁 Script executed:
#!/bin/bash # Check if the experimental flags are documented in code or documentation rg -A 3 "cli-redesign|require-explicit-bootstrap" --type mdLength of output: 52268
Experimental Flags Verified – Code Approved
The test function is well-structured and effectively covers the three key scenarios for GCS backend bootstrapping. I confirmed that the experimental flags (
--experiment cli-redesignand--strict-control require-explicit-bootstrap) are properly documented in our reference documentation (e.g., in the migration guides and CLI options docs). No further changes are needed.
- Test scenarios (absence of bootstrap flag, usage with flag, and using the backend bootstrap command) are clearly implemented using a table-driven approach.
- Experimental flags are intentional and documented in the release notes.
test/integration_aws_test.go (8)
17-18: Good addition of the AWS error type import.Adding the
awserrpackage import is necessary for the improved error handling in the bucket key existence check function.
34-36: Appropriate imports for the new functionality.The addition of the errors package and s3backend package is appropriate for the refactoring of remote state code mentioned in the PR.
51-51: Good addition of test fixture for bootstrap testing.This constant appropriately defines the path to the test fixtures for the new bootstrap functionality.
56-120: Well-structured test for bootstrap backend functionality.The test function is well designed using table-driven tests to evaluate three key scenarios:
- Applying without bootstrap flag (should fail)
- Applying with bootstrap flag (should succeed)
- Using the new backend bootstrap command (should succeed)
The test includes proper resource cleanup, clear assertions, and thorough validation of created resources.
122-166: Comprehensive test for backend delete functionality.This test thoroughly validates the delete backend functionality by:
- Setting up the environment with bootstrap
- Creating and verifying state files exist
- Executing the delete command
- Verifying the state files are properly removed
The test includes proper resource cleanup and validation.
347-347: Updated constant reference from s3backend package.Using
s3backend.SidEnforcedTLSPolicyinstead of the previous constant location is consistent with the remote state code refactoring described in the PR.
395-395: Using constant instead of hardcoded value.Good practice to use the constant
s3backend.DefaultS3BucketAccessLoggingTargetPrefixinstead of a hardcoded string.
1413-1437: Well-implemented helper for DynamoDB table item existence check.This helper function properly checks if an item exists in a DynamoDB table and includes appropriate error handling.
| deleteS3Bucket(t, helpers.TerraformRemoteStateS3Region, s3BucketName) | ||
| cleanupTableForTest(t, dynamoDBName, helpers.TerraformRemoteStateS3Region) |
There was a problem hiding this comment.
Update parameter order of function call or function declaration.
There's an inconsistency between how deleteS3Bucket is called here versus its declaration. Either update this call or change the function declaration.
If keeping the current function declaration:
-deleteS3Bucket(t, helpers.TerraformRemoteStateS3Region, s3BucketName)
+deleteS3Bucket(t, s3BucketName, helpers.TerraformRemoteStateS3Region)📝 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.
| deleteS3Bucket(t, helpers.TerraformRemoteStateS3Region, s3BucketName) | |
| cleanupTableForTest(t, dynamoDBName, helpers.TerraformRemoteStateS3Region) | |
| deleteS3Bucket(t, s3BucketName, helpers.TerraformRemoteStateS3Region) | |
| cleanupTableForTest(t, dynamoDBName, helpers.TerraformRemoteStateS3Region) |
| deleteS3Bucket(t, s3BucketName, helpers.TerraformRemoteStateS3Region) | ||
| cleanupTableForTest(t, dynamoDBName, helpers.TerraformRemoteStateS3Region) |
There was a problem hiding this comment.
Update parameter order to match function declaration.
Update the parameter order when calling deleteS3Bucket to match the function declaration.
-deleteS3Bucket(t, s3BucketName, helpers.TerraformRemoteStateS3Region)
+deleteS3Bucket(t, helpers.TerraformRemoteStateS3Region, s3BucketName)📝 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.
| deleteS3Bucket(t, s3BucketName, helpers.TerraformRemoteStateS3Region) | |
| cleanupTableForTest(t, dynamoDBName, helpers.TerraformRemoteStateS3Region) | |
| deleteS3Bucket(t, helpers.TerraformRemoteStateS3Region, s3BucketName) | |
| cleanupTableForTest(t, dynamoDBName, helpers.TerraformRemoteStateS3Region) |
| func deleteS3Bucket(t *testing.T, bucketName string, awsRegion string) { | ||
| helpers.DeleteS3Bucket(t, awsRegion, bucketName) | ||
| } |
There was a problem hiding this comment.
Fix parameter order inconsistency.
The parameters in the deleteS3Bucket function declaration don't match how they're used when calling helpers.DeleteS3Bucket. This could lead to bugs.
-func deleteS3Bucket(t *testing.T, bucketName string, awsRegion string) {
+func deleteS3Bucket(t *testing.T, awsRegion string, bucketName string) {
helpers.DeleteS3Bucket(t, awsRegion, bucketName)
}📝 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.
| func deleteS3Bucket(t *testing.T, bucketName string, awsRegion string) { | |
| helpers.DeleteS3Bucket(t, awsRegion, bucketName) | |
| } | |
| func deleteS3Bucket(t *testing.T, awsRegion string, bucketName string) { | |
| helpers.DeleteS3Bucket(t, awsRegion, bucketName) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
test/integration_aws_test.go (3)
57-120:⚠️ Potential issueFlag inconsistent parameter usage in
deleteS3Bucket.
Within this new test, you call:deleteS3Bucket(t, s3BucketName, helpers.TerraformRemoteStateS3Region)but the function signature is:
func deleteS3Bucket(t *testing.T, bucketName string, awsRegion string) { helpers.DeleteS3Bucket(t, awsRegion, bucketName) }Note that the wrapper inverts the order of
bucketNameandawsRegionwhen calling the helper function. This causes the second parameter to be treated as the region in the signature, but you pass it as the bucket name in the internal call. This mismatch may lead to incorrect behavior if the region and bucket name are not recognized as valid in each other’s position.Please align the parameter order across the call site and the function body. For example:
-func deleteS3Bucket(t *testing.T, bucketName string, awsRegion string) { - helpers.DeleteS3Bucket(t, awsRegion, bucketName) +func deleteS3Bucket(t *testing.T, bucketName string, awsRegion string) { + helpers.DeleteS3Bucket(t, bucketName, awsRegion) }
122-166:⚠️ Potential issueSame param mismatch in
TestAwsDeleteBackend.
Here at line 135:deleteS3Bucket(t, helpers.TerraformRemoteStateS3Region, s3BucketName)again reverses the signature parameters. Adjust them to match the fixed function definition to avoid confusion.
1525-1526:⚠️ Potential issueDuplicate param order issue in
deleteS3Bucketwrapper.
Here, the wrapper signature is(t *testing.T, bucketName string, awsRegion string), but calls:helpers.DeleteS3Bucket(t, awsRegion, bucketName)Again, that inverts
bucketNameandawsRegion. Use the same fix suggested above to match the call sites:-func deleteS3Bucket(t *testing.T, bucketName string, awsRegion string) { - helpers.DeleteS3Bucket(t, awsRegion, bucketName) +func deleteS3Bucket(t *testing.T, bucketName string, awsRegion string) { + helpers.DeleteS3Bucket(t, bucketName, awsRegion) }
🧹 Nitpick comments (4)
internal/remotestate/backend/gcs/client.go (4)
40-114: Suggestion to add test coverage for authentication logic.
TheNewClientfunction supports multiple authentication flows (credentials file, access token, environment variables, service account impersonation). Consider adding unit tests or integration tests to ensure each path is exercised and validated, particularly for error handling when credentials are missing or malformed.
219-256: Use the newer Uniform Bucket-Level Access if available.
SettingbucketAttrs.BucketPolicyOnly = storage.BucketPolicyOnly{Enabled: true}is now recommended viaUniformBucketLevelAccessin modern GCS libraries, asBucketPolicyOnlyis deprecated. Consider updating if the current library version supports it.
265-276: Optional: Combine repetitive wait logic with a helper function.
BothWaitUntilGCSBucketExistsandWaitUntilGCSBucketDeletedshare similar retry logic to confirm the bucket’s state. As a good-to-have refactor, you could unify these into one helper that accepts a condition or callback, reducing code duplication.
363-390: Potential performance concerns when deleting many objects.
The current approach deletes objects sequentially inDeleteGCSObjects. For very large buckets, this might be slow. You could consider batching or parallel deletions if the underlying library supports it—though that often adds complexity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/remotestate/backend/gcs/client.go(1 hunks)test/integration_aws_test.go(16 hunks)test/integration_gcp_test.go(8 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.
test/integration_gcp_test.gotest/integration_aws_test.gointernal/remotestate/backend/gcs/client.go
🧠 Learnings (1)
internal/remotestate/backend/gcs/client.go (2)
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4070
File: internal/remotestate/backend/gcs/client.go:274-284
Timestamp: 2025-03-24T14:42:18.517Z
Learning: The syntax `for retries := range maxRetriesWaitingForGcsBucket` is valid in the Terragrunt codebase. While not standard Go, the pattern `for x := range someInt` is an accepted idiom in this project.
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4070
File: internal/remotestate/backend/s3/client.go:609-620
Timestamp: 2025-03-24T14:48:52.988Z
Learning: The syntax `for retries := range maxRetriesWaitingForS3Bucket` is valid in the Terragrunt codebase. While not standard Go, the pattern `for x := range someInt` is an accepted idiom in this project, used in both GCS and S3 client implementations.
🧬 Code Definitions (3)
test/integration_gcp_test.go (4)
test/helpers/package.go (2)
CleanupTerraformFolder(718-725)CopyEnvironment(82-95)pkg/log/log.go (2)
Error(42-44)Default(12-14)internal/remotestate/backend/gcs/backend.go (6)
_(15-15)bucketName(46-46)bucketName(88-88)bucketName(134-134)bucketName(153-153)prefix(135-135)internal/remotestate/backend/gcs/client.go (2)
NewClient(41-114)Client(34-38)
test/integration_aws_test.go (2)
internal/remotestate/backend/s3/client.go (5)
SidEnforcedTLSPolicy(27-27)client(111-153)client(155-253)client(256-298)client(309-405)internal/remotestate/backend/s3/config.go (1)
DefaultS3BucketAccessLoggingTargetPrefix(25-25)
internal/remotestate/backend/gcs/client.go (3)
internal/remotestate/backend/gcs/remote_state_config.go (2)
ExtendedRemoteStateConfigGCS(24-32)RemoteStateConfigGCS(50-60)internal/remotestate/backend/gcs/config.go (2)
gcsConfig(55-55)Config(16-16)internal/remotestate/backend/gcs/errors.go (3)
err(13-15)MissingRequiredGCSRemoteStateConfig(5-5)MaxRetriesWaitingForGCSBucketExceeded(11-11)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (27)
test/integration_gcp_test.go (7)
17-18: Imports and constants look goodThe updates to import the new
gcsbackendpackage and thelogpackage align with the refactoring of the remote state code mentioned in the PR objectives. The new constant for the bootstrap test fixture path is also appropriate.Also applies to: 35-35
38-97: Well-structured and comprehensive bootstrap test implementationThe
TestGcpBootstrapBackendfunction is well-implemented using table-driven tests to cover various scenarios:
- Verifying an error occurs when no bootstrap flag is provided
- Testing bootstrap with the
--backend-bootstrapflag- Testing bootstrap with the explicit
backend bootstrapcommandGood practices observed:
- Proper cleanup with
CleanupTerraformFolder- Appropriate defer pattern for bucket deletion
- Clear test case naming and validation functions
- Parallel test execution for better performance
99-136: Comprehensive delete backend test implementationThis test effectively validates the backend delete functionality:
- Creates the bucket with bootstrap
- Creates state objects in the bucket
- Verifies the objects exist
- Runs the delete command
- Verifies the objects no longer exist
The test properly checks both the positive case (objects exist after creation) and negative case (objects don't exist after deletion).
295-326: Clean implementation of object existence checkThe new
doesGCSBucketObjectExisthelper function is well-implemented:
- Properly marked as a helper with
t.Helper()- Good error handling
- Follows the pattern of other helper functions in the file
- Correctly closes the client with defer
271-292: Good refactoring of GCS client creationThe changes to
validateGCSBucketExistsAndIsLabeledproperly integrate with the newgcsbackendpackage:
- Uses the new structured configuration
- Properly creates a client with the new
NewClientfunction- Passes the logger correctly
- Uses the client's method to check bucket existence
425-427: Improved error handling in deleteGCSBucketThe change from
t.Fatalftot.Logfwith a return is a good improvement as it allows the test to continue running when bucket deletion fails, rather than causing an immediate failure. This is especially important for cleanup operations that shouldn't prevent the test from completing.Also applies to: 431-433
334-342: Consistent refactoring across all GCS helper functionsAll GCS helper functions have been consistently updated to use the new
gcsbackendpackage and client creation pattern:
- Consistent configuration structure
- Proper error handling
- Consistent use of the logger
This comprehensive refactoring ensures that all GCS operations use the same underlying client creation logic.
Also applies to: 379-382, 404-407
internal/remotestate/backend/gcs/client.go (2)
24-32: Constants look good.
No issues found in the definition and usage of these constants. They provide clear intervals for retry logic.
302-317: Verification requested: Confirm user flow when deleting GCS buckets.
DeleteGCSBucketIfNecessarydoes not prompt for confirmation before permanently deleting a bucket, which may be intentional. If the CLI flow already handles confirmation elsewhere (e.g., a hidden-bucketflag with yes/no prompt), ignore this comment. Otherwise, confirm that users are aware they’re deleting everything in the bucket.Would you like me to search your CLI command layer (via shell script) to confirm that deletion is always prompted?
test/integration_aws_test.go (18)
18-18: Confirm usage ofawserrimport.
This import is introduced to capture AWS-specific error codes and messages. The addition looks appropriate for handling AWS errors more granularly.
34-36: New imports are consistent.
The imports forinternal/errorsands3backendare coherent with the added logic in this file. Continue ensuring that each imported package serves a clear purpose to avoid bloat.
395-395: Validation of default logging prefix.
Asserting theDefaultS3BucketAccessLoggingTargetPrefixensures that the default prefix is set properly. This check looks good and helps verify correct remote state bucket logging configuration.
903-903: New call tohelpers.RunTerragruntCommand.
This line properly captures output and errors for the test scenario. The usage of standard buffers is consistent with the rest of the test suite.
909-909: Repeated Terragrunt command check.
Running the command again for idempotency is a good approach to verify that no additionalinitor other steps are needed once the backend is properly configured.
923-925: Backing up existing AWS access variables.
Storing the original credentials in local vars and clearing them is a standard practice to ensure the test uses only the OIDC approach. Looks fine.
949-954: Restoring environment and deleting the S3 bucket with role & token.
The defer block properly restores credentials and cleans up the test bucket, including the IAM role and token requirements. This helps maintain test isolation.
1055-1055: Creation of temporary config for S3 codegen test.
Usinghelpers.CreateTmpTerragruntConfigkeeps these tests self-contained and ensures the environment is consistent.
1177-1179: Creating AWS session inTestAwsAssumeRole.
Wrapping session creation and verifying no error ensures the test fails fast if credentials or AWS session setup is invalid.
1183-1183: Validating presence of expected strings in file content.
Assertingrole_arn = "..."+identityARNis a straightforward check to confirm the patched Terraform code references the assumed identity.
1286-1288: Creating AWS session to read config.
The new session creation steps are consistent with your typical pattern. Good to see error handling withrequire.NoError.
1290-1290: Retrieving AWS identity.
Fetching the caller identity withawshelper.GetAWSIdentityArnensures the test can confirm the correct role is in effect. Good usage.
1392-1395: Verifying DynamoDB table existence.
These lines confirm the table is described without error. This is essential for ensuring Terragrunt’s remote state locking.
1397-1399: Retrieving table tags.
Fetching and verifying tags helps ensure that Terragrunt’s tagging logic is properly exercised.
1469-1479: Handling S3 NotFound errors cleanly.
Usingerrors.Asto identifyawserr.Erroris a good approach. Checking for theNotFoundcode handles typical missing key scenarios in S3. This ensures the test can accurately confirm whether an object exists or not.
1513-1513: Doc comment forcreateS3Bucket.
Adding a comment describing the function is helpful for clarity.
1517-1517: Creating an S3 client increateS3Bucket.
This is consistent with your existing approach in other tests.
1521-1523: Ensuring S3 bucket creation.
These lines verify the bucket is successfully created and raise an immediate error if something goes wrong, which is best practice for tests.
| // CreateGCSBucketIfNecessary prompts the user to create the given bucket if it doesn't already exist and if the user | ||
| // confirms, creates the bucket and enables versioning for it. | ||
| func (client *Client) CreateGCSBucketIfNecessary(ctx context.Context, bucketName string, opts *options.TerragruntOptions) error { | ||
| if client.DoesGCSBucketExist(ctx, bucketName) { | ||
| return nil | ||
| } | ||
|
|
||
| // A project must be specified in order for terragrunt to automatically create a storage bucket. | ||
| if client.Project == "" { | ||
| return errors.New(MissingRequiredGCSRemoteStateConfig("project")) | ||
| } | ||
|
|
||
| // A location must be specified in order for terragrunt to automatically create a storage bucket. | ||
| if client.Location == "" { | ||
| return errors.New(MissingRequiredGCSRemoteStateConfig("location")) | ||
| } | ||
|
|
||
| client.logger.Debugf("Remote state GCS bucket %s does not exist. Attempting to create it", bucketName) | ||
|
|
||
| if opts.FailIfBucketCreationRequired { | ||
| return backend.BucketCreationNotAllowed(bucketName) | ||
| } | ||
|
|
||
| prompt := fmt.Sprintf("Remote state GCS bucket %s does not exist or you don't have permissions to access it. Would you like Terragrunt to create it?", bucketName) | ||
|
|
||
| shouldCreateBucket, err := shell.PromptUserForYesNo(ctx, prompt, opts) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if shouldCreateBucket { | ||
| // To avoid any eventual consistency issues with creating a GCS bucket we use a retry loop. | ||
| description := "Create GCS bucket " + bucketName | ||
|
|
||
| return util.DoWithRetry(ctx, description, gcpMaxRetries, gcpSleepBetweenRetries, client.logger, log.DebugLevel, func(ctx context.Context) error { | ||
| return client.CreateGCSBucketWithVersioning(ctx, bucketName) | ||
| }) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider honoring skip-bucket-creation configuration.
The CreateGCSBucketIfNecessary method currently prompts for bucket creation if it does not exist. However, there is a SkipBucketCreation field available in ExtendedRemoteStateConfigGCS that isn’t referenced here. If the intended behavior is to skip creation outright in certain scenarios, you may want to respect that config field.
Resolved. Thank you. |
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
test/integration_strict_test.go (2)
20-50: 🛠️ Refactor suggestionVariable should be renamed to
tcfor consistencyBased on the past review comments, the team preference is to use
tcfor test case variables. Please renametestCasestotcto maintain consistency across the codebase.-testCases := []struct { +tc := []struct {
85-106: 🛠️ Refactor suggestionVariable should be renamed to
tcfor consistencyBased on the past review comments, the team preference is to use
tcfor test case variables. Please renametestCasestotcto maintain consistency across the codebase.-testCases := []struct { +tc := []struct {
♻️ Duplicate comments (2)
test/integration_aws_test.go (2)
108-109:⚠️ Potential issueUpdate parameter order to match function declaration
The parameter order when calling
deleteS3Bucketdoesn't match the function declaration and other calls to this function.-deleteS3Bucket(t, s3BucketName, helpers.TerraformRemoteStateS3Region) +deleteS3Bucket(t, helpers.TerraformRemoteStateS3Region, s3BucketName)
1525-1527: 🛠️ Refactor suggestionConsider fixing the parameter order inconsistency
The
deleteS3Bucketfunction takes parameters in a different order than how they're sometimes called. This inconsistency exists in calls to this function (see line 108) and should be addressed.Either update the function declaration to match how it's called:
-func deleteS3Bucket(t *testing.T, bucketName string, awsRegion string) { +func deleteS3Bucket(t *testing.T, awsRegion string, bucketName string) {Or update all calls to this function to match its declaration.
🧹 Nitpick comments (2)
util/jsons_test.go (1)
15-15: Consider improving naming consistency.The variable name was changed from
tctotestCaseson line 15 for better readability, but the loop variable on line 23 is still calledtc. For better consistency and readability, consider renaming the loop variable as well.testCases := []struct { value any expected string }{ {"aws_region", "aws_region"}, {[]string{"10.0.0.0/16", "10.0.0.10/16"}, "[\"10.0.0.0/16\",\"10.0.0.10/16\"]"}, } - for i, tc := range testCases { + for i, testCase := range testCases { t.Run(strconv.Itoa(i), func(t *testing.T) { t.Parallel() - actual, err := util.AsTerraformEnvVarJSONValue(tc.value) + actual, err := util.AsTerraformEnvVarJSONValue(testCase.value) require.NoError(t, err) - assert.Equal(t, tc.expected, actual) + assert.Equal(t, testCase.expected, actual)Also applies to: 23-23, 27-29
internal/remotestate/backend/s3/backend_test.go (1)
3-10: Consider adding negative test cases.While the current test coverage is good for happy paths, consider adding test cases for error scenarios such as invalid configurations or edge cases that might cause the function to fail. This would ensure robust behavior under all conditions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (63)
awshelper/config_test.go(2 hunks)cli/app_test.go(9 hunks)cli/commands/aws-provider-patch/aws-provider-patch_test.go(1 hunks)cli/commands/catalog/module/doc_test.go(2 hunks)cli/commands/catalog/module/repo_test.go(3 hunks)cli/commands/run/download_source_test.go(1 hunks)cli/commands/run/run_test.go(4 hunks)cli/commands/validate-inputs/validate-inputs_test.go(1 hunks)cli/flags/flag_test.go(3 hunks)cli/provider_cache_test.go(4 hunks)codegen/generate_test.go(8 hunks)config/config_helpers_test.go(33 hunks)config/config_test.go(13 hunks)config/include_test.go(5 hunks)config/locals_test.go(1 hunks)configstack/module_test.go(1 hunks)internal/cli/args_test.go(1 hunks)internal/cli/bool_flag_test.go(1 hunks)internal/cli/command_test.go(6 hunks)internal/cli/generic_flag_test.go(3 hunks)internal/cli/map_flag_test.go(2 hunks)internal/cli/slice_flag_test.go(3 hunks)internal/cli/sort_test.go(1 hunks)internal/ctyhelper/helper_test.go(3 hunks)internal/experiment/experiment_test.go(1 hunks)internal/hclhelper/wrap_test.go(2 hunks)internal/remotestate/backend/config_test.go(1 hunks)internal/remotestate/backend/gcs/config_test.go(1 hunks)internal/remotestate/backend/s3/backend_test.go(1 hunks)internal/remotestate/backend/s3/config_test.go(1 hunks)internal/remotestate/backend/s3/remote_state_config_test.go(1 hunks)internal/remotestate/remote_encryption_test.go(4 hunks)internal/strict/control_test.go(7 hunks)test/integration_auto_retry_test.go(1 hunks)test/integration_aws_test.go(16 hunks)test/integration_download_test.go(6 hunks)test/integration_find_test.go(3 hunks)test/integration_gcp_test.go(8 hunks)test/integration_graph_test.go(4 hunks)test/integration_include_test.go(2 hunks)test/integration_list_test.go(2 hunks)test/integration_local_dev_test.go(1 hunks)test/integration_serial_test.go(5 hunks)test/integration_strict_test.go(4 hunks)test/integration_test.go(20 hunks)test/integration_units_reading_test.go(3 hunks)test/integration_windows_test.go(2 hunks)tf/cache/models/provider_test.go(2 hunks)tf/cliconfig/config_test.go(1 hunks)tf/getproviders/hash_test.go(2 hunks)tf/getproviders/lock_test.go(1 hunks)tf/getproviders/package_authentication_test.go(5 hunks)tf/getter_test.go(2 hunks)tf/source_test.go(3 hunks)tflint/tflint_test.go(2 hunks)util/collections_test.go(10 hunks)util/datetime_test.go(2 hunks)util/file_test.go(15 hunks)util/jsons_test.go(1 hunks)util/min_test.go(2 hunks)util/prefix-writer_test.go(4 hunks)util/random_test.go(3 hunks)util/reflect_test.go(3 hunks)
✅ Files skipped from review due to trivial changes (17)
- test/integration_download_test.go
- internal/cli/args_test.go
- util/prefix-writer_test.go
- util/min_test.go
- cli/flags/flag_test.go
- config/locals_test.go
- util/reflect_test.go
- cli/commands/catalog/module/doc_test.go
- internal/strict/control_test.go
- cli/commands/run/run_test.go
- codegen/generate_test.go
- cli/provider_cache_test.go
- test/integration_find_test.go
- util/collections_test.go
- test/integration_test.go
- util/file_test.go
- cli/app_test.go
🚧 Files skipped from review as they are similar to previous changes (21)
- test/integration_list_test.go
- cli/commands/run/download_source_test.go
- internal/cli/map_flag_test.go
- tf/cliconfig/config_test.go
- tf/source_test.go
- internal/cli/slice_flag_test.go
- tf/getproviders/lock_test.go
- internal/cli/generic_flag_test.go
- internal/cli/sort_test.go
- tf/getproviders/package_authentication_test.go
- internal/cli/command_test.go
- internal/remotestate/remote_encryption_test.go
- internal/cli/bool_flag_test.go
- internal/ctyhelper/helper_test.go
- config/config_helpers_test.go
- config/config_test.go
- internal/remotestate/backend/gcs/config_test.go
- test/integration_serial_test.go
- internal/hclhelper/wrap_test.go
- config/include_test.go
- internal/remotestate/backend/s3/remote_state_config_test.go
🧰 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.
cli/commands/validate-inputs/validate-inputs_test.gotest/integration_windows_test.goconfigstack/module_test.gotest/integration_local_dev_test.goutil/datetime_test.gocli/commands/catalog/module/repo_test.goutil/random_test.goutil/jsons_test.gointernal/experiment/experiment_test.gotest/integration_graph_test.gocli/commands/aws-provider-patch/aws-provider-patch_test.gotf/cache/models/provider_test.gotest/integration_auto_retry_test.goawshelper/config_test.gotest/integration_units_reading_test.gotf/getproviders/hash_test.gointernal/remotestate/backend/config_test.gointernal/remotestate/backend/s3/config_test.gotest/integration_gcp_test.gotf/getter_test.gotflint/tflint_test.gotest/integration_strict_test.gointernal/remotestate/backend/s3/backend_test.gotest/integration_aws_test.gotest/integration_include_test.go
🧬 Code Definitions (9)
cli/commands/validate-inputs/validate-inputs_test.go (1)
cli/commands/validate-inputs/validate-inputs.go (2)
Run(28-32)GetVarFlagsFromArgList(329-363)
test/integration_windows_test.go (1)
engine/engine.go (1)
Run(78-127)
configstack/module_test.go (3)
configstack/errors.go (1)
DependencyCycleError(48-48)config/errors.go (1)
DependencyCycleError(270-270)internal/errors/export.go (1)
As(7-9)
util/random_test.go (1)
util/random.go (1)
GetRandomTime(17-43)
test/integration_units_reading_test.go (1)
test/helpers/package.go (1)
CopyEnvironment(82-95)
internal/remotestate/backend/config_test.go (6)
internal/remotestate/backend/gcs/config_test.go (1)
TestConfig_IsEqual(11-104)internal/remotestate/backend/s3/config_test.go (1)
TestConfig_IsEqual(11-118)internal/remotestate/backend/config.go (1)
Config(13-13)internal/remotestate/backend/gcs/config.go (1)
Config(16-16)internal/remotestate/backend/s3/config.go (1)
Config(30-30)internal/remotestate/config.go (1)
Config(28-35)
test/integration_gcp_test.go (4)
pkg/log/log.go (2)
Error(42-44)Default(12-14)test/helpers/package.go (4)
CleanupTerraformFolder(718-725)CopyEnvironment(82-95)UniqueID(152-165)RunTerragruntCommandWithOutput(826-830)util/file.go (1)
JoinPath(472-474)internal/remotestate/backend/gcs/client.go (2)
NewClient(41-114)Client(34-38)
test/integration_strict_test.go (1)
test/helpers/package.go (2)
CopyEnvironment(82-95)RunTerragruntCommandWithOutput(826-830)
test/integration_aws_test.go (4)
test/helpers/package.go (4)
TerraformRemoteStateS3Region(56-56)DeleteS3Bucket(202-240)CreateDynamoDBClientForTest(183-199)CreateS3ClientForTest(168-180)internal/remotestate/backend/s3/client.go (1)
SidEnforcedTLSPolicy(27-27)internal/remotestate/backend/s3/config.go (1)
DefaultS3BucketAccessLoggingTargetPrefix(25-25)awshelper/config.go (2)
CreateAwsSession(208-253)GetAWSIdentityArn(398-405)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-and-test
- GitHub Check: unessential
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (47)
util/random_test.go (1)
14-14: Improved variable naming enhances code readability.The renaming of the test cases variable from
tctotestCasesand the consistent use oftcas the loop variable improves code clarity and follows Go conventions for variable naming. This makes the test more maintainable and easier to understand without changing any functionality.Also applies to: 27-27, 36-44
test/integration_include_test.go (2)
37-48: Good variable name improvements!Renaming the variable from a less descriptive name to
testCasesand usingtcas the loop variable improves code clarity while maintaining the same test logic. This change makes the code more readable and follows good naming practices in Go.
211-215: Consistent variable naming improvementThe variable naming changes here match the pattern established in the earlier function, creating consistency throughout the codebase. These changes improve readability without altering the functionality of the tests.
configstack/module_test.go (1)
202-209: Clean variable name refactoring.The variable name change from
testCasetotcin the test loop is a good readability improvement. This follows Go best practices of using shorter variable names for loop iterators, making the code more concise without sacrificing clarity.internal/experiment/experiment_test.go (1)
113-136: Good variable renaming for improved readability.Shortening the loop variable from
testCasetotcthroughout the test function makes the code more concise and easier to read. This is consistent with Go conventions and the changes made in other test files.tf/getproviders/hash_test.go (1)
29-50: Consistent variable naming approach.The change of variable name from
tctotestCasesfor the test slice and usingtcas the loop variable is good. This aligns with Go conventions where container variables have more descriptive names while loop iterators are kept concise.test/integration_windows_test.go (1)
103-121: Improved loop variable naming.Renaming the loop variable from
testCasetotcsimplifies the code and improves readability. This is consistent with the changes made in other test files, creating a uniform coding style across the codebase.test/integration_local_dev_test.go (1)
59-67: Improved variable naming for better readability and consistency.The renaming from
testCasetotcacross the loop makes the code more concise while maintaining clarity. This change aligns with Go testing conventions and creates consistency with other tests in the codebase.util/datetime_test.go (2)
16-16: Good variable renaming for clarity.Changing the variable name from
tcto the more descriptivetestCasesimproves code readability by making the purpose of the variable clearer.
27-38: Consistent loop variable naming.Renaming the inner loop variable from
testCaseto the shortertccreates consistency with Go testing conventions while maintaining readability throughout the test function.cli/commands/catalog/module/repo_test.go (2)
55-82: Improved variable naming for consistency.The renaming from
testCasetotcthroughout the loop body makes the code more concise and consistent with Go testing conventions. This change maintains readability while reducing verbosity.
149-159: Standardized test variable naming.Consistent with the earlier changes, renaming from
testCasetotchere maintains a uniform variable naming convention across all test functions in the file.test/integration_auto_retry_test.go (1)
169-176: Enhanced variable naming clarity.Renaming from
tcto the more descriptivetestCasesfor the slice declaration, and fromtestCasetotcfor the loop variable improves readability while maintaining consistency with Go testing conventions.cli/commands/validate-inputs/validate-inputs_test.go (2)
57-58: LGTM - Updated variable references correctly.All references to the former test case variable have been properly updated to use the new
tcloop variable directly.Also applies to: 61-62
53-54:✅ Verification successful
Verify Go version for safe variable capture in parallel tests.
The change from capturing the loop variable to using it directly is generally fine, but be cautious when running parallel tests. Before Go 1.22, using the loop variable directly in parallel subtests could lead to race conditions where the wrong test case is used.
🏁 Script executed:
#!/bin/bash # Check the Go version used in the project go version # Alternatively, look for version in go.mod grep -E "^go [0-9]+\.[0-9]+" go.modLength of output: 92
Verified Go Version Compatibility
Based on thego.moddeclaration (go 1.23.0) and the installed Go version (go1.24.1), using the loop variable directly in subtests is safe. Since these versions are greater than Go 1.22, the risk of race conditions due to incorrect variable capture in parallel tests is mitigated.No changes are required for this section of the code.
tf/cache/models/provider_test.go (2)
16-17: Improved variable naming for better readability.Renaming
tctotestCasesimproves code readability by making the variable name more descriptive of its purpose - it represents a collection of test cases.
49-58: Ensure consistent variable naming for loop iterations.The renaming of the test cases slice improves readability, and your test correctly handles parallel execution with appropriate variable scoping. Just ensure this pattern is applied consistently across all test files.
tflint/tflint_test.go (2)
14-15: Improved variable naming for better readability.Renaming
tctotestCasesimproves code readability by making the variable name more descriptive of its purpose.
42-49: Simplified loop variable usage is cleaner.The test case usage is cleaner now, directly using
tcas the loop variable and consistently referencing it throughout. This makes the code more readable and easier to follow.tf/getter_test.go (2)
41-42: Improved variable naming for better readability.Renaming
tctotestCasesimproves code readability by better indicating that this variable contains a collection of test cases.
94-101: Consistent use of loop variable throughout test.Good use of the loop variable
tcdirectly in the test, and consistent application of the variable renaming pattern seen in other test files.cli/commands/aws-provider-patch/aws-provider-patch_test.go (1)
316-328: Good change - consistent variable naming improves readabilityThe change from
testCasetotcfor the loop variable is consistent with Terragrunt's coding conventions and aligns with the team's preferences as seen in the past review comments. This improves readability and maintainability of the codebase.test/integration_graph_test.go (2)
49-79: Good change - consistent variable naming improves readabilityThe change from
testCasetotcfor the loop variable is consistent with Terragrunt's coding conventions and aligns with the team's preferences as seen in the past review comments.
111-140: Good change - consistent variable naming improves readabilityThe change from
testCasetotcfor the loop variable is consistent with Terragrunt's coding conventions and aligns with the team's preferences as seen in the past review comments.awshelper/config_test.go (2)
41-48: Good improvement to the test error handlingThe changes to
TestAwsSessionValidationFailadd a proper error check after creating the AWS session, ensuring that the test proceeds only if the session was created successfully. This makes the test more robust by properly separating session creation from validation.
51-96: Well-structured new test for S3 public access block validationThe new test function
TestAwsNegativePublicAccessResponseis well-structured with:
- Multiple test cases covering different scenarios
- Proper test case isolation with parallel execution
- Clear error handling
- Appropriate assertions
This improves test coverage for the S3 public access block functionality, which is important as noted in the referenced issue #2109.
test/integration_gcp_test.go (8)
17-18: Good refactoring of importsThe changes correctly import and utilize the new backend-specific package for GCS instead of the old remote package, which aligns with the refactoring of remote state code.
38-97: Well-structured test for GCS bootstrap backend functionalityThis test function comprehensively validates the bootstrap functionality in different scenarios:
- No bootstrap without flag (expecting error)
- Bootstrap with flag
- Bootstrap with dedicated command
The test includes proper setup, teardown, and assertions to verify the expected behavior.
99-136: Comprehensive test for GCS delete backend functionalityThis test thoroughly validates the delete functionality by:
- Creating remote state objects
- Verifying their existence
- Executing the delete command
- Verifying the objects no longer exist
Good use of helper functions and proper cleanup with deferred operations.
271-293: Updated implementation for GCS bucket validationThe function has been correctly updated to use the new
gcsbackendpackage, which is consistent with the code refactoring objectives of this PR.
295-326: Well-implemented utility function for GCS object existence checkThis new helper function follows the same pattern as other GCS-related functions and properly handles error conditions. Good use of the iterator package for checking object existence.
334-341: Consistent refactoring of gcsObjectAttrs functionThe changes to this function align with the overall refactoring approach, replacing direct client creation with the new backend package method.
379-382: Good refactoring of createGCSBucket functionThe updated implementation correctly uses the new backend package, maintaining consistency with other refactored functions.
404-407: Good refactoring of deleteGCSBucket functionThis function has been updated to use the new backend package, consistent with the other changes in this file.
test/integration_aws_test.go (7)
35-36: Good addition of necessary importsAdding the AWS error handling package and the S3 backend package is appropriate for the new functionality being implemented.
56-120: Well-structured test for AWS bootstrap backend functionalityThis test comprehensively validates the bootstrap functionality in different scenarios, similar to the GCP implementation. The test cases are thorough and verify the expected behavior.
122-166: Comprehensive test for AWS delete backend functionalityThis test thoroughly validates the delete functionality for AWS backends, creating and verifying both S3 objects and DynamoDB table items, then confirming their removal after the delete command.
347-347: Good refactoring to use backend package constantThe code now correctly uses the constant from the S3 backend package for TLS policy SID, which is part of the remote state code refactoring.
395-395: Good refactoring to use backend package constantThe code now correctly uses the constant from the S3 backend package for default access logging target prefix, which is part of the remote state code refactoring.
1413-1437: Well-implemented utility function for DynamoDB table item existence checkThis new helper function is implemented correctly and follows the same pattern as other AWS-related functions. Good error handling and return value.
1456-1481: Well-implemented utility function for S3 bucket key existence checkThis new helper function is well-structured and includes proper error handling, particularly for the case where a key is not found.
internal/remotestate/backend/config_test.go (1)
11-100: Well-structured tests for backend config equalityThis test suite provides comprehensive test cases for the
IsEqualmethod of thebackend.Configtype, covering important scenarios:
- Empty configs comparison
- Identical configs for different backends (S3 and GCS)
- Various field differences (bucket, key, region, etc.)
- Boolean value handling
- Null value handling
The tests are well-organized, parameterized, and run in parallel for efficiency.
internal/remotestate/backend/s3/config_test.go (1)
11-118: Comprehensive tests for S3 backend config equalityThis test suite thoroughly validates the
IsEqualmethod of thes3.Configtype with a wide range of test cases:
- Empty and nil config handling
- Single and multiple key comparisons
- Boolean value handling (including string/bool conversion)
- Ignoring specific fields (S3 tags, DynamoDB tags, access logging options)
- Null value handling
The tests are well-structured, parameterized, and include clear descriptions.
internal/remotestate/backend/s3/backend_test.go (4)
12-16: Well-structured test function with proper parallelization.Good use of
t.Parallel()which allows the test to run concurrently with other tests, improving test execution time. The backend initialization is clean and follows good testing practices.
17-141: Comprehensive test case coverage.The test cases effectively cover various scenarios including empty configurations, valid S3 configurations, Terragrunt-specific keys filtering, lock table conversions, and assume role handling. This thorough approach ensures the
GetTFInitArgsfunction behaves correctly across different input patterns.
143-156: Good use of subtests and conditional assertions.The test correctly uses subtests with
t.Run()and handles different assertion types based on theshouldBeEqualflag. The parallelization of subtests is also implemented correctly, which is a good practice for independent test cases.
126-140:❓ Verification inconclusive
Verify assume_role map serialization behavior.
The test expects the
assume_rolemap to be serialized to a specific string format. Ensure this serialization is deterministic and doesn't depend on map iteration order, which is not guaranteed in Go. Consider if a more structured approach to verifying this transformation would be more reliable.
🏁 Script executed:
#!/bin/bash # Check how the assume_role map is handled in the backend implementation rg -A 10 "assume_role" ./internal/remotestate/backend/s3/Length of output: 4656
Ensure deterministic serialization of the
assume_rolefield.The test expects the serialized output to be:
"{external_id=\"123\",role_arn=\"arn:aws:iam::123:role/role\",session_name=\"qwe\"}"This suggests that the implementation must enforce a strict key ordering independent of Go’s native (non-deterministic) map iteration order. Please verify that the serialization routine sorts the keys—or otherwise guarantees this order—so that the output is consistent. Additionally, consider whether comparing deserialized key/value pairs (instead of a raw string) might yield a more robust test.
| @@ -21,7 +21,7 @@ func TestUnitsReading(t *testing.T) { | |||
|
|
|||
| cleanupTerraformFolder(t, testFixtureUnitsReading) | |||
|
|
|||
| tc := []struct { | |||
| testCases := []struct { | |||
There was a problem hiding this comment.
Variable naming inconsistency causes compilation error
There's a mismatch between the variable declaration and its usage throughout the function. The variable is declared as testCases on line 24, but it's referenced as tc in the for loop (line 139) and all subsequent references (lines 140, 148, 152, 156, 170).
This inconsistency will cause compilation errors as the compiler won't recognize tc as a defined variable.
Either:
- Change the declaration back to
tc:
- testCases := []struct {
+ tc := []struct {- Or update all usages to
testCases(recommended for consistency with the PR's intent to improve variable naming):
- for _, tc := range testCases {
- t.Run(tc.name, func(t *testing.T) {
+ for _, testCase := range testCases {
+ t.Run(testCase.name, func(t *testing.T) {And similarly update all other references to tc in the file.
Also applies to: 139-140, 148-148, 152-152, 156-156, 170-170
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| tmpEnvPath := helpers.CopyEnvironment(t, testFixtureFindParentWithDeprecatedRoot) | ||
| rootPath := util.JoinPath(tmpEnvPath, testFixtureFindParentWithDeprecatedRoot, "app") | ||
|
|
||
| args := "--non-interactive --log-level debug --working-dir " + rootPath | ||
| if tt.strictMode { | ||
| if tc.strictMode { | ||
| args = "--strict-mode " + args | ||
| } | ||
|
|
||
| for _, control := range tt.controls { | ||
| for _, control := range tc.controls { | ||
| args = " --strict-control " + control + " " + args | ||
| } | ||
|
|
||
| _, stderr, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt run --experiment cli-redesign "+args+" -- plan") | ||
|
|
||
| if tt.expectedError != nil { | ||
| if tc.expectedError != nil { | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), tt.expectedError.Error()) | ||
| assert.Contains(t, err.Error(), tc.expectedError.Error()) | ||
| } else { | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| assert.Contains(t, stderr, tt.expectedStderr) | ||
| assert.Contains(t, stderr, tc.expectedStderr) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Loop variable should be updated for consistency
Update the loop variable to use tc instead of testCases to maintain consistency with the rest of the codebase.
-for _, tc := range testCases {
+for _, testCase := range tc {Also update all instances of tc inside the loop to testCase.
Committable suggestion skipped: line range outside the PR's diff.
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| tmpEnvPath := helpers.CopyEnvironment(t, testFixtureEmptyState) | ||
| rootPath := util.JoinPath(tmpEnvPath, testFixtureEmptyState) | ||
|
|
||
| args := "--non-interactive --log-level trace --working-dir " + rootPath | ||
| if tt.strictMode { | ||
| if tc.strictMode { | ||
| args = "--strict-mode " + args | ||
| } | ||
|
|
||
| for _, control := range tt.controls { | ||
| for _, control := range tc.controls { | ||
| args = " --strict-control " + control + " " + args | ||
| } | ||
|
|
||
| _, stderr, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt plan-all "+args) | ||
|
|
||
| if tt.expectedError != nil { | ||
| if tc.expectedError != nil { | ||
| require.Error(t, err) | ||
| require.ErrorContains(t, err, tt.expectedError.Error()) | ||
| require.ErrorContains(t, err, tc.expectedError.Error()) | ||
| } else { | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| assert.Contains(t, stderr, tt.expectedStderr) | ||
| assert.Contains(t, stderr, tc.expectedStderr) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Loop variable should be updated for consistency
Update the loop variable to use tc instead of testCases to maintain consistency with the rest of the codebase.
-for _, tc := range testCases {
+for _, testCase := range tc {Also update all instances of tc inside the loop to testCase.
Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| remoteBackend := s3backend.NewBackend() | ||
|
|
||
| testCases := []struct { //nolint: govet |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Investigate reason for linter suppression.
The //nolint: govet comment is suppressing a Go vet warning. It would be helpful to understand what specific warning is being suppressed and whether it can be addressed directly instead of being ignored.
🏁 Script executed:
#!/bin/bash
# Check what govet warnings might be triggered by this test structure
go vet ./internal/remotestate/backend/s3/backend_test.go 2>&1 || trueLength of output: 5931
Action: Remove unnecessary govet suppression comment
Our verification shows that running go vet on internal/remotestate/backend/s3/backend_test.go produces no warnings. This indicates that the //nolint: govet directive on line 17 is likely no longer needed. Please remove it, or if there’s a justified reason (such as a known false positive), add a clear comment explaining its necessity.
- File: internal/remotestate/backend/s3/backend_test.go
- Line: 17 – Remove
//nolint: govetunless detailed documentation is provided for its continued use.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/integration_aws_test.go (1)
95-95: Fix string formatting syntax in test case nameThe
fmt.Sprintfis used incorrectly here. When passing just the string variable tofmt.Sprintf, it expects a format string, but no arguments are provided.-t.Run(fmt.Sprintf(tc.name), func(t *testing.T) { +t.Run(tc.name, func(t *testing.T) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/integration_aws_test.go(16 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.
test/integration_aws_test.go
🧬 Code Definitions (1)
test/integration_aws_test.go (5)
test/helpers/package.go (3)
TerraformRemoteStateS3Region(56-56)CopyTerragruntConfigAndFillPlaceholders(123-132)CreateS3ClientForTest(168-180)internal/remotestate/backend/s3/client.go (1)
SidEnforcedTLSPolicy(27-27)internal/remotestate/backend/s3/config.go (1)
DefaultS3BucketAccessLoggingTargetPrefix(25-25)awshelper/config.go (1)
CreateAwsSession(208-253)internal/remotestate/backend/s3/client_test.go (1)
CreateS3ClientForTest(27-43)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: unessential
- GitHub Check: build-and-test
🔇 Additional comments (12)
test/integration_aws_test.go (12)
56-120: Well-structured test for the new bootstrap commandThis test function is well-designed using table-driven tests to verify multiple scenarios. It tests both the command-line flag (
--backend-bootstrap) and the new subcommand (backend bootstrap), which is excellent for ensuring complete coverage.
122-166: Well-implemented test for the new delete commandThis test thoroughly verifies the new backend delete functionality by first bootstrapping the backend, creating state files, verifying they exist, then deleting them and confirming they're gone. This provides good coverage for the new command.
135-136: Update parameter order of function call or function declaration.There's an inconsistency between how
deleteS3Bucketis called here versus its declaration. Either update this call or change the function declaration.If keeping the current function declaration:
-deleteS3Bucket(t, helpers.TerraformRemoteStateS3Region, s3BucketName) +deleteS3Bucket(t, s3BucketName, helpers.TerraformRemoteStateS3Region)
108-109: Update parameter order to match function declaration.Update the parameter order when calling
deleteS3Bucketto match the function declaration.-deleteS3Bucket(t, s3BucketName, helpers.TerraformRemoteStateS3Region) +deleteS3Bucket(t, helpers.TerraformRemoteStateS3Region, s3BucketName)
347-347: LGTM! Updated import reference to specific backend packageGood refactoring to use the S3 backend-specific constant instead of the more generic remote package.
395-395: LGTM! Updated import reference to specific backend packageGood refactoring to use the S3 backend-specific constant instead of a hardcoded value.
923-954: LGTM! Improved environment variable handlingGood improvement in handling environment variables by properly capturing and restoring them. This prevents leakage that could affect other tests.
1177-1180: LGTM! Improved AWS session creationGood refactoring to use the dedicated helper functions for AWS session creation and identity retrieval, making the code more consistent.
Also applies to: 1286-1290
1389-1411: LGTM! Improved error handling in validation functionsGood improvement in the validation functions by using
require.NoErrorinstead of assertions and adding proper handling for nil tag maps.Also applies to: 1441-1454
1413-1437: LGTM! Well-implemented existence check functionsThese new utility functions provide a clean way to check for the existence of DynamoDB items and S3 objects, with proper error handling for the "not found" case.
Also applies to: 1456-1481
1525-1527: Fix parameter order inconsistency.The parameters in the
deleteS3Bucketfunction declaration don't match how they're used when callinghelpers.DeleteS3Bucket. This could lead to bugs.-func deleteS3Bucket(t *testing.T, bucketName string, awsRegion string) { +func deleteS3Bucket(t *testing.T, awsRegion string, bucketName string) { helpers.DeleteS3Bucket(t, awsRegion, bucketName) }
1513-1523: LGTM! Improved client creation and error handlingGood refactoring to use the dedicated helper functions for AWS client creation and improved error handling with better error type checking.
Also applies to: 1532-1550, 1556-1557, 1571-1572
Description
backend bootstrapbackend delete(The-bucketflag is hidden and when used, a prompt with a yes/no question is displayed.)TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
New Features
Documentation
--backend-bootstrapflag for backend resource initialization.Tests
ValidatePublicAccessBlockfunctionality and S3 client creation.