Azurerm storage for remote state backend #4487
Conversation
…p fetching output from remote state. Done with Claude 3.5 Sonnet
…runt into azurerm_storage
📝 WalkthroughWalkthroughThis update introduces full support for the Azure Resource Manager (azurerm) backend as an experimental remote state backend in Terragrunt. It implements the backend, configuration parsing, authentication (Azure AD, MSI, Service Principal), error handling, telemetry, dependency injection, and comprehensive unit and integration tests. Documentation is updated to reflect Azure backend support, usage, and troubleshooting. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TerragruntCLI
participant BackendRegistry
participant AzureBackend
participant AzureSDK
participant AzureAD
User->>TerragruntCLI: Run with --experiment azure-backend
TerragruntCLI->>BackendRegistry: RegisterBackends(opts)
BackendRegistry->>AzureBackend: Register if experiment enabled
User->>TerragruntCLI: terragrunt apply (with azurerm backend)
TerragruntCLI->>AzureBackend: Bootstrap/NeedsBootstrap/StateOps
AzureBackend->>AzureSDK: Create/check storage account/container
AzureBackend->>AzureAD: Authenticate (AD/MSI/SP)
AzureBackend->>AzureSDK: Perform blob/state operations
AzureBackend-->>TerragruntCLI: Return status, errors, telemetry
TerragruntCLI-->>User: Output results, logs, errors
Possibly related issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Nitpick comments (30)
internal/remotestate/backend/errors.go (1)
14-26: Consider generalising the type/field names to match the new messageThe error text has been broadened from “S3 bucket” to “Remote state storage”, but the surrounding identifiers still speak in S3 terms (
BucketDoesNotExistError,bucketNamefield).
This mismatch may confuse future contributors now that multiple backends are supported.Diff showing minimal rename (illustrative – would require project-wide updates):
-type BucketDoesNotExistError struct { - bucketName string -} +type StorageDoesNotExistError struct { + name string +}If renaming everywhere is too disruptive today, at least add a short comment explaining that the type is legacy-named for backward compatibility.
docs/_docs/04_reference/02-cli-options.md (1)
2238-2244: Mirror the change indocs-starlight& tighten wording
The migration note in the coding guidelines requires that every change under
docs/be replicated under the new Starlight docs (docs-starlight/**). Please add the same Azure-inclusion sentence to the matching file to keep both doc sets in sync.Minor wording nit: “AWS S3 and Azure Storage backends” is slightly ambiguous. Consider “AWS S3 and Azure Storage (Azure RM) backends” so readers instantly recognise it’s the
azurermremote-state backend.cli/commands/backend/migrate/migrate_test.go (1)
12-42: Good starting point for migration testing.The smoke test properly verifies function callability and follows testing best practices with parallel execution and clear error expectations. Consider expanding test coverage to include more comprehensive migration scenarios when feasible.
azurehelper/azure_blob_service_test.go (1)
307-312: Remove duplicate helper function and standardize naming.There's a duplicate comment and inconsistent naming for string pointer helper functions. The file already has
stringPtrdefined at line 121, but here introducesstrPtr.Either remove this duplicate function and use the existing
stringPtr, or standardize on one name throughout the file:-// Helper function to create string pointers - -// Helper function to create string pointers -func strPtr(s string) *string { - return &s -} +// Use the existing stringPtr function defined earlier in the filedocs-starlight/src/content/docs/02-features/04-state-backend.mdx (1)
188-188: Fix minor grammatical issues in documentation.The static analysis correctly identified these issues:
Apply these fixes:
- - Set up [Azure Monitor alerts](https://learn.microsoft.com/en-us/azure/storage/blobs/monitor-blob-storage) for unauthorized access attempts + - Set up [Azure Monitor alerts](https://learn.microsoft.com/en-us/azure/storage/blobs/monitor-blob-storage) for unauthorized access attempts to the storage - - Set up alerts for unauthorized access attempts + - Set up alerts for unauthorized access attempts to the storage -#### Storage Account Bootstrap Requirements +#### Storage Account Bootstrap Requirements, For storage account bootstrap to work:Also applies to: 412-412, 486-486
internal/remotestate/backend/azurerm/config_test.go (2)
164-166: Remove unnecessary struct field alignment comment.The comment about struct field alignment and memory layout is not helpful in a test context and adds unnecessary complexity. Focus on the test logic instead.
- config *azurerm.ExtendedRemoteStateConfigAzurerm // pointer type (8 bytes) - name string // string (16 bytes) - expectError bool // bool (1 byte) + config *azurerm.ExtendedRemoteStateConfigAzurerm + name string + expectError bool
254-256: Remove premature optimization comments.The comments about struct field ordering for memory alignment are not necessary in test code and make the code harder to read. This is premature optimization in a testing context.
- name string // string (16 bytes) - first for alignment - expected string // string (16 bytes) - group strings together - config azurerm.ExtendedRemoteStateConfigAzurerm // largest field at end + name string + expected string + config azurerm.ExtendedRemoteStateConfigAzurerminternal/remotestate/backend/azurerm/testing/azure_test_utils.go (3)
24-34: Remove unnecessary struct optimization comments.The comments about struct field ordering and padding are not helpful in a test utility context and add unnecessary complexity.
type AzureTestConfig struct { - // Group string fields together StorageAccountName string ContainerName string Location string AccessKey string - // Put bool field at the end UseAzureAD bool - // Add padding to optimize struct size - _ [3]byte // padding to align to 8-byte boundary }
134-134: Define magic number as a constant.The retry count should be defined as a constant for better maintainability and clarity.
Add at the package level:
const ( // AzureStorageContainerMinLength is the minimum length for Azure storage container names (3 characters) AzureStorageContainerMinLength = 3 // AzureStorageContainerMaxLength is the maximum length for Azure storage container names (63 characters) AzureStorageContainerMaxLength = 63 + // DefaultTestRetryCount is the default number of retries for test operations + DefaultTestRetryCount = 3 )Then update the usage:
- maxRetries := 3 + maxRetries := DefaultTestRetryCount
186-186: Use the same retry constant for consistency.Apply the same constant here for consistency with the cleanup function.
- maxRetries := 3 + maxRetries := DefaultTestRetryCountazurehelper/azure_resource_group_test.go (1)
537-549: Remove commented code or convert to TODO items.Commented out code should be removed. If these are planned tests, track them as TODO items or issues instead.
-// In a real test environment with Azure credentials, we could test the actual client -// but for unit tests, we would need to mock the Azure SDK clients. -// Below is an example of how we might structure those tests if we had mocks: - -/* -func TestCreateResourceGroupClient(t *testing.T) { - // This would be implemented with mocks -} - -func TestResourceGroupExists(t *testing.T) { - // This would be implemented with mocks -} - -func TestCreateResourceGroupIfNotExists(t *testing.T) { - // This would be implemented with mocks -} -*/ +// TODO: Add integration tests with mocked Azure SDK clients for: +// - TestCreateResourceGroupClient +// - TestResourceGroupExists +// - TestCreateResourceGroupIfNotExistsazurehelper/azure_resource_group.go (1)
33-53: Optimize duplicate Azure credentials retrieval.The function calls
GetAzureCredentialstwice - once to get the subscription ID and again to get credentials. This is inefficient and could be optimized.func CreateResourceGroupClient(ctx context.Context, l log.Logger, subscriptionID string) (*ResourceGroupClient, error) { + // Get Azure credentials once + cred, envSubscriptionID, err := GetAzureCredentials(ctx, l) + if err != nil { + return nil, fmt.Errorf("error getting azure credentials: %w", err) + } + if subscriptionID == "" { - // Try to get subscription ID from environment variables - _, envSubscriptionID, err := GetAzureCredentials(ctx, l) - if err != nil { - return nil, fmt.Errorf("error getting azure credentials: %w", err) - } - if envSubscriptionID != "" { subscriptionID = envSubscriptionID l.Infof("Using subscription ID from environment: %s", subscriptionID) } else { return nil, errors.Errorf("subscription_id is required either in configuration or as an environment variable") } } - // Get Azure credentials - cred, _, err := GetAzureCredentials(ctx, l) - if err != nil { - return nil, fmt.Errorf("error getting azure credentials: %w", err) - } -internal/remotestate/backend/azurerm/config.go (1)
45-46: Consider removing manual struct padding.Using unnamed padding fields (
_ [6]byteand_ [7]byte) for alignment is fragile and makes the struct harder to maintain. Go's compiler typically handles struct alignment efficiently without manual intervention.- // Add padding to optimize struct size (align to 8 bytes for bool fields) - _ [6]byte // 6 bytes padding to align the following bools to 8-byte boundary - // Group bool fields together at the end (1 byte each) UseMsi bool `mapstructure:"use_msi"` UseAzureADAuth bool `mapstructure:"use_azuread_auth"` // Default is now trueRemoteStateConfigAzurerm RemoteStateConfigAzurerm `mapstructure:",squash"` // large struct // Put smaller fields at the end DisableBlobPublicAccess bool `mapstructure:"disable_blob_public_access"` // 1 byte at end - _ [7]byte // 7 bytes padding to align struct sizeIf performance is critical and you've measured that alignment matters, document the reasoning with benchmarks.
Also applies to: 59-60
azurehelper/azure_storage_account_test.go (1)
1-2791: Consider splitting this large test file into smaller, focused test files.At 2791 lines, this test file is quite large and could benefit from being split into smaller, more focused test files such as:
azure_storage_account_validation_test.gofor validation testsazure_storage_account_sku_test.gofor SKU-related testsazure_storage_account_update_test.gofor update logic testsazure_storage_account_helpers_test.gofor helper function testsThis would improve maintainability and make it easier to locate specific tests.
internal/remotestate/backend/azurerm/telemetry.go (1)
383-391: Consider making the sensitive keys list configurable.The hardcoded list of sensitive keys may need updates as new sensitive attributes are added. Consider:
- Moving this list to a configuration file or constants
- Making it extensible through configuration
- Documenting these sensitive keys in a central location
internal/remotestate/remote_state.go (1)
41-57: Remove explicit padding field - Go compiler handles alignment automatically.The explicit padding field
_ [6]byteon line 56 is unnecessary. Modern Go compilers automatically handle struct field alignment and padding. Removing it will make the struct cleaner and easier to maintain without affecting performance.type RemoteState struct { // Put map first Encryption map[string]any `mapstructure:"encryption"` // Put pointers together Generate *ConfigGenerate `mapstructure:"generate"` backend backend.Backend // Struct after pointers BackendConfig backend.Config `mapstructure:"config"` // String field BackendName string `mapstructure:"backend"` // Bool fields at the end DisableInit bool `mapstructure:"disable_init"` DisableDependencyOptimization bool `mapstructure:"disable_dependency_optimization"` - // Add padding to optimize struct size - _ [6]byte // padding to align to 8-byte boundary }internal/remotestate/backend/azurerm/backend_test.go (4)
91-93: Use the existing createLogger helper function.You've already defined a
createLogger()helper function. Consider using it here for consistency.- // Create a logger for testing - formatter := format.NewFormatter(format.NewKeyValueFormatPlaceholders()) - formatter.SetDisabledColors(true) - l := log.New(log.WithLevel(log.DebugLevel), log.WithFormatter(formatter)) + l := createLogger()
609-609: Remove commented-out code for EnableHierarchicalNS.These commented lines appear to be remnants of a removed feature. Clean them up to maintain code clarity.
- // "EnableHierarchicalNS": false, // removed - // if v, exists := tc.expectedSAConfig["EnableHierarchicalNS"]; exists && !tc.skipExtendedChecks { - // assert.Equal(t, v, azureCfg.StorageAccountConfig.EnableHierarchicalNS) - // }Also applies to: 726-728
866-871: Fix struct field ordering instead of using nolint directive.Rather than suppressing the govet warning, reorder the struct fields for better memory alignment.
- //nolint: govet testCases := []struct { - name string containerName string - expectError bool errorMessage string + name string + expectError bool }{
987-987: Consider fixing struct field ordering throughout instead of using nolint:govet.Multiple test structs use
//nolint: govetto suppress struct field ordering warnings. It's better to fix the root cause by ordering fields from largest to smallest alignment requirement.For example, reorder fields as:
testCases := []struct { config backend.Config // 8 bytes (map pointer) envVars map[string]string // 8 bytes (map pointer) expectedErrorType interface{} // 16 bytes (interface) expectedErrorString string // 16 bytes (string) name string // 16 bytes (string) expectError bool // 1 byte }{Also applies to: 1094-1094, 1201-1201, 1291-1291, 1356-1356, 1433-1433
azurehelper/azure_blob.go (1)
70-86: Consider documenting these methods as deprecated or removing them.These methods always return true/nil and only log warnings. They don't provide actual functionality and might confuse users. Consider either properly implementing versioning checks at the storage account level or clearly marking these as deprecated.
internal/remotestate/backend/azurerm/backend.go (1)
762-764: Remove outdated TODO comment about armstorage package.This comment appears to be outdated since the storage account functionality is working properly.
- // Import the armstorage package conditionally - // We need to add the armstorage package to go.mod - // go get github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/storage/armstorageinternal/remotestate/backend/azurerm/backend_interface_test.go (1)
377-385: Consider more specific error type assertions.The test checks for multiple possible error messages, which makes it fragile and suggests inconsistent error handling. Consider using specific error types instead of string matching.
- assert.True(t, - strings.Contains(err.Error(), "subscription_id") || - strings.Contains(err.Error(), "storage account") || - strings.Contains(err.Error(), "credential") || - strings.Contains(err.Error(), "auth") || - strings.Contains(err.Error(), "no such host"), - "Error should be related to authentication/connectivity, got: %v", err) + // Check for specific error types instead + var authErr *azurerm.AuthenticationError + var connErr *azurerm.ConnectivityError + assert.True(t, + errors.As(err, &authErr) || errors.As(err, &connErr), + "Expected authentication or connectivity error, got: %v", err)internal/remotestate/backend/azurerm/errors.go (1)
193-206: Consider adding more retryable HTTP status codes.The
IsRetryable()method is missing some common transient error codes that Azure services may return.func (err TransientAzureError) IsRetryable() bool { // Common transient HTTP status codes switch err.StatusCode { case http.StatusTooManyRequests, // 429 - Too Many Requests http.StatusInternalServerError, // 500 - Internal Server Error http.StatusBadGateway, // 502 - Bad Gateway http.StatusServiceUnavailable, // 503 - Service Unavailable - http.StatusGatewayTimeout: // 504 - Gateway Timeout + http.StatusGatewayTimeout, // 504 - Gateway Timeout + http.StatusRequestTimeout, // 408 - Request Timeout + http.StatusConflict: // 409 - Conflict (often transient in Azure) return true default: return false } }azurehelper/azure_storage_account.go (2)
440-561: Consider breaking down this large function for better maintainability.The
updateStorageAccountIfNeededfunction is over 120 lines long and handles multiple concerns. Consider extracting the property comparison logic into separate functions.Extract property checking into helper functions:
func (c *StorageAccountClient) checkAccessTierUpdate(account *armstorage.Account, config StorageAccountConfig, updateParams *armstorage.AccountUpdateParameters) bool { if !CompareAccessTier(account.Properties.AccessTier, config.AccessTier) && config.AccessTier != "" { // Update logic here return true } return false } func (c *StorageAccountClient) checkTagsUpdate(account *armstorage.Account, config StorageAccountConfig, updateParams *armstorage.AccountUpdateParameters) bool { if !CompareStringMaps(account.Tags, config.Tags) && len(config.Tags) > 0 { // Update logic here return true } return false }
780-836: Simplify the role assignment retry logic.The retry logic in
createRoleAssignmentWithRetryis complex with nested conditions. Consider extracting the retry logic into a separate function.func (c *StorageAccountClient) createRoleAssignmentWithRetry( ctx context.Context, l log.Logger, storageAccountResourceID, roleAssignmentID string, roleAssignment armauthorization.RoleAssignmentCreateParameters, userObjectID string, isServicePrincipal bool, ) error { _, err := c.roleAssignmentClient.Create(ctx, storageAccountResourceID, roleAssignmentID, roleAssignment, nil) if err == nil { return nil } // Handle specific error cases if handled := c.handleRoleAssignmentError(ctx, l, err, userObjectID, isServicePrincipal); handled { return nil } // Try alternative ID format for InvalidRoleAssignmentId error var respErr *azcore.ResponseError if errors.As(err, &respErr) && respErr.ErrorCode == "InvalidRoleAssignmentId" { return c.retryWithAlternativeID(ctx, l, storageAccountResourceID, roleAssignment, userObjectID) } return err }test/integration_azure_test.go (4)
32-35: Extract more magic values as constants.Many magic values are used throughout the tests. Consider extracting them as constants for better maintainability.
const ( testFixtureAzureBackend = "./fixtures/azure-backend" testFixtureAzureOutputFromRemoteState = "./fixtures/azure-output-from-remote-state" + defaultAzureLocation = "westeurope" + containerNameMaxLength = 63 + storageAccountNameMaxLength = 24 + defaultTestTimeout = 2 * time.Minute + retryMaxAttempts = 5 + retryDelay = 5 * time.Second )
387-500: Simplify TestAzureRMBootstrapBackend test structure.This test has complex nested logic in the
checkExpectedResultFn. Consider breaking it into separate, focused test functions.Instead of one test with complex logic, create separate tests:
func TestAzureRMBootstrapBackend_Create(t *testing.T) { // Test container creation } func TestAzureRMBootstrapBackend_Delete(t *testing.T) { // Test container deletion } func TestAzureRMBootstrapBackend_DeleteNonExistent(t *testing.T) { // Test deleting non-existent container }
1514-1515: Replace sleep with polling for more reliable tests.Using
time.Sleepcan make tests flaky. Consider polling with a timeout instead.- // Wait for storage account deletion to complete before deleting resource group - time.Sleep(10 * time.Second) + // Poll until storage account is deleted + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + for { + exists, _ := cleanupStorageClient.StorageAccountExists(ctx) + if !exists { + break + } + select { + case <-ctx.Done(): + t.Logf("Timeout waiting for storage account deletion") + break + case <-time.After(1 * time.Second): + // Continue polling + } + }Similar improvement needed at line 1554.
1960-2237: Consider organizing test helpers into a separate file.The test file has grown quite large with many helper functions. Consider moving the test helpers to a separate file like
azure_test_helpers.gofor better organization.Move these helper functions to a new file:
azureTestContexttype and methodssetupAzureTestcreateTestStorageAccountsetupTestContainertestBlobOperationscreateTerraformModulegenerateAzureTerragruntConfig- Other generic helpers
This will make the main test file more focused on the actual test cases.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (57)
azurehelper/azure_auth_test.go(1 hunks)azurehelper/azure_blob.go(1 hunks)azurehelper/azure_blob_service_test.go(1 hunks)azurehelper/azure_blob_test.go(1 hunks)azurehelper/azure_constants.go(1 hunks)azurehelper/azure_resource_group.go(1 hunks)azurehelper/azure_resource_group_test.go(1 hunks)azurehelper/azure_storage_account.go(1 hunks)azurehelper/azure_storage_account_test.go(1 hunks)azurehelper/test_helpers.go(1 hunks)azurehelper/utility_functions_test.go(1 hunks)cli/commands/backend/migrate/migrate.go(4 hunks)cli/commands/backend/migrate/migrate_test.go(1 hunks)cli/flags/global/flags.go(1 hunks)config/config.go(1 hunks)config/config_as_cty.go(1 hunks)config/config_test.go(1 hunks)config/dependency.go(4 hunks)config/dependency_test.go(2 hunks)docs-starlight/src/content/docs/02-features/04-state-backend.mdx(5 hunks)docs-starlight/src/content/docs/03-community/01-contributing.mdx(1 hunks)docs-starlight/src/content/docs/04-reference/04-experiments.md(2 hunks)docs-starlight/src/data/flags/dependency-fetch-output-from-state.mdx(1 hunks)docs/_docs/01_getting-started/04-terminology.md(1 hunks)docs/_docs/04_reference/02-cli-options.md(1 hunks)docs/_docs/05_troubleshooting/03-performance.md(2 hunks)go.mod(1 hunks)internal/experiment/experiment.go(2 hunks)internal/remotestate/backend/azurerm/backend.go(1 hunks)internal/remotestate/backend/azurerm/backend_interface_test.go(1 hunks)internal/remotestate/backend/azurerm/backend_test.go(1 hunks)internal/remotestate/backend/azurerm/config.go(1 hunks)internal/remotestate/backend/azurerm/config_test.go(1 hunks)internal/remotestate/backend/azurerm/errors.go(1 hunks)internal/remotestate/backend/azurerm/errors_test.go(1 hunks)internal/remotestate/backend/azurerm/retry.go(1 hunks)internal/remotestate/backend/azurerm/retry_test.go(1 hunks)internal/remotestate/backend/azurerm/telemetry.go(1 hunks)internal/remotestate/backend/azurerm/telemetry_test.go(1 hunks)internal/remotestate/backend/azurerm/testing/azure_test_utils.go(1 hunks)internal/remotestate/backend/backend.go(1 hunks)internal/remotestate/backend/common.go(1 hunks)internal/remotestate/backend/config_test.go(3 hunks)internal/remotestate/backend/errors.go(1 hunks)internal/remotestate/remote_state.go(4 hunks)internal/remotestate/remote_state_test.go(3 hunks)options/options.go(4 hunks)test/fixtures/azure-backend/terragrunt.hcl(1 hunks)test/fixtures/azure-output-from-remote-state/env1/app1/main.tf(1 hunks)test/fixtures/azure-output-from-remote-state/env1/app1/terragrunt.hcl(1 hunks)test/fixtures/azure-output-from-remote-state/env1/app2/main.tf(1 hunks)test/fixtures/azure-output-from-remote-state/env1/app2/terragrunt.hcl(1 hunks)test/fixtures/azure-output-from-remote-state/env1/app3/main.tf(1 hunks)test/fixtures/azure-output-from-remote-state/env1/app3/terragrunt.hcl(1 hunks)test/fixtures/azure-output-from-remote-state/root.hcl(1 hunks)test/helpers/package.go(1 hunks)test/integration_azure_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`docs/**/*.md`: Review the documentation for clarity, grammar, and spelling. Mak...
docs/**/*.md: Review the documentation for clarity, grammar, and spelling. Make sure that the documentation is easy to understand and follow. There is currently a migration underway from the Jekyll based documentation indocsto the Starlight + Astro based documentation indocs-starlight. Whenever changes are made to thedocsdirectory, ensure that an equivalent change is made in thedocs-starlightdirectory to keep thedocs-starlightdocumentation accurate.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs/_docs/05_troubleshooting/03-performance.mddocs/_docs/01_getting-started/04-terminology.mddocs/_docs/04_reference/02-cli-options.md
`docs-starlight/**/*.md*`: Review the documentation for clarity, grammar, and sp...
docs-starlight/**/*.md*: Review the documentation for clarity, grammar, and spelling. Make sure that the documentation is easy to understand and follow. There is currently a migration underway from the Jekyll based documentation indocsto the Starlight + Astro based documentation indocs-starlight. Make sure that thedocs-starlightdocumentation is accurate and up-to-date with thedocsdocumentation, and that any difference between them results in an improvement in thedocs-starlightdocumentation.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs-starlight/src/data/flags/dependency-fetch-output-from-state.mdxdocs-starlight/src/content/docs/03-community/01-contributing.mdxdocs-starlight/src/content/docs/04-reference/04-experiments.mddocs-starlight/src/content/docs/02-features/04-state-backend.mdx
`**/*.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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
internal/remotestate/backend/common.gointernal/remotestate/backend/errors.goazurehelper/test_helpers.gointernal/experiment/experiment.gocli/commands/backend/migrate/migrate.gointernal/remotestate/backend/backend.gocli/flags/global/flags.goconfig/config_test.goazurehelper/azure_constants.gocli/commands/backend/migrate/migrate_test.goconfig/config_as_cty.gointernal/remotestate/backend/config_test.goazurehelper/azure_blob_test.goconfig/dependency.goconfig/dependency_test.gointernal/remotestate/backend/azurerm/retry.goconfig/config.goazurehelper/azure_blob_service_test.gointernal/remotestate/backend/azurerm/config_test.goazurehelper/azure_resource_group_test.gointernal/remotestate/backend/azurerm/testing/azure_test_utils.goazurehelper/azure_resource_group.gooptions/options.gointernal/remotestate/backend/azurerm/telemetry_test.gointernal/remotestate/backend/azurerm/retry_test.gointernal/remotestate/backend/azurerm/errors_test.gointernal/remotestate/backend/azurerm/config.goazurehelper/azure_auth_test.gointernal/remotestate/backend/azurerm/telemetry.goazurehelper/utility_functions_test.gotest/helpers/package.goazurehelper/azure_storage_account_test.gointernal/remotestate/backend/azurerm/backend_test.gointernal/remotestate/remote_state_test.goazurehelper/azure_blob.gointernal/remotestate/remote_state.goazurehelper/azure_storage_account.gointernal/remotestate/backend/azurerm/errors.gointernal/remotestate/backend/azurerm/backend.gointernal/remotestate/backend/azurerm/backend_interface_test.gotest/integration_azure_test.go
🧠 Learnings (23)
docs/_docs/05_troubleshooting/03-performance.md (1)
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
test/fixtures/azure-output-from-remote-state/env1/app3/terragrunt.hcl (1)
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#3868
File: docs-starlight/patches/@astrojs%[email protected]:33-33
Timestamp: 2025-02-10T23:20:04.295Z
Learning: In Terragrunt projects, all `.hcl` files can be assumed to be Terragrunt configurations by default, with specific exceptions like `.terraform.lock.hcl` that need explicit handling.
cli/commands/backend/migrate/migrate.go (3)
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
Learnt from: partcyborg
PR: gruntwork-io/terragrunt#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.
test/fixtures/azure-output-from-remote-state/root.hcl (1)
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#3868
File: docs-starlight/patches/@astrojs%[email protected]:33-33
Timestamp: 2025-02-10T23:20:04.295Z
Learning: In Terragrunt projects, all `.hcl` files can be assumed to be Terragrunt configurations by default, with specific exceptions like `.terraform.lock.hcl` that need explicit handling.
config/config_test.go (1)
Learnt from: partcyborg
PR: gruntwork-io/terragrunt#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.
test/fixtures/azure-output-from-remote-state/env1/app2/terragrunt.hcl (1)
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#3868
File: docs-starlight/patches/@astrojs%[email protected]:33-33
Timestamp: 2025-02-10T23:20:04.295Z
Learning: In Terragrunt projects, all `.hcl` files can be assumed to be Terragrunt configurations by default, with specific exceptions like `.terraform.lock.hcl` that need explicit handling.
test/fixtures/azure-output-from-remote-state/env1/app1/terragrunt.hcl (1)
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#3868
File: docs-starlight/patches/@astrojs%[email protected]:33-33
Timestamp: 2025-02-10T23:20:04.295Z
Learning: In Terragrunt projects, all `.hcl` files can be assumed to be Terragrunt configurations by default, with specific exceptions like `.terraform.lock.hcl` that need explicit handling.
cli/commands/backend/migrate/migrate_test.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
test/fixtures/azure-backend/terragrunt.hcl (1)
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#3868
File: docs-starlight/patches/@astrojs%[email protected]:33-33
Timestamp: 2025-02-10T23:20:04.295Z
Learning: In Terragrunt projects, all `.hcl` files can be assumed to be Terragrunt configurations by default, with specific exceptions like `.terraform.lock.hcl` that need explicit handling.
azurehelper/azure_blob_test.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
config/dependency.go (2)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
config/dependency_test.go (3)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
Learnt from: partcyborg
PR: gruntwork-io/terragrunt#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.
internal/remotestate/backend/azurerm/retry.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
azurehelper/azure_blob_service_test.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
internal/remotestate/backend/azurerm/testing/azure_test_utils.go (2)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
options/options.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
internal/remotestate/backend/azurerm/retry_test.go (2)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
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.
internal/remotestate/backend/azurerm/errors_test.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
azurehelper/azure_auth_test.go (1)
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
internal/remotestate/backend/azurerm/telemetry.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
azurehelper/utility_functions_test.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
test/helpers/package.go (2)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
internal/remotestate/backend/azurerm/errors.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
🧬 Code Graph Analysis (12)
internal/remotestate/backend/backend.go (2)
internal/remotestate/config.go (1)
Config(28-35)options/options.go (1)
TerragruntOptions(120-346)
cli/flags/global/flags.go (1)
options/options.go (1)
RunHooks(48-52)
config/config_test.go (1)
internal/remotestate/remote_state.go (1)
RemoteState(42-57)
internal/remotestate/backend/config_test.go (2)
internal/remotestate/backend/azurerm/config.go (1)
Config(64-64)pkg/log/log.go (1)
Default(12-14)
config/dependency_test.go (4)
test/helpers/logger/logger.go (1)
CreateLogger(9-14)options/options.go (2)
NewTerragruntOptionsForTest(497-515)TerragruntOptions(120-346)internal/remotestate/remote_state.go (2)
New(60-79)RemoteState(42-57)azurehelper/azure_blob.go (1)
CreateBlobServiceClient(89-205)
internal/remotestate/backend/azurerm/retry.go (3)
pkg/log/log.go (3)
Infof(82-84)Debugf(72-74)Warnf(92-94)internal/remotestate/backend/azurerm/errors.go (3)
WrapMaxRetriesExceededError(333-344)TransientAzureError(177-181)WrapTransientAzureError(320-330)internal/errors/export.go (1)
As(7-9)
config/config.go (1)
internal/remotestate/remote_state.go (1)
RemoteState(42-57)
azurehelper/azure_resource_group_test.go (1)
azurehelper/azure_resource_group.go (1)
ResourceGroupConfig(22-30)
azurehelper/azure_resource_group.go (3)
azurehelper/azure_storage_account.go (1)
GetAzureCredentials(854-922)pkg/log/log.go (1)
Infof(82-84)internal/errors/export.go (1)
As(7-9)
options/options.go (1)
config/config.go (1)
Hook(834-842)
internal/remotestate/backend/azurerm/telemetry_test.go (2)
internal/remotestate/backend/azurerm/telemetry.go (21)
ErrorClassification(31-31)ErrorClassAuthentication(34-34)ErrorClassConfiguration(35-35)ErrorClassStorage(36-36)ErrorClassResourceNotFound(42-42)ErrorClassContainer(37-37)ErrorClassValidation(41-41)ErrorClassTransient(40-40)ErrorClassPermissions(39-39)ErrorClassQuotaLimits(43-43)ErrorClassUserInput(44-44)ClassifyError(82-152)MaskSubscriptionID(329-335)NewAzureTelemetryCollector(23-28)OperationBootstrap(51-51)AzureErrorMetrics(63-79)OperationDelete(53-53)OperationValidation(58-58)IsSensitiveAttribute(382-396)FormatLogMessage(338-363)FormatSuccessMessage(366-379)cli/commands/run/run.go (1)
IsRetryable(530-536)
internal/remotestate/backend/azurerm/errors.go (3)
internal/errors/export.go (1)
Unwrap(24-26)azurehelper/azure_blob.go (1)
ContainerCreationError(373-376)internal/remotestate/remote_state.go (1)
New(60-79)
🪛 LanguageTool
docs/_docs/05_troubleshooting/03-performance.md
[style] ~58-~58: To make your writing concise, consider an alternative.
Context: ... - Gotchas The first thing you need to be aware of when considering usage of the `--depend...
(AWARE_OF)
docs-starlight/src/content/docs/04-reference/04-experiments.md
[uncategorized] ~183-~183: The preposition ‘to’ seems more likely in this position.
Context: ...les the Azure Storage backend (azurerm) for Terraform remote state. When this exper...
(AI_HYDRA_LEO_REPLACE_FOR_TO)
docs-starlight/src/content/docs/02-features/04-state-backend.mdx
[grammar] ~188-~188: Probably a preposition is missing after ‘attempts’.
Context: ...r-blob-storage) for unauthorized access attempts - Consider using [Azure Private Endpoints](https:/...
(ATD_VERBS_TO_COLLOCATION)
[grammar] ~412-~412: Probably a preposition is missing after ‘attempts’.
Context: ...- Set up alerts for unauthorized access attempts - Configure Activity Log alerts for critical operat...
(ATD_VERBS_TO_COLLOCATION)
[uncategorized] ~486-~486: Possible missing comma found.
Context: ...unt Bootstrap Requirements For storage account bootstrap to work: 1. You must provide...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (65)
test/fixtures/azure-output-from-remote-state/env1/app3/main.tf (1)
1-3: Output block looks goodStatic output is straightforward and adequate for the fixture.
No issues spotted. 👍docs-starlight/src/data/flags/dependency-fetch-output-from-state.mdx (1)
16-16: Documentation accurately reflects expanded backend support.The update correctly includes Azure Storage alongside S3 as supported backends for direct state output fetching. This aligns with the Azure backend functionality being introduced in this PR.
test/fixtures/azure-output-from-remote-state/env1/app3/terragrunt.hcl (1)
1-7: Test fixture configuration follows established patterns.The configuration correctly uses the standard Terragrunt pattern with
includeblock referencing the root configuration andterraformblock with local source. This is appropriate for test fixtures.docs/_docs/01_getting-started/04-terminology.md (1)
214-214: Documentation accurately updated to reflect expanded backend support.The update correctly includes Azure Storage alongside AWS S3 as supported backends for the experimental dependency output fetching feature. This aligns with the Azure backend functionality being introduced.
azurehelper/test_helpers.go (1)
4-9: Clean and well-documented helper function.The
StringPtrfunction is a common and useful pattern for working with APIs that require string pointers. The implementation is simple, safe, and well-documented.internal/remotestate/backend/common.go (1)
72-77: Method implementation follows established pattern.The
DeleteStorageAccountmethod follows the same pattern as other placeholder methods inCommonBackend, providing a consistent no-op implementation with appropriate warning logging for backends that don't support this functionality.docs/_docs/05_troubleshooting/03-performance.md (1)
48-48: Documentation accurately reflects Azure backend support.The updates correctly expand the documentation to include Azure Storage backends alongside AWS S3, maintaining consistency and clarity. The changes appropriately inform users about the expanded backend compatibility for the
--dependency-fetch-output-from-stateflag.Also applies to: 58-58
go.mod (1)
85-90: Azure SDK dependencies properly added as direct requirements.The explicit addition of Azure SDK packages as direct dependencies is appropriate for the new Azure backend implementation. The selected packages cover the necessary Azure services (storage, identity, authorization, resources) with reasonable version constraints.
test/fixtures/azure-output-from-remote-state/env1/app2/main.tf (1)
1-5: Test fixture correctly implements dependency testing structure.The Terraform configuration properly defines a variable input and formatted output for testing dependency chains with Azure remote state backends. The implementation is clean and serves its intended purpose in the integration test suite.
internal/remotestate/backend/backend.go (1)
46-47: Interface extension follows established patterns.The
DeleteStorageAccountmethod addition is well-designed and consistent with the existing interface methods. The signature correctly follows the established pattern with appropriate parameters and error handling.internal/experiment/experiment.go (1)
30-31: Experiment flag properly implemented following established patterns.The
AzureBackendexperiment is correctly defined with appropriate naming, documentation, and default configuration. The implementation follows the established pattern for experimental features and properly gates the Azure backend functionality.Also applies to: 73-75
docs-starlight/src/content/docs/03-community/01-contributing.mdx (1)
293-325: Excellent documentation for Azure integration tests.The new section provides comprehensive guidance for contributors who want to run Azure integration tests. The documentation includes all necessary prerequisites, environment setup, and clear instructions. The warning about not using production storage accounts is particularly important for security.
test/fixtures/azure-output-from-remote-state/env1/app2/terragrunt.hcl (1)
1-19: Well-structured test fixture for Azure remote state output testing.This configuration correctly defines the dependency relationships and mock outputs needed to test output retrieval from Azure remote state backends. The structure follows Terragrunt best practices.
test/fixtures/azure-backend/terragrunt.hcl (1)
1-14: Appropriate test fixture for Azure backend configuration.The configuration correctly demonstrates Azure backend usage with proper placeholders, Azure AD authentication, and backend file generation. This fixture will effectively support Azure backend integration testing.
test/fixtures/azure-output-from-remote-state/env1/app1/main.tf (1)
1-6: Simple and effective Terraform test configuration.This configuration appropriately defines the input variables and output needed for testing output dependency resolution across modules using Azure remote state. The structure is clean and purpose-built for integration testing.
config/config_test.go (1)
1860-1862: Correct test updates reflecting RemoteState struct refactoring.The test assertions have been properly updated to access
DisableInit,DisableDependencyOptimization, andBackendConfigdirectly onRemoteStateinstead of through a nestedConfigfield. This aligns with the flattenedRemoteStatestruct definition shown in the relevant code snippets.cli/flags/global/flags.go (1)
263-266: Excellent solution for experimental backend timing issue.The hook-based approach elegantly solves the timing problem where experimental backends need re-registration after CLI flags are processed. The clear comments explain the rationale well.
azurehelper/azure_constants.go (2)
9-31: Well-structured constants with good practices.The constant organization is clean, follows Go naming conventions, and appropriately uses standard library constants for HTTP status codes. The default values for timeout and region are reasonable.
22-25: Azure Role Assignment API version and Storage Blob Data Owner role ID are up-to-dateAs of July 1, 2025, the Azure Role Assignments REST API’s latest stable version remains
2022-04-01, and the Storage Blob Data Owner role definition IDb7e6dc6d-f1e8-4753-8033-0f276bb0955bis still valid. No changes are needed.test/fixtures/azure-output-from-remote-state/root.hcl (1)
1-14: Well-structured test fixture for Azure remote state.The configuration properly demonstrates Azure backend usage with secure defaults (Azure AD authentication) and appropriate placeholder values for testing scenarios. The dynamic key using
path_relative_to_include()follows Terragrunt best practices.cli/commands/backend/migrate/migrate.go (2)
50-58: Proper experiment flag propagation for experimental backends.The experiment flag propagation and backend re-registration correctly addresses the timing issue with experimental backends like Azure. The debug logging will help troubleshoot any experiment-related issues during migration.
88-96: Enhanced migration feedback and error handling.The explicit error handling and success messaging provide better user experience during backend migration operations. The logging and console output give clear feedback on migration status.
test/fixtures/azure-output-from-remote-state/env1/app1/terragrunt.hcl (1)
1-29: LGTM! Well-structured test fixture configuration.The Terragrunt configuration follows best practices with proper include block usage, dependency declarations with mock outputs, and clean input mapping. This provides a good foundation for testing Azure remote state dependency resolution.
config/config_as_cty.go (1)
602-636: LGTM! Correct adaptation to flattened RemoteState structure.The function has been properly updated to access fields directly on the
RemoteStatestruct instead of through a nestedConfigfield. This change is consistent with the broader refactoring to flatten theRemoteStatestructure and aligns with the relevant code snippets showing the new struct layout.config/config.go (1)
363-373: LGTM! Consistent updates to RemoteState field access.The
WriteTomethod has been correctly updated to accessDisableInit,DisableDependencyOptimization, andBackendConfigdirectly on theRemoteStatestruct. These changes are consistent with the flattening of theRemoteStatestructure seen throughout the codebase and maintain the same functionality.docs-starlight/src/content/docs/04-reference/04-experiments.md (1)
177-207: LGTM! Well-structured documentation for the Azure backend experiment.The documentation clearly explains the purpose, configuration options, feedback mechanism, and stabilization criteria for the new Azure backend experiment. The structure follows the existing pattern for other experiments.
options/options.go (3)
34-52: Well-designed hook mechanism for options initialization.The hook registration system provides a clean way for packages to register functionality that needs to run when TerragruntOptions are created. This is particularly useful for conditional feature registration like the Azure backend experiment.
456-458: Correct integration of hooks in the constructor.The hooks are properly executed after all options are initialized, ensuring that registered hooks have access to the fully constructed options object.
841-841: Good use of named constant for file permissions.Using
0o644octal notation improves readability over the numeric literal.internal/remotestate/backend/config_test.go (2)
36-138: Comprehensive test coverage for Azure backend configurations.The test cases properly validate equality checks for various Azure backend configuration scenarios, including storage account names, container names, keys, environments, and connection strings. The tests follow the existing pattern and structure.
150-256: Well-structured test function for Azure backend comparison scenarios.The
TestConfig_AzureBackendComparisonfunction thoroughly tests authentication method changes, credential changes, endpoint changes, and subscription/tenant changes. Good use of table-driven tests with descriptive names.azurehelper/azure_blob_test.go (1)
1-179: Well-structured Azure blob error handling and validation tests.The tests provide good coverage for error conversion, error message formatting, and input validation. The use of table-driven tests with descriptive names makes the test cases easy to understand. Good adherence to Go testing best practices with proper use of
t.Parallel().Also applies to: 182-194
config/dependency_test.go (1)
164-257: Comprehensive test for Azure direct state access.The test properly handles Azure environment setup, creates mock state data, and validates the direct state access functionality. Good practices include:
- Proper environment variable checking with test skipping
- Unique container naming to avoid conflicts
- Proper cleanup with defer
- Use of
t.Context()for test context- Thorough validation of the retrieved state data
config/dependency.go (1)
965-979: LGTM! Azure backend integration follows established patterns.The implementation correctly follows the same pattern as the S3 backend for direct state output fetching.
internal/remotestate/backend/azurerm/retry.go (1)
1-241: Excellent retry implementation with comprehensive error handling!The retry logic is well-designed with:
- Proper exponential backoff and jitter to avoid thundering herd
- Comprehensive classification of retryable Azure errors
- Context-aware implementation with cancellation support
- Clear logging at appropriate levels
internal/remotestate/backend/azurerm/errors_test.go (1)
1-582: Comprehensive error type testing with excellent coverage!The tests thoroughly validate all custom error types with proper coverage of:
- Error message formatting
- Error wrapping and unwrapping
- Compatibility with
errors.Asanderrors.Is- Edge cases including nil handling
- Multi-level error chaining
docs-starlight/src/content/docs/02-features/04-state-backend.mdx (1)
288-513: Excellent Azure backend documentation!The documentation provides comprehensive coverage of:
- Authentication methods with clear security recommendations
- Storage account bootstrap process with detailed configuration options
- Best practices for security, monitoring, and organization
- Troubleshooting guide with common error messages
- Clear migration path from deprecated authentication methods
internal/remotestate/backend/azurerm/config_test.go (1)
155-157: Fix syntax error in test closure.There's a misplaced comma and closing parenthesis that will cause a compilation error.
- }, - ) + })Likely an incorrect or invalid review comment.
azurehelper/azure_auth_test.go (1)
1-416: LGTM! Comprehensive Azure authentication testing.This test file provides excellent coverage of Azure authentication scenarios including:
- Environment variable precedence (AZURE_ vs ARM_ prefixes)
- Credential resolution logic
- Safe logging of sensitive credentials
- Multiple authentication methods
The tests are well-structured with proper use of table-driven tests, parallel execution, and test helpers.
internal/remotestate/backend/azurerm/retry_test.go (1)
1-417: LGTM! Excellent retry logic test coverage.This test file provides comprehensive coverage of the retry mechanism including:
- Success and failure scenarios with proper call counting
- Timing verification for delays and backoff
- Context cancellation handling
- Error classification for retryable vs non-retryable errors
- Jitter behavior testing
- Error wrapping and unwrapping
The tests are well-structured with appropriate assertions and timing checks.
test/helpers/package.go (2)
125-201: LGTM! Well-implemented test helper functions.The new helper functions provide excellent flexibility for configuration file templating:
CopyAndFillMapPlaceholders- Generic placeholder replacement using a mapCopyTerragruntConfigAndFillProviderPlaceholders- Provider-specific templating with optional location supportThe functions are well-documented with clear examples, follow established error handling patterns using
require.NoError, and will be valuable for Azure backend testing.
203-216: LGTM! Good refactoring to use the new helper.The
CopyAndReplaceMapPlaceholdersfunction properly delegates to the error handling pattern used throughout the test helpers, maintaining consistency in the codebase.azurehelper/azure_resource_group_test.go (3)
14-122: LGTM! Well-structured validation tests.The table-driven test approach with parallel execution is appropriate for testing the validation logic of ResourceGroupConfig.
277-280: LGTM! Proper UUID validation.The helper function correctly validates the UUID format for Azure subscription IDs.
282-348: LGTM! Comprehensive tag handling tests.Good coverage of different tag scenarios including nil, empty, and special characters.
internal/remotestate/backend/azurerm/telemetry_test.go (1)
1-469: LGTM! Comprehensive and well-structured telemetry tests.Excellent test coverage for telemetry functionality including:
- Error classification with all edge cases
- Security-conscious subscription ID masking
- Nil safety for telemetry collector
- Proper testing of sensitive attribute detection
- Comprehensive field coverage for metrics struct
The table-driven approach with parallel execution is well implemented throughout.
azurehelper/azure_resource_group.go (2)
67-166: LGTM! Well-implemented resource group management methods.The implementation properly handles:
- Resource group creation with existence checking
- Default tag injection when no tags are provided
- Proper error handling for 404 responses
- Comprehensive logging for debugging
Good use of Azure SDK patterns and error handling.
179-193: LGTM! Proper validation implementation.The validation method correctly checks all required fields with clear error messages.
internal/remotestate/backend/azurerm/config.go (1)
82-223: LGTM! Well-implemented configuration parsing and validation.Excellent implementation with:
- Proper default values for security (Azure AD auth by default, blob public access disabled)
- Comprehensive validation of authentication methods preventing conflicting configurations
- Clear error messages for missing or incomplete configurations
- Appropriate list of Terragrunt-only configuration keys
The validation logic properly handles all authentication scenarios and edge cases.
azurehelper/utility_functions_test.go (1)
1-865: LGTM! Excellent utility function test coverage.This is an exemplary test file with:
- Comprehensive table-driven tests for all utility functions
- Excellent edge case coverage (nil values, empty strings, special characters, Unicode)
- Proper parallel test execution throughout
- Good validation of error conditions and error interface implementation
- Security-conscious testing (UUID uniqueness, proper error messages)
The tests properly validate the utility functions that support the Azure backend implementation.
azurehelper/azure_storage_account_test.go (2)
20-2791: Excellent comprehensive test coverage!The test suite provides thorough coverage of Azure Storage Account functionality including:
- Configuration validation
- SKU generation and validation
- Advanced features and encryption settings
- Network rules and URL parsing
- Edge cases, concurrency, and memory usage
- Error handling and data integrity
The table-driven test approach and clear test naming make the tests easy to understand and maintain.
216-315: No existing name‐validation helper function – inline validation is appropriate
The azurehelper package does not expose anyValidateStorageAccountName(or similarly named) function. TheStorageAccountConfig.Validate()method only checks for required fields and does not enforce naming rules. Therefore, this test must reimplement the storage account name rules inline as there is no helper to call.Likely an incorrect or invalid review comment.
internal/remotestate/backend/azurerm/telemetry.go (2)
1-397: Well-designed telemetry implementation for Azure backend operations.The telemetry module provides excellent observability with:
- Structured error classification and metrics collection
- Appropriate log levels based on error types
- Privacy protection through ID masking
- Clean separation between logging and telemetry collection
Great work on implementing comprehensive monitoring capabilities!
81-152: Switch to Azure SDK’s typed errors for robust classificationInstead of fragile string matching on
err.Error(), leverage the Azure SDK’s error types (for example,*azcore.ResponseError) viaerrors.Asto inspectStatusCodeandErrorCode. This will be more reliable across SDK upgrades and localized messages.• File: internal/remotestate/backend/azurerm/telemetry.go
Function:ClassifyErrorSuggestions:
- Import the core error type:
import "github.com/Azure/azure-sdk-for-go/sdk/azcore"- In
ClassifyError, first do:var respErr *azcore.ResponseError if errors.As(err, &respErr) { switch respErr.StatusCode { case 401, 403: return ErrorClassAuthentication case 404: return ErrorClassResourceNotFound // ...and so on, or use respErr.ErrorCode for finer granularity } }- Fall back to your existing string-based cases only if the error isn’t a
ResponseError.Please verify that the Azure SDK version you’re using (e.g. azcore v1.18.0) exports
ResponseErroror other typed error structs suitable for classification.internal/remotestate/remote_state_test.go (1)
204-307: Excellent test coverage for Azure backend argument filtering.The tests properly verify that:
- Bootstrap-related options are filtered from terraform init args
- Resource group name is excluded when bootstrap is enabled
- Only standard terraform backend options are passed through
This ensures correct separation between Terragrunt-specific and Terraform-standard options.
internal/remotestate/remote_state.go (2)
33-125: Excellent implementation of experiment gating for Azure backend.The implementation provides:
- Clear, actionable error messages when the experiment is not enabled
- Graceful fallback behavior with the
experimentalAzureBackendplaceholder- Clean separation between stable and experimental backends
- Proper integration with the options hook system
This is a great pattern for introducing experimental features safely.
270-276: Good addition of UpdateBackend method for dynamic backend updates.This method allows RemoteState instances to update their backend when experiment flags change, providing flexibility in backend selection without recreating the entire RemoteState object.
internal/remotestate/backend/azurerm/backend_test.go (1)
42-46: Good practice: struct field optimization for memory alignment.The struct field ordering with alignment comments demonstrates attention to memory efficiency.
azurehelper/azure_blob.go (1)
88-205: Excellent error handling and connectivity validation.The comprehensive error handling with specific error messages for different scenarios (404, 403, 401, 5xx) and the connectivity test provide great user experience and debugging capabilities.
internal/remotestate/backend/azurerm/backend.go (1)
83-304: Excellent Bootstrap implementation with comprehensive error handling.The method properly validates configuration, uses mutex for thread safety, implements retry logic, and provides detailed telemetry. Setting Azure AD auth as default is a good security practice.
internal/remotestate/backend/azurerm/backend_interface_test.go (1)
1-3: Consider the implications of the azure build tag.The
//go:build azuretag means these interface compliance tests won't run during normalgo testexecutions. This could lead to interface compliance issues being missed. Consider whether these basic interface tests should run without the build tag.internal/remotestate/backend/azurerm/errors.go (1)
1-13: Package documentation is clear and follows best practices.The package comment clearly describes the purpose and follows Go conventions. The import organization is clean with standard library imports separated from third-party imports.
azurehelper/azure_storage_account.go (2)
1-23: LGTM! Clean package declaration and imports.The package comment is concise and imports are well-organized with Azure SDK imports grouped together.
41-75: Well-structured configuration types with sensible defaults.Good practice organizing struct fields by size/alignment and providing a default configuration function. The security-by-default approach (versioning enabled, public access disabled) is excellent.
test/integration_azure_test.go (1)
1-31: LGTM! Well-organized imports and build tag.Good use of build tag to conditionally compile Azure tests. Imports are properly organized.
|
|
||
| // TestResourceGroupClientCreation tests the creation of resource group client with various inputs | ||
| func TestResourceGroupClientCreation(t *testing.T) { | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing parallel test execution.
For consistency with other tests and to improve test execution time, add t.Parallel().
func TestResourceGroupClientCreation(t *testing.T) {
+ t.Parallel()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In azurehelper/azure_resource_group_test.go at line 206, the test is missing the
call to t.Parallel(), which is needed for parallel test execution. Add
t.Parallel() at the beginning of the test function to enable it to run
concurrently with other tests, improving test execution speed and consistency.
azurehelper/azure_storage_account.go
Outdated
| roleDefinitionID := fmt.Sprintf("/subscriptions/%s/providers/Microsoft.Authorization/roleDefinitions/%s", | ||
| c.subscriptionID, storageBlobDataOwnerRoleID) |
There was a problem hiding this comment.
Undefined constant storageBlobDataOwnerRoleID.
The constant storageBlobDataOwnerRoleID is not defined in this file.
Add the constant definition:
const storageBlobDataOwnerRoleID = "b7e6dc6d-f1e8-4753-8033-0f276bb0955b" // Storage Blob Data Owner role🤖 Prompt for AI Agents
In azurehelper/azure_storage_account.go around lines 713 to 714, the constant
storageBlobDataOwnerRoleID is used but not defined. Define this constant at the
top of the file or in an appropriate constants section with the value
"b7e6dc6d-f1e8-4753-8033-0f276bb0955b" and a comment indicating it is the
Storage Blob Data Owner role ID.
azurehelper/azure_storage_account.go
Outdated
| if respErr.StatusCode == httpStatusNotFound { | ||
| return false, nil, nil | ||
| } |
There was a problem hiding this comment.
Undefined constant httpStatusNotFound.
The constant httpStatusNotFound is not defined. Use the standard library constant instead.
- if respErr.StatusCode == httpStatusNotFound {
+ if respErr.StatusCode == http.StatusNotFound {Similar issues appear at:
- Line 572:
httpStatusNotFound - Line 654:
defaultHTTPClientTimeout - Line 679:
httpStatusOK - Line 795:
httpStatusConflict - Line 805:
httpStatusForbiddenandhttpStatusUnauthorized
Either define these constants or use the standard library http.Status* constants directly.
📝 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.
| if respErr.StatusCode == httpStatusNotFound { | |
| return false, nil, nil | |
| } | |
| if respErr.StatusCode == http.StatusNotFound { | |
| return false, nil, nil | |
| } |
🤖 Prompt for AI Agents
In azurehelper/azure_storage_account.go around lines 173 to 175, replace the
undefined constant httpStatusNotFound with the standard library constant
http.StatusNotFound. Similarly, for the other mentioned lines (572, 654, 679,
795, 805), replace all undefined HTTP status constants with their corresponding
http.Status* constants from the net/http package. For defaultHTTPClientTimeout,
define it properly or use an appropriate standard timeout constant. This ensures
all constants are properly defined and consistent with the standard library.
azurehelper/azure_storage_account.go
Outdated
| clientOptions := &arm.ClientOptions{ | ||
| ClientOptions: azcore.ClientOptions{ | ||
| APIVersion: defaultRoleAssignmentAPIVersion, // Use the latest API version for role assignments | ||
| }, | ||
| } |
There was a problem hiding this comment.
Undefined constant defaultRoleAssignmentAPIVersion.
The constant defaultRoleAssignmentAPIVersion is not defined in this file, which will cause a compilation error.
Add the constant definition at the package level:
const defaultRoleAssignmentAPIVersion = "2022-04-01" // or appropriate versionOr define it in a constants file if you have one.
🤖 Prompt for AI Agents
In azurehelper/azure_storage_account.go around lines 136 to 140, the constant
defaultRoleAssignmentAPIVersion is used but not defined, causing a compilation
error. Define this constant at the package level by adding a line such as 'const
defaultRoleAssignmentAPIVersion = "2022-04-01"' with the appropriate API
version, either directly in this file or in a shared constants file if one
exists.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
azurehelper/azure_resource_group.go (1)
180-187: Fix undefined constanthttpStatusNotFound.The constant
httpStatusNotFoundis not defined, which will cause a compilation error.func IsNotFoundError(err error) bool { var respErr *azcore.ResponseError if errors.As(err, &respErr) { - return respErr.StatusCode == httpStatusNotFound + return respErr.StatusCode == 404 } return false }Alternatively, define the constant at the package level:
const httpStatusNotFound = 404test/integration_azure_test.go (1)
217-217: Remove unreachable code after switch statement.The
return falsestatement is unreachable since all switch cases either return a value or callt.Fatalf()which terminates the test.
🧹 Nitpick comments (2)
test/integration_azure_test.go (2)
420-533: Consider simplifying the test structure for better readability.The inline
checkExpectedResultFnmakes the test case structure complex. Consider either:
- Extracting the check function as a separate helper function
- Simplifying the test to directly test the delete functionality without the complex callback pattern
Example of a simpler approach:
- { - checkExpectedResultFn: func(t *testing.T, output string, containerName string, rootPath string, name string, args string) { - // Complex inline function logic... - }, - name: "delete backend command", - args: "backend delete --force", - containerName: "terragrunt-test-container-" + strings.ToLower(uniqueID()), - }, + // Move the test logic directly into the test body or create a dedicated test function + t.Run("delete backend command", func(t *testing.T) { + containerName := "terragrunt-test-container-" + strings.ToLower(uniqueID()) + // Bootstrap logic + // Delete logic + // Verification logic + })
1-2177: Consider monitoring for Azure resource limits in parallel test execution.While the tests use unique resource names and proper cleanup, running many tests in parallel against the same Azure subscription could hit rate limits or resource quotas. Consider:
- Monitoring test failures related to Azure API rate limits
- Potentially adding test tags to control parallelism for resource-intensive tests
- Documenting any Azure subscription requirements for running the full test suite
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
azurehelper/azure_blob_test.go(1 hunks)azurehelper/azure_resource_group.go(1 hunks)azurehelper/azure_resource_group_test.go(1 hunks)azurehelper/utility_functions_test.go(1 hunks)config/dependency.go(4 hunks)config/dependency_test.go(2 hunks)docs-starlight/src/content/docs/04-reference/04-experiments.md(2 hunks)internal/remotestate/backend/azurerm/backend_test.go(1 hunks)internal/remotestate/backend/azurerm/errors.go(1 hunks)internal/remotestate/remote_state_test.go(3 hunks)test/integration_azure_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- azurehelper/azure_blob_test.go
- config/dependency.go
- config/dependency_test.go
- azurehelper/utility_functions_test.go
- internal/remotestate/backend/azurerm/errors.go
- azurehelper/azure_resource_group_test.go
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
azurehelper/azure_resource_group.gointernal/remotestate/remote_state_test.gointernal/remotestate/backend/azurerm/backend_test.gotest/integration_azure_test.go
`docs-starlight/**/*.md*`: Review the documentation for clarity, grammar, and sp...
docs-starlight/**/*.md*: Review the documentation for clarity, grammar, and spelling. Make sure that the documentation is easy to understand and follow. There is currently a migration underway from the Jekyll based documentation indocsto the Starlight + Astro based documentation indocs-starlight. Make sure that thedocs-starlightdocumentation is accurate and up-to-date with thedocsdocumentation, and that any difference between them results in an improvement in thedocs-starlightdocumentation.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs-starlight/src/content/docs/04-reference/04-experiments.md
🧠 Learnings (2)
internal/remotestate/remote_state_test.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
test/integration_azure_test.go (1)
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
🧬 Code Graph Analysis (1)
test/integration_azure_test.go (9)
test/helpers/logger/logger.go (1)
CreateLogger(9-14)azurehelper/azure_blob.go (3)
CreateBlobServiceClient(89-205)BlobServiceClient(20-23)ContainerCreationError(373-376)internal/remotestate/backend/azurerm/errors.go (9)
AuthenticationError(107-110)StorageAccountCreationError(75-78)ContainerValidationError(123-125)ContainerCreationError(91-94)MissingSubscriptionIDError(51-51)MissingLocationError(59-59)MissingResourceGroupError(133-133)MissingRequiredAzureRemoteStateConfig(15-15)NoValidAuthMethodError(67-67)cli/commands/backend/migrate/migrate.go (1)
Run(18-97)internal/remotestate/backend/azurerm/backend.go (1)
NewBackend(76-81)util/file.go (2)
JoinPath(481-483)IsDir(184-187)azurehelper/azure_storage_account.go (3)
StorageAccountConfig(43-62)CreateStorageAccountClient(78-161)GetAzureCredentials(854-922)azurehelper/azure_resource_group.go (1)
CreateResourceGroupClient(34-76)test/helpers/package.go (3)
RunTerragruntCommandWithOutput(894-898)RunTerragruntCommand(853-856)RunTerragrunt(866-870)
🪛 LanguageTool
docs-starlight/src/content/docs/04-reference/04-experiments.md
[uncategorized] ~179-~179: The preposition ‘to’ seems more likely in this position.
Context: ...les the Azure Storage backend (azurerm) for Terraform remote state. When this exper...
(AI_HYDRA_LEO_REPLACE_FOR_TO)
🔇 Additional comments (8)
internal/remotestate/remote_state_test.go (1)
19-618: Excellent test coverage for Azure backend support!The test enhancements are comprehensive and well-structured:
- Clean MockBackend implementation for test isolation
- Proper experiment enablement through helper functions
- Thorough coverage of Azure backend configuration scenarios
- Good validation of Terragrunt-specific option filtering
- Comprehensive OpenTofu code generation tests
internal/remotestate/backend/azurerm/backend_test.go (1)
1-1609: Outstanding test suite for Azure backend implementation!This is an exemplary test suite with:
- Comprehensive coverage of all backend methods and error paths
- Extensive edge case testing, especially for container name validation
- Proper test isolation avoiding actual Azure API calls
- Clean table-driven test structure
- Appropriate use of parallel test execution
- Thorough validation of Azure-specific constraints and requirements
The attention to detail in testing error scenarios and edge cases is particularly commendable.
test/integration_azure_test.go (6)
32-65: Well-implemented experiment flag helpers!The
setAndRestoreTGExperimentandaddAzureBackendExperimentFlagfunctions are well-designed with proper cleanup logic and edge case handling.
248-284: Excellent implementation of object ID lookup with fallback logic!The function provides a robust way to determine the Azure user's object ID with proper fallback to Azure CLI and graceful handling when the ID cannot be determined.
1526-1558: Excellent resource cleanup implementation!The cleanup logic correctly handles the deletion order (storage account before resource group) and includes proper error handling. The approach prevents orphaned resources and handles potential race conditions gracefully.
1868-1896: Good use of helper functions for experiment setup.The implementation properly uses
setAndRestoreTGExperimentandaddAzureBackendExperimentFlaghelpers. While the pattern is duplicated across the three functions, this is acceptable given their different signatures and purposes.
1912-2006: Outstanding test setup helper with robust cleanup!The
setupAzureTestfunction provides comprehensive test environment setup with:
- Proper credential and subscription validation
- Robust cleanup with retry logic
- Good resource tagging for identification
- Excellent error handling throughout
This is a model implementation for integration test helpers.
1-2177: Excellent comprehensive test coverage!This integration test suite provides thorough coverage of Azure backend functionality including:
- RBAC and authentication scenarios
- Storage account and container operations
- Error handling and custom error types
- Backend migration workflows
- Configuration updates
The test organization and coverage demonstrate high-quality testing practices.
|
|
||
| #### `azure-backend` - How to provide feedback | ||
|
|
||
| If you use the Azure backend, please share your experience in the [Azure Backend for Remote State](https://github.com/gruntwork-io/terragrunt/discussions) GitHub Discussions. Your feedback will help improve the stability and features of this backend. |
There was a problem hiding this comment.
Complete the GitHub Discussions link with a specific discussion.
The feedback link should point to a specific GitHub Discussion for the Azure Backend experiment, not just the general discussions page.
-If you use the Azure backend, please share your experience in the [Azure Backend for Remote State](https://github.com/gruntwork-io/terragrunt/discussions) GitHub Discussions. Your feedback will help improve the stability and features of this backend.
+If you use the Azure backend, please share your experience in the [Azure Backend for Remote State](https://github.com/gruntwork-io/terragrunt/discussions/XXXX) GitHub Discussions. Your feedback will help improve the stability and features of this backend.Replace XXXX with the actual discussion number once created.
📝 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.
| If you use the Azure backend, please share your experience in the [Azure Backend for Remote State](https://github.com/gruntwork-io/terragrunt/discussions) GitHub Discussions. Your feedback will help improve the stability and features of this backend. | |
| If you use the Azure backend, please share your experience in the [Azure Backend for Remote State](https://github.com/gruntwork-io/terragrunt/discussions/XXXX) GitHub Discussions. Your feedback will help improve the stability and features of this backend. |
🤖 Prompt for AI Agents
In docs-starlight/src/content/docs/04-reference/04-experiments.md at line 191,
the GitHub Discussions link currently points to the general discussions page.
Update the URL to link directly to the specific GitHub Discussion for the Azure
Backend experiment by replacing the placeholder `XXXX` with the actual
discussion number once it is created, ensuring users are directed to the
relevant feedback thread.
…reated before trying to use it
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
azurehelper/azure_blob_test.go (1)
180-181: Remove orphaned comment.The comment placeholder for the stringPtr helper function should be removed since the tests are using
azurehelper.StringPtrinstead.azurehelper/azure_resource_group.go (1)
180-187: Fix undefined constanthttpStatusNotFound.The constant
httpStatusNotFoundis not defined, which will cause a compilation error.func IsNotFoundError(err error) bool { var respErr *azcore.ResponseError if errors.As(err, &respErr) { - return respErr.StatusCode == httpStatusNotFound + return respErr.StatusCode == 404 } return false }azurehelper/azure_blob.go (1)
364-400: Fix nil logger and implement streaming for large files.Two critical issues in the CopyBlobToContainer implementation:
- Line 395 passes
nilas the logger parameter toUploadBlob, which could cause issues- Reading the entire blob into memory (line 389) is problematic for large files
- if err := dstClient.UploadBlob(ctx, nil, dstContainer, dstKey, blobData); err != nil { + // Get logger from context or create a default one + l := log.Default() + if err := dstClient.UploadBlob(ctx, l, dstContainer, dstKey, blobData); err != nil {Consider implementing streaming copy for large files to avoid memory issues. This would require refactoring to use Azure's blob copy operations or implementing a streaming upload method.
🧹 Nitpick comments (2)
azurehelper/azure_blob_test.go (1)
155-166: Remove unused MockResponseError struct.The
MockResponseErrorstruct and itsError()method are not used anywhere in the tests. Remove this dead code to keep the test file clean.-// Mock Azure Response Error for testing -type MockResponseError struct { - // String fields first (8-byte alignment) - ErrorCode string - Message string - // Then int fields (4-byte alignment) - StatusCode int -} - -func (e *MockResponseError) Error() string { - return fmt.Sprintf("Status: %d, Code: %s, Message: %s", e.StatusCode, e.ErrorCode, e.Message) -}test/integration_azure_test.go (1)
2061-2063: Consider making sleep duration configurable.The hardcoded 5-second sleep could make tests flaky in different environments. Consider making this configurable via environment variable or using retry logic instead.
- sleepTime := 5 * time.Second // Allow some time for the container to be fully ready - time.Sleep(sleepTime) + sleepTime := 5 * time.Second + if envSleep := os.Getenv("TERRAGRUNT_TEST_SLEEP_SECONDS"); envSleep != "" { + if duration, err := time.ParseDuration(envSleep + "s"); err == nil { + sleepTime = duration + } + } + time.Sleep(sleepTime)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
azurehelper/azure_blob.go(1 hunks)azurehelper/azure_blob_service_test.go(1 hunks)azurehelper/azure_blob_test.go(1 hunks)azurehelper/azure_resource_group.go(1 hunks)azurehelper/azure_resource_group_test.go(1 hunks)azurehelper/azure_storage_account_test.go(1 hunks)test/integration_azure_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- azurehelper/azure_resource_group_test.go
- azurehelper/azure_blob_service_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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
azurehelper/azure_blob_test.goazurehelper/azure_resource_group.goazurehelper/azure_storage_account_test.goazurehelper/azure_blob.gotest/integration_azure_test.go
🧠 Learnings (3)
azurehelper/azure_blob_test.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
azurehelper/azure_blob.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
test/integration_azure_test.go (1)
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
🧬 Code Graph Analysis (2)
azurehelper/azure_blob_test.go (2)
azurehelper/azure_blob.go (2)
AzureResponseError(62-66)GetObjectInput(47-50)azurehelper/test_helpers.go (1)
StringPtr(7-9)
test/integration_azure_test.go (9)
test/helpers/logger/logger.go (1)
CreateLogger(9-14)azurehelper/azure_blob.go (3)
CreateBlobServiceClient(110-235)BlobServiceClient(41-44)ContainerCreationError(403-406)internal/remotestate/backend/azurerm/errors.go (9)
AuthenticationError(107-110)StorageAccountCreationError(75-78)ContainerValidationError(123-125)ContainerCreationError(91-94)MissingSubscriptionIDError(51-51)MissingLocationError(59-59)MissingResourceGroupError(133-133)MissingRequiredAzureRemoteStateConfig(15-15)NoValidAuthMethodError(67-67)cli/commands/backend/migrate/migrate.go (1)
Run(18-97)internal/remotestate/backend/azurerm/backend.go (1)
NewBackend(76-81)util/file.go (2)
JoinPath(481-483)IsDir(184-187)azurehelper/azure_storage_account.go (3)
StorageAccountConfig(43-62)CreateStorageAccountClient(78-161)GetAzureCredentials(854-922)azurehelper/azure_resource_group.go (1)
CreateResourceGroupClient(34-76)test/helpers/package.go (3)
RunTerragruntCommandWithOutput(894-898)RunTerragruntCommand(853-856)RunTerragrunt(866-870)
🔇 Additional comments (14)
azurehelper/azure_storage_account_test.go (1)
1-2487: Comprehensive test coverage!The test file provides excellent coverage including:
- Input validation and edge cases
- Concurrency safety tests
- Memory usage scenarios
- Update logic validation
- Error handling paths
The table-driven test approach and proper parallel execution make the tests maintainable and efficient.
test/integration_azure_test.go (13)
154-218: Well-implemented error type assertion helper.The
assertAzureErrorTypefunction provides comprehensive error type checking with proper fallbacks. Good implementation of handling both custom error types and general error message patterns.
286-414: Excellent RBAC integration test with proper error handling.The test properly handles environment setup, credentials management, and validates both success and failure scenarios appropriately. Good use of cleanup patterns and error type assertions.
420-533: Comprehensive bootstrap backend test with good cleanup.The table-driven test structure effectively validates both bootstrap and delete operations. The nested verification logic ensures proper state management throughout the test lifecycle.
536-595: Well-structured remote state output test.The test properly manages dependency order and validates output retrieval from Azure remote state. Good use of JSON parsing and output verification.
615-700: Comprehensive container creation test with proper error validation.The test covers both successful container creation and proper error handling for invalid container names. Good use of helper functions and error type assertions.
703-789: Thorough storage account bootstrap test coverage.The tests properly validate both existing and non-existent storage account scenarios with appropriate error type checking and fallback validation.
790-823: Clean blob operations test with proper setup.The test effectively validates basic blob operations with appropriate setup and cleanup patterns.
826-889: Excellent end-to-end storage account and blob test.The test provides comprehensive validation of the complete workflow from storage account creation to blob operations with proper data integrity checks.
892-995: Comprehensive backend bootstrap integration test.The test properly validates both success and failure scenarios for backend bootstrap operations with appropriate error type checking and resource cleanup.
998-1222: Excellent comprehensive error type validation test.The test systematically validates all custom Azure error types with proper table-driven testing for container validation scenarios. Good fallback error checking when specific types aren't matched.
1224-1409: Thorough error unwrapping and propagation test.The test excellently validates error handling through the entire call stack, ensuring proper error wrapping, unwrapping, and propagation with comprehensive type checking.
1629-1801: Excellent comprehensive migration test.The migration test provides thorough validation of state migration between different paths, including proper verification of state preservation and cleanup. Good use of directory management and state validation.
1804-2177: Well-structured helper functions with good reusability.The helper functions provide excellent support for the test infrastructure with proper error handling, cleanup patterns, and resource management. Good separation of concerns and reusability.
Cleanup error handling
Add rbac retry
|
@omattsson is attempting to deploy a commit to the Gruntwork Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (6)
azurehelper/azure_blob.go (1)
316-352: Two concerns with the CopyBlobToContainer implementation remain unaddressed.
- Line 347 passes
nilas the logger parameter toUploadBlob, which expects alog.Logger. This could cause a panic.- Reading the entire blob into memory (line 341) could cause issues with large files.
The previous review already identified these issues. Apply this fix:
- // Upload to the destination - if err := dstClient.UploadBlob(ctx, nil, dstContainer, dstKey, blobData); err != nil { + // Get logger from context or create a default one + l := log.Default() + if err := dstClient.UploadBlob(ctx, l, dstContainer, dstKey, blobData); err != nil {For large file support, consider implementing streaming copy:
// Stream copy implementation func (c *BlobServiceClient) StreamCopyBlobToContainer(ctx context.Context, srcContainer, srcKey string, dstClient *BlobServiceClient, dstContainer, dstKey string) error { // Use Azure's built-in copy functionality or stream the data // without loading it entirely into memory }#!/bin/bash # Check if there are other calls to UploadBlob with nil logger rg "UploadBlob\([^,]+,\s*nil" --type goazurehelper/azure_storage_account.go (5)
264-266: Undefined constantdefaultLocation.The constant
defaultLocationis not defined in this file.- location = defaultLocation // Default location + location = "westeurope" // Default locationOr better, define it as a package constant:
const defaultLocation = "westeurope"This issue also appears at lines 369-371.
138-142: Undefined constantdefaultRoleAssignmentAPIVersion.The constant
defaultRoleAssignmentAPIVersionis not defined in this file, which will cause a compilation error.Add the constant definition at the package level:
const defaultRoleAssignmentAPIVersion = "2022-04-01" // or appropriate versionOr define it in a constants file if you have one.
715-716: Undefined constantstorageBlobDataOwnerRoleID.The constant
storageBlobDataOwnerRoleIDis not defined in this file.Add the constant definition:
const storageBlobDataOwnerRoleID = "b7e6dc6d-f1e8-4753-8033-0f276bb0955b" // Storage Blob Data Owner role
175-177: Undefined constanthttpStatusNotFound.The constant
httpStatusNotFoundis not defined. Use the standard library constant instead.- if respErr.StatusCode == httpStatusNotFound { + if respErr.StatusCode == http.StatusNotFound {Similar issues appear at:
- Line 572:
httpStatusNotFound- Line 654:
defaultHTTPClientTimeout- Line 679:
httpStatusOK- Line 795:
httpStatusConflict- Line 805:
httpStatusForbiddenandhttpStatusUnauthorizedEither define these constants or use the standard library
http.Status*constants directly.
981-984: MoveGenerateUUIDto a utility package.This generic UUID generation function doesn't belong in the storage account file. Consider moving it to a shared utility package.
Move this function to a utility package like
internal/util/uuid.goor similar, and import it here.
🧹 Nitpick comments (3)
azurehelper/azure_rbac_constants_test.go (1)
34-49: Consider removing redundant test cases.The test cases in lines 34-48 don't add value as they only verify basic arithmetic (maxRetries + 1 = expectedAttempts) without testing the actual package constants. These test cases use hardcoded values rather than the actual constants from the
azurehelperpackage.Consider either removing these test cases or refactoring them to test actual behavior:
- // Test different values of the relationship - testCases := []struct { - maxRetries int - expectedAttempts int - }{ - {3, 4}, - {5, 6}, // Current value - {10, 11}, - {0, 1}, // Edge case: no retries means 1 attempt - } - - for _, tc := range testCases { - calculatedAttempts := tc.maxRetries + 1 - assert.Equal(t, tc.expectedAttempts, calculatedAttempts, - "When max retries is %d, attempts should be %d", tc.maxRetries, tc.expectedAttempts) - } + // Verify the relationship holds for the actual constants + assert.Equal(t, azurehelper.RbacMaxRetries+1, azurehelper.RbacRetryAttempts, + "RbacRetryAttempts should equal RbacMaxRetries+1")azurehelper/azure_storage_account_test.go (1)
954-961: Use standard libraryslices.Containsinstead of custom implementation.Go 1.21+ provides
slices.Containsin the standard library.+import "slices" -// Helper function to check if a slice contains a string -func contains(slice []string, str string) bool { - for _, v := range slice { - if v == str { - return true - } - } - return false -} // Then replace all calls to contains() with slices.Contains() -if !contains(validTiers, tier) { +if !slices.Contains(validTiers, tier) {internal/remotestate/backend/azurerm/telemetry.go (1)
365-371: Consider more secure subscription ID masking.The current implementation shows 8 characters of the subscription ID, which might be too revealing for security purposes.
Consider showing fewer characters or using a hash:
func MaskSubscriptionID(subscriptionID string) string { - if len(subscriptionID) < 8 { - return "****" - } - - return subscriptionID[:4] + "****" + subscriptionID[len(subscriptionID)-4:] + if len(subscriptionID) < 12 { + return "****" + } + // Show only first 2 and last 2 characters + return subscriptionID[:2] + "****" + subscriptionID[len(subscriptionID)-2:] }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
azurehelper/azure_blob.go(1 hunks)azurehelper/azure_constants.go(1 hunks)azurehelper/azure_rbac_constants_test.go(1 hunks)azurehelper/azure_storage_account.go(1 hunks)azurehelper/azure_storage_account_test.go(1 hunks)internal/remotestate/backend/azurerm/backend.go(1 hunks)internal/remotestate/backend/azurerm/retry.go(1 hunks)internal/remotestate/backend/azurerm/telemetry.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- azurehelper/azure_constants.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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
azurehelper/azure_rbac_constants_test.gointernal/remotestate/backend/azurerm/retry.goazurehelper/azure_storage_account_test.gointernal/remotestate/backend/azurerm/telemetry.goazurehelper/azure_blob.gointernal/remotestate/backend/azurerm/backend.goazurehelper/azure_storage_account.go
🧠 Learnings (5)
azurehelper/azure_rbac_constants_test.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
internal/remotestate/backend/azurerm/retry.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
internal/remotestate/backend/azurerm/telemetry.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
azurehelper/azure_blob.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
azurehelper/azure_storage_account.go (1)
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
🧬 Code Graph Analysis (4)
azurehelper/azure_rbac_constants_test.go (2)
azurehelper/azure_constants.go (3)
RbacRetryDelay(33-33)RbacMaxRetries(34-34)RbacRetryAttempts(35-35)azurehelper/azure_storage_account.go (1)
StorageAccountClient(27-41)
internal/remotestate/backend/azurerm/retry.go (4)
pkg/log/log.go (3)
Infof(82-84)Debugf(72-74)Warnf(92-94)internal/remotestate/backend/azurerm/errors.go (3)
WrapMaxRetriesExceededError(333-344)TransientAzureError(177-181)WrapTransientAzureError(320-330)internal/errors/export.go (1)
As(7-9)azurehelper/azure_blob.go (1)
ConvertAzureError(48-63)
internal/remotestate/backend/azurerm/telemetry.go (3)
telemetry/telemeter.go (1)
Telemeter(11-14)azurehelper/azure_blob.go (1)
ConvertAzureError(48-63)pkg/log/log.go (2)
Warnf(92-94)Infof(82-84)
azurehelper/azure_blob.go (4)
internal/errors/export.go (2)
As(7-9)Unwrap(24-26)azurehelper/azure_storage_account.go (2)
StorageAccountClient(27-41)CreateStorageAccountClient(80-163)pkg/log/log.go (1)
Infof(82-84)internal/remotestate/backend/azurerm/errors.go (1)
ContainerCreationError(91-94)
🔇 Additional comments (6)
internal/remotestate/backend/azurerm/backend.go (3)
32-67: Excellent container name validation implementation.The validation logic comprehensively covers Azure Storage container naming requirements with clear error messages and proper regex patterns. The step-by-step validation approach makes the code readable and maintainable.
188-191: Proper concurrency control implementation.Good use of mutex to prevent race conditions during storage account initialization. The locking pattern correctly ensures only one goroutine can initialize storage at a time.
245-276: Excellent retry logic implementation.The retry logic with
WithRetryandDefaultRetryConfig()provides robust handling of transient Azure API failures. The error wrapping withWrapTransientErroris particularly well done.azurehelper/azure_storage_account.go (3)
86-97: Excellent fallback logic for resource group naming.The automatic fallback to using storage account name + "-rg" when no resource group is specified provides a sensible default while warning the user. This improves usability without being overly opinionated.
986-1056: Robust authentication credential resolution.The
GetAzureCredentialsfunction properly handles multiple authentication methods with clear precedence (AZURE_* over ARM_* environment variables) and comprehensive logging. The fallback to default authentication is well implemented.
704-740: Well-structured RBAC role assignment with proper error handling.The role assignment logic correctly handles various scenarios: existing assignments (409 conflict), permission errors, and retry logic. The separation of concerns with helper functions improves readability.
| // Check for HTTP status codes in error messages (common pattern from Azure SDK) | ||
| errorStr := strings.ToLower(err.Error()) | ||
|
|
||
| // HTTP 429 - Too Many Requests | ||
| if strings.Contains(errorStr, "429") || strings.Contains(errorStr, "too many requests") || | ||
| strings.Contains(errorStr, "rate limit") { | ||
| return true | ||
| } | ||
|
|
||
| // HTTP 500 - Internal Server Error | ||
| if strings.Contains(errorStr, "500") || strings.Contains(errorStr, "internal server error") { | ||
| return true | ||
| } | ||
|
|
||
| // HTTP 502 - Bad Gateway | ||
| if strings.Contains(errorStr, "502") || strings.Contains(errorStr, "bad gateway") { | ||
| return true | ||
| } | ||
|
|
||
| // HTTP 503 - Service Unavailable | ||
| if strings.Contains(errorStr, "503") || strings.Contains(errorStr, "service unavailable") || | ||
| strings.Contains(errorStr, "temporarily unavailable") { | ||
| return true | ||
| } | ||
|
|
||
| // HTTP 504 - Gateway Timeout | ||
| if strings.Contains(errorStr, "504") || strings.Contains(errorStr, "gateway timeout") || | ||
| strings.Contains(errorStr, "request timeout") { | ||
| return true | ||
| } | ||
|
|
||
| // Network-related errors | ||
| if strings.Contains(errorStr, "connection reset") || | ||
| strings.Contains(errorStr, "connection refused") || | ||
| strings.Contains(errorStr, "network is unreachable") || | ||
| strings.Contains(errorStr, "timeout") || | ||
| strings.Contains(errorStr, "temporary failure") { | ||
| return true | ||
| } | ||
|
|
||
| // Azure-specific transient errors | ||
| if strings.Contains(errorStr, "throttled") || | ||
| strings.Contains(errorStr, "server busy") || | ||
| strings.Contains(errorStr, "operation timeout") || | ||
| strings.Contains(errorStr, "request rate is large") { | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error classification robustness.
The current implementation relies on string matching which can lead to false positives. For example, checking for "429" in the error string could match unrelated numbers.
Consider using more specific patterns or regular expressions:
- // HTTP 429 - Too Many Requests
- if strings.Contains(errorStr, "429") || strings.Contains(errorStr, "too many requests") ||
- strings.Contains(errorStr, "rate limit") {
- return true
- }
+ // HTTP 429 - Too Many Requests
+ if strings.Contains(errorStr, "status 429") || strings.Contains(errorStr, "status code 429") ||
+ strings.Contains(errorStr, "http 429") || strings.Contains(errorStr, "too many requests") ||
+ strings.Contains(errorStr, "rate limit") {
+ return true
+ }Additionally, consider extracting these patterns into constants for better maintainability:
var retryablePatterns = []string{
"status 429", "too many requests", "rate limit",
"status 500", "internal server error",
// ... other patterns
}🤖 Prompt for AI Agents
In internal/remotestate/backend/azurerm/retry.go between lines 126 and 175, the
error classification uses simple substring matching which risks false positives
like matching unrelated numbers. To fix this, replace the substring checks with
more precise regular expressions that match HTTP status codes and error phrases
in context, ensuring accurate detection. Also, extract all retryable error
patterns into a dedicated constant slice or array at the top of the file for
better maintainability and clarity, then iterate over this collection to check
errors instead of hardcoding each condition.
| // Add jitter to avoid thundering herd | ||
| if config.Jitter { | ||
| jitterRange := duration / JitterDivisor // 25% jitter | ||
| // Generate random jitter between 0 and jitterRange | ||
| nanoTime := time.Now().UnixNano() | ||
| jitterMultiplier := float64(nanoTime%JitterModulo) / JitterDivisorFloat64 // 0.0 to 1.0 | ||
| jitter := time.Duration(float64(jitterRange) * jitterMultiplier) | ||
| duration += jitter | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper random number generator for jitter calculation.
Using time.Now().UnixNano() for jitter calculation can produce predictable patterns in high-frequency scenarios.
Use Go's math/rand package with a proper seed:
+import (
+ "math/rand"
+ "sync"
+)
+
+var (
+ jitterRand *rand.Rand
+ jitterMu sync.Mutex
+)
+
+func init() {
+ jitterRand = rand.New(rand.NewSource(time.Now().UnixNano()))
+}
// Add jitter to avoid thundering herd
if config.Jitter {
jitterRange := duration / JitterDivisor // 25% jitter
- // Generate random jitter between 0 and jitterRange
- nanoTime := time.Now().UnixNano()
- jitterMultiplier := float64(nanoTime%JitterModulo) / JitterDivisorFloat64 // 0.0 to 1.0
+ // Generate random jitter between 0 and jitterRange
+ jitterMu.Lock()
+ jitterMultiplier := jitterRand.Float64() // 0.0 to 1.0
+ jitterMu.Unlock()
jitter := time.Duration(float64(jitterRange) * jitterMultiplier)
duration += jitter
}📝 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.
| // Add jitter to avoid thundering herd | |
| if config.Jitter { | |
| jitterRange := duration / JitterDivisor // 25% jitter | |
| // Generate random jitter between 0 and jitterRange | |
| nanoTime := time.Now().UnixNano() | |
| jitterMultiplier := float64(nanoTime%JitterModulo) / JitterDivisorFloat64 // 0.0 to 1.0 | |
| jitter := time.Duration(float64(jitterRange) * jitterMultiplier) | |
| duration += jitter | |
| } | |
| // Add jitter to avoid thundering herd | |
| if config.Jitter { | |
| jitterRange := duration / JitterDivisor // 25% jitter | |
| // Generate random jitter between 0 and jitterRange | |
| jitterMu.Lock() | |
| jitterMultiplier := jitterRand.Float64() // 0.0 to 1.0 | |
| jitterMu.Unlock() | |
| jitter := time.Duration(float64(jitterRange) * jitterMultiplier) | |
| duration += jitter | |
| } |
🤖 Prompt for AI Agents
In internal/remotestate/backend/azurerm/retry.go around lines 189 to 197, the
jitter calculation uses time.Now().UnixNano() which can lead to predictable
patterns. Replace this with a call to math/rand's random number generator,
properly seeded, to generate the jitter multiplier. Import math/rand, seed it
once (e.g., in init or globally), and use rand.Float64() to get a random float
between 0 and 1 for the jitter calculation instead of using time-based values.
| // Implement a basic validation function similar to what Azure might use | ||
| // This doesn't call the actual Azure helper but mimics what validation logic would do | ||
| var err error | ||
| switch { | ||
| case tc.saName == "": | ||
| err = errors.New("name cannot be empty") | ||
| case len(tc.saName) < 3 || len(tc.saName) > 24: | ||
| err = errors.New("name must be between 3 and 24 characters") | ||
| case tc.saName != strings.ToLower(tc.saName): | ||
| err = errors.New("name must be lowercase") | ||
| case !regexp.MustCompile("^[a-z][a-z0-9]*$").MatchString(tc.saName): | ||
| if !regexp.MustCompile("^[a-z]").MatchString(tc.saName) { | ||
| err = errors.New("name must start with a letter") | ||
| } else { | ||
| err = errors.New("name must contain only letters and numbers") | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Test should use actual validation function instead of reimplementing logic.
The test reimplements validation logic instead of calling the actual validation function from the package, which means it's not testing the real implementation.
Replace the inline validation with a call to the actual validation function:
- // Implement a basic validation function similar to what Azure might use
- // This doesn't call the actual Azure helper but mimics what validation logic would do
- var err error
- switch {
- case tc.saName == "":
- err = errors.New("name cannot be empty")
- case len(tc.saName) < 3 || len(tc.saName) > 24:
- err = errors.New("name must be between 3 and 24 characters")
- case tc.saName != strings.ToLower(tc.saName):
- err = errors.New("name must be lowercase")
- case !regexp.MustCompile("^[a-z][a-z0-9]*$").MatchString(tc.saName):
- if !regexp.MustCompile("^[a-z]").MatchString(tc.saName) {
- err = errors.New("name must start with a letter")
- } else {
- err = errors.New("name must contain only letters and numbers")
- }
- }
+ // Call the actual validation function from the package
+ err := azurehelper.ValidateStorageAccountName(tc.saName)🏁 Script executed:
#!/bin/bash
# Search for the actual storage account name validation function
ast-grep --pattern 'func ValidateStorageAccountName($$$) $$$'
rg -A 5 "ValidateStorageAccountName|validate.*storage.*account.*name" --type goLength of output: 145
To locate the real validation logic, let’s list all functions in the azurehelper package and look for any Validate… methods:
This will help us identify if a storage account name validation function exists and what it’s called.
🏁 Script executed:
#!/bin/bash
# List all function definitions in the azurehelper directory
rg -n "^func " azurehelper --type goLength of output: 12160
We need to inspect how StorageAccountConfig.Validate() handles the name. Let’s pull in its implementation:
🏁 Script executed:
#!/bin/bash
rg -A 50 "func (cfg StorageAccountConfig) Validate" azurehelper/azure_storage_account.goLength of output: 90
To pinpoint the storage-account name checks, let’s search for all Validate( methods in the azurehelper package:
🏁 Script executed:
#!/bin/bash
# Find all Validate methods in azurehelper
rg -n "Validate(" azurehelperLength of output: 100
To confirm whether StorageAccountConfig.Validate() covers name validation, let’s pull its definition and surrounding logic:
🏁 Script executed:
#!/bin/bash
# Locate the Validate method for StorageAccountConfig
rg -n "func (cfg StorageAccountConfig) Validate" azurehelper/azure_storage_account.go --type go
# Print the implementation of that Validate method
sed -n '1070,1150p' azurehelper/azure_storage_account.goLength of output: 2130
Implement and Reuse a Storage Account Name Validation Helper
The test currently re-implements the Azure name rules because there’s no shared validation function in the azurehelper package. To DRY this up and ensure your tests cover the real logic, please:
- In azurehelper/azure_storage_account.go, add a new helper:
+ import (
+ "errors"
+ "regexp"
+ "strings"
+ )
+
+ // ValidateStorageAccountName checks Azure storage account name constraints.
+ func ValidateStorageAccountName(name string) error {
+ switch {
+ case name == "":
+ return errors.New("name cannot be empty")
+ case len(name) < 3 || len(name) > 24:
+ return errors.New("name must be between 3 and 24 characters")
+ case name != strings.ToLower(name):
+ return errors.New("name must be lowercase")
+ case !regexp.MustCompile("^[a-z][a-z0-9]*$").MatchString(name):
+ if !regexp.MustCompile("^[a-z]").MatchString(name) {
+ return errors.New("name must start with a letter")
+ }
+ return errors.New("name must contain only letters and numbers")
+ }
+ return nil
+ }- In azurehelper/azure_storage_account_test.go (around lines 287–300), replace the inline switch with:
- // Inline validation logic
- var err error
- switch {
- // … existing cases …
- }
+ // Use the shared validation helper
+ err := azurehelper.ValidateStorageAccountName(tc.saName)Now both your production code and tests rely on the same validation implementation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In azurehelper/azure_storage_account.go, create a new helper function that
validates storage account names according to Azure rules. Then, in
azurehelper/azure_storage_account_test.go around lines 287 to 300, remove the
inline switch statement that duplicates validation logic and replace it by
calling this new helper function. This ensures both production and test code use
the same validation logic and avoids duplication.
azurehelper/azure_blob.go
Outdated
| defer func() { | ||
| if closeErr := srcBlobOutput.Body.Close(); closeErr != nil { | ||
| err = fmt.Errorf("failed to close blob: %w (original error: %w)", closeErr, err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Fix error handling in defer block.
The defer block attempts to modify the error return value, but this doesn't work without named return values.
Either add named return values or handle the close error differently:
-func (c *BlobServiceClient) CopyBlobToContainer(ctx context.Context, srcContainer, srcKey string, dstClient *BlobServiceClient,
- dstContainer, dstKey string) error {
+func (c *BlobServiceClient) CopyBlobToContainer(ctx context.Context, srcContainer, srcKey string, dstClient *BlobServiceClient,
+ dstContainer, dstKey string) (err error) {Or log the close error instead:
defer func() {
if closeErr := srcBlobOutput.Body.Close(); closeErr != nil {
- err = fmt.Errorf("failed to close blob: %w (original error: %w)", closeErr, err)
+ // Log the error since we can't return it
+ log.Warnf("Failed to close blob body: %v", closeErr)
}
}()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In azurehelper/azure_blob.go around lines 334 to 338, the defer block tries to
assign a new error to the named return variable 'err', but the function does not
have named return values, so this assignment has no effect. To fix this, either
add named return values to the function signature so the defer can modify 'err',
or change the defer block to handle the close error differently, such as logging
the close error instead of assigning it to 'err'.
azurehelper/azure_storage_account.go
Outdated
| } | ||
|
|
||
| switch accessTierStr { | ||
| case AccessTierHot, AccessTierCool, AccessTierPremium: | ||
| // Valid tier | ||
| default: | ||
| l.Warnf("Unsupported access tier %s, using Hot", accessTierStr) | ||
| accessTierStr = AccessTierHot | ||
| } | ||
|
|
||
| // Convert tags map to pointer map | ||
| l.Infof("Using access tier: %s", accessTierStr) | ||
|
|
||
| tags := make(map[string]*string, len(config.Tags)) | ||
|
|
||
| if len(config.Tags) > 0 { | ||
| for k, v := range config.Tags { | ||
| value := v // Create a new variable to avoid capturing the loop variable | ||
| tags[k] = &value | ||
| } | ||
| } else { | ||
| // Set default tags if none provided | ||
| defaultTag := "terragrunt" | ||
| tags["created-by"] = &defaultTag | ||
| } | ||
|
|
||
| // Use provided location or default | ||
| location := config.Location | ||
| if location == "" { | ||
| location = c.location | ||
| if location == "" { | ||
| location = defaultLocation // Default location | ||
| l.Warnf("No location specified, using default location: %s", location) | ||
| } | ||
| } | ||
|
|
||
| // Note: The actual structure depends on the SDK version | ||
| // This is a simplified version that should work with most SDK versions | ||
| parameters := armstorage.AccountCreateParameters{ | ||
| SKU: &armstorage.SKU{ | ||
| Name: &sku, | ||
| }, | ||
| Kind: &kind, | ||
| // Properties are set directly on AccountCreateParameters in some SDK versions | ||
| Location: to.Ptr(location), | ||
| Tags: tags, | ||
| } | ||
|
|
||
| // Set properties for the storage account | ||
| var accessTier *armstorage.AccessTier | ||
|
|
||
| switch accessTierStr { | ||
| case AccessTierHot: | ||
| accessTier = to.Ptr(armstorage.AccessTierHot) | ||
| case AccessTierCool: | ||
| accessTier = to.Ptr(armstorage.AccessTierCool) | ||
| case AccessTierPremium: | ||
| accessTier = to.Ptr(armstorage.AccessTierPremium) | ||
| } | ||
|
|
||
| // Create properties object | ||
| parameters.Properties = &armstorage.AccountPropertiesCreateParameters{ | ||
| EnableHTTPSTrafficOnly: to.Ptr(true), | ||
| MinimumTLSVersion: to.Ptr(armstorage.MinimumTLSVersionTLS12), | ||
| AllowBlobPublicAccess: to.Ptr(config.AllowBlobPublicAccess), | ||
| AccessTier: accessTier, | ||
| // Add more properties as needed based on your requirements | ||
| } | ||
|
|
||
| l.Infof("Creating storage account %s in %s (Kind: %s, SKU: %s)", | ||
| c.storageAccountName, location, *parameters.Kind, *parameters.SKU.Name) | ||
|
|
||
| pollerResp, err := c.client.BeginCreate(ctx, c.resourceGroupName, c.storageAccountName, parameters, nil) | ||
| if err != nil { | ||
| return errors.Errorf("error creating storage account: %w", err) | ||
| } | ||
|
|
||
| _, err = pollerResp.PollUntilDone(ctx, nil) | ||
| if err != nil { | ||
| return errors.Errorf("error waiting for storage account creation: %w", err) | ||
| } | ||
|
|
||
| l.Infof("Successfully created storage account %s", c.storageAccountName) | ||
|
|
||
| // Assign Storage Blob Data Owner role to the current user | ||
| err = c.AssignStorageBlobDataOwnerRole(ctx, l) | ||
| if err != nil { | ||
| l.Warnf("Failed to assign Storage Blob Data Owner role: %v", err) | ||
| } | ||
|
|
||
| // If versioning is enabled, enable it on the storage account | ||
| if config.EnableVersioning { | ||
| err = c.EnableStorageAccountVersioning(ctx, l) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function complexity is too high - consider decomposition.
The createStorageAccount function is 150+ lines and handles multiple concerns: SKU mapping, kind mapping, parameter creation, and post-creation setup. This affects maintainability and testability.
Consider breaking this into smaller functions:
mapStorageAccountSKU(config)mapAccountKind(config)buildCreateParameters(config)executeStorageAccountCreation(ctx, parameters)configurePostCreationSettings(ctx, config)
func (c *StorageAccountClient) createStorageAccount(ctx context.Context, l log.Logger, config StorageAccountConfig) error {
+ sku := c.mapStorageAccountSKU(config.ReplicationType)
+ kind := c.mapAccountKind(config.AccountKind)
+ parameters := c.buildCreateParameters(config, sku, kind)
+
+ if err := c.executeStorageAccountCreation(ctx, l, parameters); err != nil {
+ return err
+ }
+
+ return c.configurePostCreationSettings(ctx, l, config)
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In azurehelper/azure_storage_account.go from lines 290 to 439, the
createStorageAccount function is too large and handles multiple
responsibilities, making it hard to maintain and test. Refactor by extracting
distinct parts into smaller functions: one to map the SKU based on config,
another to map the account kind, a function to build the AccountCreateParameters
struct, one to execute the storage account creation call, and another to handle
post-creation settings like role assignment and versioning. Replace the
corresponding code blocks in createStorageAccount with calls to these new helper
functions to improve clarity and modularity.
…ng storage account in most test cases
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (11)
azurehelper/azure_storage_account.go (11)
138-142: Undefined constant causes compilation error.The constant
defaultRoleAssignmentAPIVersionis referenced but not defined, which will prevent the code from compiling.
175-177: Undefined constant causes compilation error.The constant
httpStatusNotFoundis not defined. Use the standard library constant instead.
264-266: Undefined constant causes compilation error.The constant
defaultLocationis not defined in this file.
290-439: Function complexity is too high - consider decomposition.The
createStorageAccountfunction is 150+ lines and handles multiple concerns, affecting maintainability and testability.
572-576: Undefined constant causes compilation error.The constant
httpStatusNotFoundis referenced but not defined.
654-657: Undefined constant causes compilation error.The constant
defaultHTTPClientTimeoutis not defined in this file.
679-683: Undefined constant causes compilation error.The constant
httpStatusOKis not defined. Usehttp.StatusOKfrom the standard library.
715-716: Undefined constant causes compilation error.The constant
storageBlobDataOwnerRoleIDis not defined in this file.
795-805: Multiple undefined HTTP status constants.The constants
httpStatusConflict,httpStatusForbidden, andhttpStatusUnauthorizedare not defined.
924-924: Undefined constants in permission error checking.The constants
httpStatusUnauthorizedandhttpStatusForbiddenare referenced but not defined.
981-984: MoveGenerateUUIDto a utility package.This generic UUID generation function doesn't belong in the storage account file. Consider moving it to a shared utility package.
🧹 Nitpick comments (10)
internal/remotestate/backend/azurerm/testing/azure_test_utils.go (3)
32-35: Remove unnecessary struct padding and comments.The padding field and alignment comments are not needed for this simple struct. Go's compiler handles struct alignment automatically, and explicit padding is rarely necessary.
type AzureTestConfig struct { - // Group string fields together StorageAccountName string ContainerName string Location string AccessKey string - // Put bool field at the end UseAzureAD bool - // Add padding to optimize struct size - _ [3]byte // padding to align to 8-byte boundary }
78-133: Extract magic numbers to constants.The retry logic uses hardcoded values (3 retries, 2 second delays) that should be defined as constants for better maintainability.
+const ( + maxRetries = 3 + retryDelay = 2 * time.Second +) + func CleanupAzureContainer(t *testing.T, config *AzureTestConfig) { // ... existing code ... - maxRetries := 3 + maxRetries := maxRetries // ... existing code ... - time.Sleep(2 * time.Second) + time.Sleep(retryDelay) // ... existing code ... }
271-271: Overly aggressive test name cleaning.Removing all instances of "test" from test names might remove meaningful parts of the test name, potentially causing naming conflicts.
- cleanName = strings.ReplaceAll(cleanName, "test", "") + // Only remove "test" prefix/suffix to preserve meaningful test names + cleanName = strings.TrimPrefix(cleanName, "test") + cleanName = strings.TrimSuffix(cleanName, "test")azurehelper/azure_storage_account.go (3)
104-107: Consider using the custom errors package consistently.While
fmt.Errorfis used here, the project uses a custom error package that provides stack traces. Consider usingerrors.Errorffor consistency.- return nil, fmt.Errorf("error getting azure credentials: %w", err) + return nil, errors.Errorf("error getting azure credentials: %w", err)
596-600: Inconsistent error wrapping pattern.This function uses
fmt.Errorfwhile most other functions in the file use the customerrors.Errorf. Be consistent with error handling patterns.- return fmt.Errorf("error creating resource group client: %w", err) + return errors.Errorf("error creating resource group client: %w", err)The same issue appears at lines 610-611 and 635-636.
1132-1142: Simplify access tier comparison logic.The comparison logic can be simplified using a more direct approach.
func CompareAccessTier(current *armstorage.AccessTier, desired string) bool { - if current == nil && desired == "" { - return true - } - - if current == nil || desired == "" { - return false - } - - return string(*current) == desired + if current == nil { + return desired == "" + } + return string(*current) == desired }test/integration_azure_test.go (4)
692-694: Document why parallel execution is disabled.This test doesn't call
t.Parallel()unlike other tests. If this is intentional due to resource conflicts, please add a comment explaining why.func TestStorageAccountBootstrap(t *testing.T) { - t.Parallel() + // Not running in parallel to avoid conflicts with storage account creation/deletion + // t.Parallel()
1801-1807: Add parallel execution or document why it's disabled.This test is missing
t.Parallel(). Either add it for consistency with other tests or document why parallel execution is avoided.func TestDynamicAzureStorage(t *testing.T) { + t.Parallel() + // Skip if running in a CI pipeline without proper setup
1-2321: Consider splitting this large test file.At 2321 lines, this file is quite large and could benefit from being split into multiple files for better maintainability:
integration_azure_backend_test.go- Backend-specific testsintegration_azure_storage_test.go- Storage account and blob testsintegration_azure_helpers_test.go- Test helpers and utilitiesintegration_azure_migration_test.go- Migration-specific tests
432-489: Simplify test case structure.The anonymous struct with
checkExpectedResultFninline makes the test harder to read. Consider extracting the verification logic into a named helper function.- testCases := []struct { - checkExpectedResultFn func(t *testing.T, output string, containerName string, rootPath string, name string, args string) - name string - args string - containerName string - }{ + type backendTestCase struct { + name string + args string + containerName string + } + + verifyBackendDelete := func(t *testing.T, output string, containerName string, rootPath string) { + // Move verification logic here + } + + testCases := []backendTestCase{
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
azurehelper/azure_storage_account.go(1 hunks)internal/remotestate/backend/azurerm/testing/azure_test_utils.go(1 hunks)test/integration_azure_test.go(1 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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
internal/remotestate/backend/azurerm/testing/azure_test_utils.goazurehelper/azure_storage_account.gotest/integration_azure_test.go
🧠 Learnings (3)
internal/remotestate/backend/azurerm/testing/azure_test_utils.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
azurehelper/azure_storage_account.go (1)
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
test/integration_azure_test.go (1)
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
🔇 Additional comments (3)
test/integration_azure_test.go (3)
1-3: LGTM!Build tag and package declaration are correct for Azure-specific integration tests.
32-65: Well-implemented experiment flag management.The helper functions properly handle environment variable restoration and command flag injection with good edge case handling.
1978-2143: Excellent test infrastructure implementation.The
setupDynamicAzureTestfunction provides comprehensive test environment setup with:
- Proper resource cleanup order (storage before resource group)
- Retry logic for cleanup operations
- Clear error messages and logging
- Appropriate timeouts
This is a well-designed test helper that ensures reliable test execution and cleanup.
internal/remotestate/backend/azurerm/testing/azure_test_utils.go
Outdated
Show resolved
Hide resolved
|
|
||
| // Only perform object ID lookup if Azure AD auth is enabled and MSI is NOT being used | ||
| useAzureAD := os.Getenv("TERRAGRUNT_AZURE_USE_AZUREAD_AUTH") == "true" || os.Getenv("USE_AZUREAD_AUTH") == "true" | ||
| usingMSI := os.Getenv("AZURE_CLIENT_ID") != "" || os.Getenv("ARM_CLIENT_ID") != "" |
There was a problem hiding this comment.
Incorrect MSI detection logic.
The check os.Getenv("AZURE_CLIENT_ID") != "" || os.Getenv("ARM_CLIENT_ID") != "" doesn't accurately detect MSI usage, as these variables are also set for service principal authentication.
To properly detect MSI, check for the absence of client secret:
-usingMSI := os.Getenv("AZURE_CLIENT_ID") != "" || os.Getenv("ARM_CLIENT_ID") != ""
+// MSI is used when client ID is set but no client secret is provided
+usingMSI := (os.Getenv("AZURE_CLIENT_ID") != "" || os.Getenv("ARM_CLIENT_ID") != "") &&
+ os.Getenv("AZURE_CLIENT_SECRET") == "" && os.Getenv("ARM_CLIENT_SECRET") == ""📝 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.
| usingMSI := os.Getenv("AZURE_CLIENT_ID") != "" || os.Getenv("ARM_CLIENT_ID") != "" | |
| // MSI is used when client ID is set but no client secret is provided | |
| usingMSI := (os.Getenv("AZURE_CLIENT_ID") != "" || os.Getenv("ARM_CLIENT_ID") != "") && | |
| os.Getenv("AZURE_CLIENT_SECRET") == "" && os.Getenv("ARM_CLIENT_SECRET") == "" |
🤖 Prompt for AI Agents
In test/integration_azure_test.go at line 339, the current MSI detection logic
incorrectly checks for environment variables that are also set for service
principal authentication. To fix this, change the detection to verify the
absence of a client secret environment variable instead, ensuring MSI is
properly identified by confirming that the client secret is not set.
| t.Parallel() | ||
|
|
||
| log := logger.CreateLogger() | ||
| ctx := t.Context() | ||
|
|
||
| // Use the GetAzureCredentials helper to check for credentials and subscription ID | ||
| _, subscriptionID := getAzureCredentialsHelper(ctx, t) | ||
| if subscriptionID == "" { | ||
| t.Skip("Skipping storage account configuration test: No subscription ID found in environment variables") | ||
| } | ||
|
|
||
| location := os.Getenv("AZURE_LOCATION") | ||
| if location == "" { | ||
| location = os.Getenv("ARM_LOCATION") | ||
| if location == "" { | ||
| location = "westeurope" | ||
| t.Logf("Neither AZURE_LOCATION nor ARM_LOCATION set, using default: %s", location) | ||
| } | ||
| } | ||
|
|
||
| // Test different storage account configurations | ||
| testCases := []struct { | ||
| name string | ||
| expectUpdate bool | ||
| expectWarnings bool | ||
| accessTierChange bool | ||
| replicationChange bool | ||
| publicAccessChange bool | ||
| tagsChange bool | ||
| }{ | ||
| { | ||
| name: "UpdateAccessTier", | ||
| expectUpdate: true, | ||
| expectWarnings: false, | ||
| accessTierChange: true, | ||
| }, | ||
| { | ||
| name: "UpdateBlobPublicAccess", | ||
| expectUpdate: true, | ||
| expectWarnings: false, | ||
| publicAccessChange: true, | ||
| }, | ||
| { | ||
| name: "UpdateTags", | ||
| expectUpdate: true, | ||
| expectWarnings: false, | ||
| tagsChange: true, | ||
| }, | ||
| { | ||
| name: "ReadOnlyPropertyWarning", | ||
| expectUpdate: false, | ||
| expectWarnings: true, | ||
| replicationChange: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| tc := tc | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| // Remove t.Parallel() here to prevent resource conflicts between subtests | ||
| // Each subtest will have completely unique resources | ||
|
|
There was a problem hiding this comment.
Inconsistent parallelization strategy.
The parent test runs in parallel (line 1411) but the comment on line 1471 indicates subtests shouldn't run in parallel. This could cause resource conflicts if multiple parent test instances run concurrently.
Either remove t.Parallel() from the parent test or ensure subtests can handle concurrent execution:
func TestStorageAccountConfigurationAndUpdate(t *testing.T) {
- t.Parallel()
+ // Not running in parallel because subtests share resources
+ // t.Parallel()📝 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.
| t.Parallel() | |
| log := logger.CreateLogger() | |
| ctx := t.Context() | |
| // Use the GetAzureCredentials helper to check for credentials and subscription ID | |
| _, subscriptionID := getAzureCredentialsHelper(ctx, t) | |
| if subscriptionID == "" { | |
| t.Skip("Skipping storage account configuration test: No subscription ID found in environment variables") | |
| } | |
| location := os.Getenv("AZURE_LOCATION") | |
| if location == "" { | |
| location = os.Getenv("ARM_LOCATION") | |
| if location == "" { | |
| location = "westeurope" | |
| t.Logf("Neither AZURE_LOCATION nor ARM_LOCATION set, using default: %s", location) | |
| } | |
| } | |
| // Test different storage account configurations | |
| testCases := []struct { | |
| name string | |
| expectUpdate bool | |
| expectWarnings bool | |
| accessTierChange bool | |
| replicationChange bool | |
| publicAccessChange bool | |
| tagsChange bool | |
| }{ | |
| { | |
| name: "UpdateAccessTier", | |
| expectUpdate: true, | |
| expectWarnings: false, | |
| accessTierChange: true, | |
| }, | |
| { | |
| name: "UpdateBlobPublicAccess", | |
| expectUpdate: true, | |
| expectWarnings: false, | |
| publicAccessChange: true, | |
| }, | |
| { | |
| name: "UpdateTags", | |
| expectUpdate: true, | |
| expectWarnings: false, | |
| tagsChange: true, | |
| }, | |
| { | |
| name: "ReadOnlyPropertyWarning", | |
| expectUpdate: false, | |
| expectWarnings: true, | |
| replicationChange: true, | |
| }, | |
| } | |
| for _, tc := range testCases { | |
| tc := tc | |
| t.Run(tc.name, func(t *testing.T) { | |
| // Remove t.Parallel() here to prevent resource conflicts between subtests | |
| // Each subtest will have completely unique resources | |
| func TestStorageAccountConfigurationAndUpdate(t *testing.T) { | |
| + // Not running in parallel because subtests share resources | |
| + // t.Parallel() | |
| - t.Parallel() | |
| log := logger.CreateLogger() | |
| ctx := t.Context() | |
| // Use the GetAzureCredentials helper to check for credentials and subscription ID | |
| _, subscriptionID := getAzureCredentialsHelper(ctx, t) | |
| if subscriptionID == "" { | |
| t.Skip("Skipping storage account configuration test: No subscription ID found in environment variables") | |
| } | |
| location := os.Getenv("AZURE_LOCATION") | |
| if location == "" { | |
| location = os.Getenv("ARM_LOCATION") | |
| if location == "" { | |
| location = "westeurope" | |
| t.Logf("Neither AZURE_LOCATION nor ARM_LOCATION set, using default: %s", location) | |
| } | |
| } | |
| // Test different storage account configurations | |
| testCases := []struct { | |
| name string | |
| expectUpdate bool | |
| expectWarnings bool | |
| accessTierChange bool | |
| replicationChange bool | |
| publicAccessChange bool | |
| tagsChange bool | |
| }{ | |
| { | |
| name: "UpdateAccessTier", | |
| expectUpdate: true, | |
| expectWarnings: false, | |
| accessTierChange: true, | |
| }, | |
| { | |
| name: "UpdateBlobPublicAccess", | |
| expectUpdate: true, | |
| expectWarnings: false, | |
| publicAccessChange: true, | |
| }, | |
| { | |
| name: "UpdateTags", | |
| expectUpdate: true, | |
| expectWarnings: false, | |
| tagsChange: true, | |
| }, | |
| { | |
| name: "ReadOnlyPropertyWarning", | |
| expectUpdate: false, | |
| expectWarnings: true, | |
| replicationChange: true, | |
| }, | |
| } | |
| for _, tc := range testCases { | |
| tc := tc | |
| t.Run(tc.name, func(t *testing.T) { | |
| // Remove t.Parallel() here to prevent resource conflicts between subtests | |
| // Each subtest will have completely unique resources |
🤖 Prompt for AI Agents
In test/integration_azure_test.go around lines 1411 to 1472, the parent test is
marked to run in parallel with t.Parallel(), but the subtests are not
parallelized due to resource conflict concerns. To fix this inconsistency,
remove the t.Parallel() call from the parent test to prevent concurrent
execution of multiple parent test instances, ensuring resource conflicts do not
occur during subtest runs.
There was a problem hiding this comment.
Actionable comments posted: 36
♻️ Duplicate comments (2)
internal/remotestate/backend/azurerm/backend.go (2)
270-474: Function complexity needs to be addressed.The Bootstrap function spans over 200 lines and handles multiple responsibilities, making it difficult to maintain and test.
1130-1266: Method complexity is too high.The bootstrapStorageAccount method handles multiple concerns across 130+ lines, making it difficult to test and maintain.
🧹 Nitpick comments (18)
internal/azure/implementations/production_test.go (1)
10-23: Consider expanding test coverage beyond basic instantiation.While the test correctly verifies that the container is not nil, it could be more comprehensive by testing that the container implements the expected interface and that the configuration is properly stored.
Consider adding interface validation:
func TestProductionServiceContainerCreation(t *testing.T) { t.Parallel() config := map[string]interface{}{ "subscriptionId": "test-subscription", } container := implementations.NewProductionServiceContainer(config) // Test that container is not nil and implements the interface if container == nil { t.Fatal("Expected non-nil container") } + + // Verify it implements the expected interface + _, ok := container.(interfaces.AzureServiceContainer) + if !ok { + t.Fatal("Expected container to implement AzureServiceContainer interface") + } }internal/azure/azureutil/types.go (1)
26-42: Consider adding validation for ErrorMetrics struct.The
ErrorMetricsstruct has many fields that could benefit from validation to ensure data quality for telemetry.Consider adding a validation method:
// Validate checks if the ErrorMetrics contains required fields func (em *ErrorMetrics) Validate() error { if em.ErrorType == "" { return fmt.Errorf("ErrorType is required") } if em.Operation == "" { return fmt.Errorf("Operation is required") } return nil }internal/azure/azurehelper/azure_blob_service_test.go (2)
118-121: Inconsistent helper function usage.The code uses
azurehelper.StringPtrfrom line 120, but later defines a localstrPtrfunction (line 310). This creates inconsistency and potential confusion.-Container: azurehelper.StringPtr("invalid/container/name"), -Key: azurehelper.StringPtr("test.txt"), +Container: strPtr("invalid/container/name"), +Key: strPtr("test.txt"),Or use the helper from
azurehelperpackage consistently throughout the file.
308-309: Remove duplicate comment.-// Helper function to create string pointers - -// Helper function to create string pointers +// Helper function to create string pointersinternal/azure/backend_di.go (1)
52-55: Potential configuration conflict with "options" key.Adding the "options" key to the blob service config might conflict with existing configuration if the original config already contains this key.
Consider using a more specific key name or checking for conflicts:
-// Include opts in blob service config -blobConfig := mergeConfig(config, map[string]interface{}{ - "options": opts, -}) +// Include opts in blob service config +blobConfig := mergeConfig(config, map[string]interface{}{ + "terragrunt_options": opts, +})internal/azure/CONFIGURATION.md (1)
88-88: Fix grammatical redundancy.The static analysis tool correctly identified repeated use of "only" in the sentence.
Apply this diff to fix the redundancy:
-| `enable_https_traffic_only` | `true` | Allow only HTTPS traffic | +| `enable_https_traffic_only` | `true` | Allow HTTPS traffic only |internal/azure/azureutil/error_types.go (1)
35-44: Enhance error formatting to include operation and resource context.The current
Error()method only includes the message, wrapped error, and suggestion. Consider including operation and resource context for better debugging.Apply this diff to enhance error formatting:
// Error implements the error interface func (e *AzureError) Error() string { base := e.Message + if e.Operation != "" && e.ResourceType != "" && e.ResourceName != "" { + base = fmt.Sprintf("[%s %s/%s] %s", e.Operation, e.ResourceType, e.ResourceName, base) + } else if e.Operation != "" { + base = fmt.Sprintf("[%s] %s", e.Operation, base) + } if e.Wrapped != nil { base = fmt.Sprintf("%s: %v", base, e.Wrapped) } if e.Suggestion != "" { base = fmt.Sprintf("%s\nSuggestion: %s", base, e.Suggestion) } return base }internal/azure/interfaces/rbac.go (1)
79-81: Consider using the existing IsPermissionError implementation.The interface includes an
IsPermissionErrormethod, but there's already an implementation ininternal/azure/errors/types.go. Having multiple implementations of permission error detection could lead to inconsistency.Consider either:
- Remove this method from the interface and use the existing utility function
- Have the implementation delegate to the existing function for consistency
- Document why a separate implementation is needed if there are RBAC-specific requirements
internal/azure/azurehelper/azure_resource_group_test.go (3)
11-11: Remove the commented-out import.The commented import is not being used and should be removed to keep the code clean.
- // "github.com/gruntwork-io/terragrunt/internal/errors"
108-108: Remove outdated "capture range variable" pattern.Starting from Go 1.22, loop variables are captured correctly by default. The
tc := tcpattern is no longer necessary.Remove all instances of the capture pattern. For example:
- tc := tc // capture range variable t.Run(tc.name, func(t *testing.T) {Also applies to: 180-180, 243-243, 305-305, 413-413, 479-479
506-518: Consider implementing or removing the placeholder test functions.The commented-out test functions appear to be placeholders for future mock-based testing. Consider either implementing them with proper mocks or removing them if they're not needed.
Would you like me to help implement these test functions using mock Azure SDK clients, or should we remove them for now?
internal/azure/azureauth/auth.go (1)
569-577: Clarify the unusual default value pattern in getFirstEnvValue.The function uses the last element of the keys slice as a default value, which is unconventional and could be confusing. Consider making this more explicit.
-// Helper function to get the first non-empty value from environment variables -func getFirstEnvValue(keys ...string) string { - for _, key := range keys[:len(keys)-1] { // Last element is default value +// Helper function to get the first non-empty value from environment variables +// The last argument is the default value to return if no environment variables are set +func getFirstEnvValue(keys ...string) string { + if len(keys) == 0 { + return "" + } + // Check all keys except the last one (which is the default value) + for i := 0; i < len(keys)-1; i++ { + key := keys[i] if val := os.Getenv(key); val != "" { return val } } return keys[len(keys)-1] // Default value }internal/azure/azureutil/errorhandling_test.go (1)
234-234: Remove outdated "capture range variable" pattern.Similar to the previous file, the
tc := tcpattern is no longer needed in Go 1.22+.- tc := tc t.Run(tc.name, func(t *testing.T) {Also applies to: 279-279, 336-336, 364-364, 426-426, 466-466
internal/azure/errors/types.go (1)
171-209: Consider refining error classification logic to avoid overlaps.The current classification logic might have overlapping cases. For example:
- "forbidden" is classified as
ErrorClassPermission(line 184-186)- "unauthorized" is classified as
ErrorClassAuthentication(line 181-183)However, in practice, these could overlap. Consider documenting the precedence or using a more specific classification approach.
Add documentation to clarify the classification precedence:
// ClassifyError determines the error classification from an error +// The function checks error messages in a specific order, with earlier checks +// taking precedence over later ones. For example, "unauthorized" will be +// classified as Authentication rather than Permission, even though both could apply. func ClassifyError(err error) ErrorClass {internal/azure/azurehelper/azure_blob.go (1)
71-197: Consider refactoring this large function for better maintainability.The
CreateBlobServiceClientfunction is doing multiple things: parameter validation, storage account verification, authentication, client creation, and connectivity testing. Consider breaking it down into smaller functions likevalidateConfig,verifyStorageAccount,createAuthenticatedClient, andtestConnectivity.internal/azure/factory/adapters.go (1)
204-209: Consider memory usage for large blobs.Reading the entire blob content into memory with
io.ReadAllcould cause out-of-memory issues for large blobs.Consider implementing a streaming approach or adding a size limit check:
// Check blob size first const maxBlobSize = 100 * 1024 * 1024 // 100MB // Add size validation before readingOr return an io.Reader interface to allow streaming processing by the caller.
internal/azure/factory/factory.go (1)
130-134: Implement actual health checks.The
Healthmethod just returns nil without checking service health.Consider implementing actual health checks by verifying connectivity to Azure services. Would you like me to generate a health check implementation?
internal/azure/azurehelper/azure_storage_account.go (1)
827-831: Clarify the comment about error handling.The comment is contradictory - it says "handle the error properly by ignoring it". While ignoring Close() errors in defer is standard practice, the comment should be clearer.
- // Simply handle the error properly by ignoring it in defer - // This is sufficient to satisfy the errcheck linter + // Explicitly ignore Close() error in defer as per Go best practices + // Close() errors are typically not actionable in this context
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
config/dependency.go(4 hunks)config/dependency_test.go(2 hunks)internal/azure/CONFIGURATION.md(1 hunks)internal/azure/README.md(1 hunks)internal/azure/azureauth/auth.go(1 hunks)internal/azure/azureauth/auth_test.go(1 hunks)internal/azure/azurehelper/azure_auth_test.go(1 hunks)internal/azure/azurehelper/azure_blob.go(1 hunks)internal/azure/azurehelper/azure_blob_service_test.go(1 hunks)internal/azure/azurehelper/azure_blob_test.go(1 hunks)internal/azure/azurehelper/azure_constants.go(1 hunks)internal/azure/azurehelper/azure_rbac_constants_test.go(1 hunks)internal/azure/azurehelper/azure_resource_group.go(1 hunks)internal/azure/azurehelper/azure_resource_group_test.go(1 hunks)internal/azure/azurehelper/azure_storage_account.go(1 hunks)internal/azure/azurehelper/azure_storage_account_test.go(1 hunks)internal/azure/azurehelper/storage_account_adapter.go(1 hunks)internal/azure/azurehelper/test_helpers.go(1 hunks)internal/azure/azurehelper/utility_functions_test.go(1 hunks)internal/azure/azureutil/error_types.go(1 hunks)internal/azure/azureutil/errorhandling.go(1 hunks)internal/azure/azureutil/errorhandling_test.go(1 hunks)internal/azure/azureutil/errors.go(1 hunks)internal/azure/azureutil/types.go(1 hunks)internal/azure/azureutil/types_test.go(1 hunks)internal/azure/backend_di.go(1 hunks)internal/azure/errors/types.go(1 hunks)internal/azure/errors/types_test.go(1 hunks)internal/azure/errors/wrappers.go(1 hunks)internal/azure/factory/adapters.go(1 hunks)internal/azure/factory/factory.go(1 hunks)internal/azure/factory/factory_test.go(1 hunks)internal/azure/implementations/production.go(1 hunks)internal/azure/implementations/production_test.go(1 hunks)internal/azure/interfaces/auth.go(1 hunks)internal/azure/interfaces/container.go(1 hunks)internal/azure/interfaces/factory.go(1 hunks)internal/azure/interfaces/interfaces_test.go(1 hunks)internal/azure/interfaces/rbac.go(1 hunks)internal/azure/interfaces/storage.go(1 hunks)internal/azure/mocks/mock_services.go(1 hunks)internal/azure/types/blob_types.go(1 hunks)internal/azure/types/storage_account.go(1 hunks)internal/azure/types/storage_types.go(1 hunks)internal/azure/types/storage_types_test.go(1 hunks)internal/azure/types/types_test.go(1 hunks)internal/remotestate/backend/azurerm/backend.go(1 hunks)internal/remotestate/backend/azurerm/backend_di.go(1 hunks)internal/remotestate/backend/azurerm/backend_factory_test.go(1 hunks)internal/remotestate/backend/azurerm/backend_interface_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- internal/azure/mocks/mock_services.go
- internal/azure/azurehelper/storage_account_adapter.go
- internal/azure/azurehelper/test_helpers.go
- internal/azure/types/blob_types.go
- internal/azure/azurehelper/azure_constants.go
- internal/azure/types/types_test.go
- internal/azure/types/storage_types.go
- internal/azure/types/storage_account.go
- internal/azure/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- config/dependency.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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
internal/azure/implementations/production_test.gointernal/azure/azurehelper/azure_blob_test.gointernal/azure/azureauth/auth_test.gointernal/azure/azurehelper/azure_rbac_constants_test.gointernal/remotestate/backend/azurerm/backend_di.gointernal/azure/interfaces/factory.goconfig/dependency_test.gointernal/azure/factory/factory_test.gointernal/azure/backend_di.gointernal/azure/errors/types_test.gointernal/azure/azureutil/types_test.gointernal/azure/azureutil/types.gointernal/azure/azurehelper/azure_blob_service_test.gointernal/azure/types/storage_types_test.gointernal/azure/errors/wrappers.gointernal/azure/interfaces/interfaces_test.gointernal/azure/azureutil/errors.gointernal/azure/interfaces/container.gointernal/azure/azurehelper/azure_auth_test.gointernal/azure/azureutil/error_types.gointernal/remotestate/backend/azurerm/backend_factory_test.gointernal/azure/azurehelper/azure_resource_group_test.gointernal/azure/azurehelper/azure_resource_group.gointernal/azure/interfaces/auth.gointernal/azure/azureauth/auth.gointernal/azure/azureutil/errorhandling_test.gointernal/azure/errors/types.gointernal/azure/factory/adapters.gointernal/azure/azurehelper/utility_functions_test.gointernal/azure/interfaces/rbac.gointernal/azure/azureutil/errorhandling.gointernal/azure/azurehelper/azure_blob.gointernal/azure/azurehelper/azure_storage_account_test.gointernal/remotestate/backend/azurerm/backend_interface_test.gointernal/azure/factory/factory.gointernal/azure/interfaces/storage.gointernal/remotestate/backend/azurerm/backend.gointernal/azure/azurehelper/azure_storage_account.gointernal/azure/implementations/production.go
🧬 Code Graph Analysis (16)
internal/azure/implementations/production_test.go (1)
internal/azure/implementations/production.go (1)
NewProductionServiceContainer(735-743)
internal/azure/azurehelper/azure_blob_test.go (2)
internal/azure/azurehelper/azure_blob.go (2)
AzureResponseError(41-45)GetObjectInput(26-29)internal/azure/azurehelper/test_helpers.go (1)
StringPtr(7-9)
internal/azure/interfaces/factory.go (3)
internal/azure/interfaces/container.go (1)
AzureServiceContainer(27-118)internal/azure/factory/factory.go (1)
Options(17-22)internal/remotestate/backend/azurerm/retry.go (1)
RetryConfig(67-108)
internal/azure/factory/factory_test.go (4)
internal/azure/factory/factory.go (2)
NewAzureServiceFactory(50-60)Options(17-22)internal/azure/interfaces/container.go (2)
AzureServiceContainer(27-118)ServiceContainerConfig(122-156)internal/azure/interfaces/factory.go (1)
RetryConfig(32-44)internal/azure/types/storage_types.go (5)
StorageAccountConfig(12-23)AccountKind(5-5)AccountTier(6-6)AccessTier(7-7)ReplicationType(8-8)
internal/azure/backend_di.go (4)
internal/azure/interfaces/storage.go (3)
StorageAccountService(38-46)BlobService(49-62)ResourceGroupService(65-73)internal/remotestate/backend/azurerm/backend_di.go (2)
NewAzureBackendDependencies(21-50)AzureBackendDependencies(14-18)options/options.go (1)
TerragruntOptions(120-346)internal/azure/implementations/production.go (1)
NewProductionServiceContainer(735-743)
internal/azure/azureutil/types.go (5)
internal/remotestate/backend/azurerm/telemetry.go (2)
ClassifyError(179-284)ErrorClassUnknown(109-109)internal/azure/azureutil/error_types.go (10)
ErrorClass(8-8)ErrorClassUnknown(20-20)ErrorClassAuthorization(12-12)ErrorClassPermission(16-16)ErrorClassInvalidRequest(13-13)ErrorClassResource(17-17)ErrorClassNetworking(14-14)ErrorClassNotFound(15-15)ErrorClassThrottling(19-19)ErrorClassSystem(18-18)internal/azure/errors/types.go (11)
ErrorClass(10-10)ClassifyError(172-209)ErrorClassUnknown(25-25)ErrorClassAuthorization(15-15)ErrorClassPermission(20-20)ErrorClassInvalidRequest(17-17)ErrorClassResource(21-21)ErrorClassNetworking(18-18)ErrorClassNotFound(19-19)ErrorClassThrottling(23-23)ErrorClassSystem(22-22)cli/commands/run/run.go (1)
IsRetryable(530-536)internal/azure/azureauth/auth.go (1)
AuthMethod(20-20)
internal/azure/azurehelper/azure_blob_service_test.go (4)
internal/azure/azurehelper/azure_blob.go (2)
GetObjectInput(26-29)GetObjectOutput(32-34)internal/azure/types/blob_types.go (2)
GetObjectInput(4-8)GetObjectOutput(11-14)internal/azure/azurehelper/test_helpers.go (1)
StringPtr(7-9)internal/azure/types/types_test.go (1)
TestGetObjectOutput(203-221)
internal/azure/interfaces/container.go (3)
internal/azure/interfaces/storage.go (3)
StorageAccountService(38-46)BlobService(49-62)ResourceGroupService(65-73)internal/azure/interfaces/rbac.go (1)
RBACService(68-81)internal/azure/interfaces/auth.go (1)
AuthenticationService(42-138)
internal/azure/azureutil/error_types.go (4)
internal/azure/errors/types.go (12)
ErrorClass(10-10)ResourceType(29-29)AzureError(40-48)WithError(88-92)ErrorOption(85-85)WithSuggestion(95-99)WithClassification(102-106)WithResourceType(109-113)WithResourceName(116-120)WithOperation(123-127)ErrorClassUnknown(25-25)ErrorClassPermission(20-20)pkg/log/format/placeholders/message.go (1)
Message(20-28)internal/errors/export.go (1)
Unwrap(24-26)internal/remotestate/backend/azurerm/telemetry.go (1)
ErrorClassUnknown(109-109)
internal/azure/azurehelper/azure_resource_group.go (4)
internal/azure/azureauth/auth.go (2)
GetAuthConfig(333-405)GetTokenCredential(418-482)pkg/log/log.go (1)
Infof(82-84)internal/errors/export.go (1)
As(7-9)internal/remotestate/backend/azurerm/errors.go (1)
IsNotFoundError(290-302)
internal/azure/interfaces/auth.go (3)
internal/azure/azureutil/errors.go (1)
IsPermissionError(17-32)internal/azure/errors/types.go (1)
IsPermissionError(154-169)internal/remotestate/backend/azurerm/errors.go (1)
IsPermissionError(270-287)
internal/azure/azureauth/auth.go (3)
internal/remotestate/backend/azurerm/config.go (1)
Config(369-369)pkg/log/log.go (1)
Debugf(72-74)internal/errors/export.go (1)
As(7-9)
internal/azure/factory/adapters.go (8)
internal/azure/types/storage_types.go (5)
StorageAccountConfig(12-23)AccountKind(5-5)AccountTier(6-6)AccessTier(7-7)ReplicationType(8-8)pkg/log/log.go (1)
Default(12-14)internal/azure/types/storage_account.go (2)
StorageAccount(4-9)StorageAccountProperties(12-23)internal/azure/azureutil/errors.go (1)
IsPermissionError(17-32)internal/azure/azurehelper/azure_blob.go (3)
BlobServiceClient(20-23)GetObjectInput(26-29)GetObjectOutput(32-34)internal/azure/types/blob_types.go (2)
GetObjectInput(4-8)GetObjectOutput(11-14)internal/azure/interfaces/storage.go (1)
BlobService(49-62)internal/azure/azurehelper/azure_resource_group.go (1)
ResourceGroupClient(18-21)
internal/azure/azurehelper/utility_functions_test.go (4)
internal/azure/azurehelper/azure_blob.go (2)
AzureResponseError(41-45)ConvertAzureError(48-63)internal/azure/azurehelper/azure_storage_account.go (7)
GetStorageAccountSKU(1168-1187)CompareStringMaps(1213-1225)ConvertToPointerMap(1228-1237)CompareAccessTier(1240-1250)DefaultStorageAccountConfig(217-227)GenerateUUID(1135-1137)StorageAccountConfig(109-214)internal/azure/azurehelper/azure_constants.go (3)
AccessTierHot(28-28)AccessTierCool(29-29)AccessTierPremium(30-30)internal/azure/azurehelper/azure_resource_group.go (2)
ResourceGroupConfig(86-117)IsNotFoundError(278-285)
internal/azure/interfaces/rbac.go (1)
internal/azure/errors/types.go (1)
IsPermissionError(154-169)
internal/azure/interfaces/storage.go (5)
internal/azure/types/storage_types.go (5)
AccountKind(5-5)AccountTier(6-6)AccessTier(7-7)ReplicationType(8-8)StorageAccountConfig(12-23)internal/azure/azurehelper/azure_storage_account.go (1)
StorageAccountConfig(109-214)internal/azure/types/storage_account.go (2)
StorageAccount(4-9)StorageAccountProperties(12-23)internal/azure/azurehelper/azure_blob.go (2)
GetObjectInput(26-29)GetObjectOutput(32-34)internal/azure/types/blob_types.go (2)
GetObjectInput(4-8)GetObjectOutput(11-14)
🪛 LanguageTool
internal/azure/CONFIGURATION.md
[style] ~88-~88: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...le_https_traffic_only|true` | Allow only HTTPS traffic | ## Authentication Meth...
(ADVERB_REPETITION_PREMIUM)
🔇 Additional comments (63)
internal/azure/implementations/production_test.go (1)
25-35: LGTM - Nil config handling test is appropriate.The test correctly verifies that the function handles nil configuration gracefully, which aligns with the implementation that creates an empty map when config is nil.
internal/azure/azurehelper/azure_blob_test.go (2)
14-69: LGTM - Comprehensive error formatting tests.The test cases thoroughly cover various Azure error scenarios and the struct field ordering comments show good attention to memory alignment optimization.
155-166: LGTM - Simple mock implementation.The mock struct is appropriately simple and follows good naming conventions with proper field ordering comments.
internal/azure/interfaces/factory.go (2)
7-14: LGTM - Well-designed factory interface.The interface follows good design patterns for dependency injection and service creation with proper context handling.
16-29: LGTM - Comprehensive factory options.The
FactoryOptionsstruct provides good flexibility for configuration, mocking, and retry policies.internal/azure/azurehelper/azure_rbac_constants_test.go (2)
15-51: LGTM - Comprehensive RBAC constants validation.The test thoroughly validates the RBAC retry constants and their relationships, including edge cases. The additional test cases for the retry calculation logic demonstrate good test coverage.
53-152: LGTM - Thorough permission error detection tests.The test comprehensively covers various permission error scenarios including wrapped errors. The test cases are well-structured and cover both positive and negative cases for permission error detection.
internal/azure/azureauth/auth_test.go (3)
15-132: LGTM - Comprehensive authentication configuration tests.The test thoroughly covers all authentication methods with both direct configuration and environment variable scenarios. The use of
t.Setenvfor environment variable testing is appropriate and the test structure is well-organized.
134-190: LGTM - Good validation testing coverage.The test covers both valid and invalid authentication configurations with clear error expectations.
192-219: LGTM - Appropriate URL generation tests.The test covers both default and custom endpoint scenarios for Azure Storage URL generation.
config/dependency_test.go (1)
164-302: Excellent implementation of Azure direct state access testing.The test implementation demonstrates several best practices:
- Proper environment-based test skipping
- Unique resource naming to avoid conflicts
- Comprehensive error handling and cleanup
- Context propagation (addressing the past review comment)
- Realistic test data with proper JSON structure
The helper function
getTerragruntOutputJSONFromRemoteStateAzurermcorrectly accepts a context parameter and handles all error scenarios appropriately.internal/azure/types/storage_types_test.go (1)
1-310: Comprehensive test coverage for Azure storage types.The test file demonstrates excellent testing practices:
- Table-driven tests with parallel execution
- Comprehensive coverage of validation scenarios
- Both positive and negative test cases
- Edge case handling (empty values, partial configurations)
- Clear test naming and structure
The validation logic properly checks required fields and the endpoint detection accurately identifies empty vs. populated endpoints.
internal/azure/interfaces/interfaces_test.go (1)
1-407: Comprehensive interface testing with excellent coverage.The test file provides thorough coverage of:
- Struct field assignments and validations
- Error constants and their proper behavior
- Default value verification
- Struct equality and comparison logic
- ResourceNotFoundError with various scenarios
- String constant validation
- Basic interface usage patterns
The tests are well-structured with parallel execution and clear naming conventions.
internal/azure/azureutil/types.go (1)
73-108: Well-structured error classification logic.The error classification function demonstrates good practices:
- Logical ordering from most specific to general
- Case-insensitive string matching
- Comprehensive error pattern coverage
- Clear comments explaining the classification priority
The helper functions
containsAnyandcontainsprovide clean abstraction for string operations.internal/azure/errors/types_test.go (1)
1-373: Excellent test coverage and structure.This test file demonstrates excellent testing practices:
- Comprehensive coverage of error types and constants
- Well-structured table-driven tests with parallel execution
- Proper testing of error chaining and unwrapping behavior
- Good use of testify assertions for clear test validation
The tests cover all critical aspects of the Azure error handling system including classification, resource information, suggestions, and formatting.
internal/azure/backend_di.go (1)
77-105: Helpful documentation with usage examples.The commented examples provide clear guidance on how to refactor existing backend code to use dependency injection. This is excellent documentation that will help with implementation.
internal/azure/factory/factory_test.go (1)
1-300: Well-structured factory tests with comprehensive coverage.The test file demonstrates good testing practices with:
- Table-driven tests with parallel execution
- Comprehensive coverage of factory options, configuration, and context handling
- Good use of testify assertions and require statements
- Proper testing of boundary conditions in retry configuration
internal/azure/errors/wrappers.go (6)
7-31: LGTM: Well-structured error wrapper with consistent pattern.The
WrapBlobErrorfunction follows a clean pattern: nil-check, permission error detection, then generic error handling. The error enrichment with context, suggestions, and resource metadata is comprehensive.
33-57: LGTM: Consistent error wrapping pattern maintained.The
WrapResourceGroupErrorfunction maintains the same high-quality pattern as the blob wrapper, providing appropriate suggestions for RBAC permissions.
59-83: LGTM: Storage account error wrapper is well-implemented.The function properly handles storage account-specific permissions, suggesting both Storage Account Contributor and Storage Blob Data Owner roles as needed.
85-109: LGTM: Container error wrapper follows established pattern.Consistent implementation with appropriate permission suggestions for container operations.
111-120: LGTM: Specialized error wrapper for not-found scenarios.The
WrapContainerDoesNotExistErrorfunction correctly handles the specific case of missing containers by always returning a not-found classification, which is appropriate for this scenario.
122-130: LGTM: Authentication error wrapper is well-designed.The function provides helpful context about the authentication method and includes appropriate suggestions for credential verification.
internal/azure/azureutil/types_test.go (7)
11-81: LGTM: Comprehensive operation type testing.The test thoroughly covers all operation types with proper parallel execution and clear test structure. The table-driven approach is well-implemented.
83-97: LGTM: Error class constants are properly validated.All error class constants are tested to ensure they have the expected string values.
99-249: LGTM: Excellent error classification test coverage.The tests cover a comprehensive range of error scenarios including:
- Different error types (auth, permission, resource, networking, etc.)
- Case-insensitive matching
- Priority handling when multiple keywords are present
- Nil and edge cases
The test cases are well-structured and provide thorough validation of the error classification logic.
251-284: LGTM: Complete ErrorMetrics struct validation.The test verifies all fields of the ErrorMetrics struct are properly assigned and accessible, including complex types like maps.
286-307: LGTM: Default values are properly tested.The test ensures that struct fields have appropriate zero values when not explicitly set.
309-346: LGTM: Field assignment validation is thorough.The test verifies that all struct fields can be properly assigned and retrieved, ensuring the struct is fully functional.
348-398: LGTM: Edge case testing is comprehensive.The additional edge case tests cover important scenarios like empty messages, whitespace handling, partial matches, and keyword priority. This ensures robust error classification behavior.
internal/azure/CONFIGURATION.md (1)
1-696: LGTM: Comprehensive and well-structured Azure configuration documentation.This documentation provides excellent coverage of:
- Quick start examples with practical configurations
- Complete authentication method explanations
- Detailed configuration options with clear descriptions
- Advanced features like versioning and snapshots
- Best practices for security, performance, and operations
- Troubleshooting guidance
- Migration instructions
The documentation is well-organized with clear sections and helpful examples. It will be valuable for users implementing Azure backend support.
internal/remotestate/backend/azurerm/backend_factory_test.go (4)
13-73: LGTM: Comprehensive backend creation testing.The test covers various configuration scenarios including nil config, empty config, and custom configurations. The verification of enhanced factory creation is thorough.
75-106: LGTM: Remote state config integration is well-tested.The test verifies that the backend can be properly created from remote state configurations and that authentication configuration is extracted correctly.
108-149: LGTM: Runtime configuration updates are properly tested.The test validates that factory configurations can be updated at runtime and that the changes are properly applied and retrievable.
151-211: LGTM: Authentication method extraction is comprehensive.The test covers all supported authentication methods (Azure AD, MSI, Service Principal, SAS token, Access key) and includes proper fallback behavior to Azure AD as default.
internal/azure/azurehelper/azure_auth_test.go (7)
1-2: LGTM: Proper build constraint for Azure-specific tests.The
//go:build azureconstraint ensures these tests only run when Azure features are enabled, which is appropriate for this authentication testing.
17-39: LGTM: Environment variable testing is thorough.The test validates that environment variables are properly set and retrieved, including testing the fallback behavior when AZURE_ prefixed variables are cleared.
41-119: LGTM: Priority testing is comprehensive.The test covers all important priority scenarios:
- AZURE_ prefix taking precedence over ARM_
- ARM_ prefix as fallback
- Mixed prefix scenarios
- No credentials scenario
The inline credential resolution functions properly demonstrate the expected behavior.
121-233: LGTM: Credential priority testing with GetAzureCredentials function.The test validates the actual
GetAzureCredentialsfunction behavior with various environment variable combinations, ensuring proper subscription ID resolution.
235-323: LGTM: Comprehensive environment variable scenario testing.The test covers various authentication scenarios including Azure CLI, Service Principal, Managed Identity, and ARM variables, with proper priority handling.
325-405: LGTM: Excellent sensitive data redaction testing.The test ensures that sensitive information like client secrets, passwords, and account keys are properly redacted in logs. This is crucial for security and demonstrates good attention to protecting sensitive data.
407-410: LGTM: Proper mock logger implementation.The helper function creates a logger that discards output, which is appropriate for testing scenarios where log output is not needed.
internal/azure/interfaces/container.go (1)
1-169: Well-designed service container interface!The
AzureServiceContainerinterface is comprehensive and well-documented with:
- Clear separation of concerns (factories, lifecycle, registration, introspection)
- Excellent documentation with usage examples
- Flexible configuration options with sensible defaults
- Support for custom service registration enabling testability
The interface follows dependency injection best practices and will provide a solid foundation for the Azure backend implementation.
internal/azure/azurehelper/utility_functions_test.go (1)
1-865: Excellent test coverage and structure!The test file demonstrates high-quality testing practices:
- Comprehensive coverage of all utility functions
- Well-structured table-driven tests with descriptive test cases
- Proper use of parallel test execution
- Good coverage of edge cases (nil values, empty strings, special characters)
- Clear test naming and organization
The tests provide confidence in the utility functions' correctness and robustness.
internal/azure/azurehelper/azure_resource_group.go (1)
1-317: Well-implemented resource group client with comprehensive documentation!The implementation demonstrates excellent practices:
- Comprehensive documentation with clear examples
- Proper error handling and validation
- Good use of context and structured logging
- Clean separation of concerns
- Robust validation logic with proper error messages
Once the minor issues (subscription ID masking and missing constant) are addressed, this will provide a solid foundation for Azure resource group management.
internal/azure/azurehelper/azure_resource_group_test.go (1)
15-505: Well-structured and comprehensive test coverage!The test suite covers all key aspects of resource group operations including validation, naming constraints, client creation, tags handling, and location validation. The table-driven test approach is excellent for maintainability.
internal/azure/azureauth/auth.go (1)
1-596: Excellent authentication package implementation!The package provides comprehensive Azure authentication support with:
- Multiple authentication methods (Azure AD, MSI, Service Principal, CLI, SAS Token)
- Excellent documentation with detailed examples
- Proper environment variable support
- Clean separation of concerns
- Good error handling patterns
The code is well-structured and follows Go best practices.
internal/azure/azureutil/errorhandling_test.go (1)
1-511: Excellent test coverage for error handling utilities!The test suite provides comprehensive coverage including:
- Success and failure scenarios
- Error type detection and classification
- Retryable error detection
- Authentication error handling
- Edge cases and large error messages
- Context propagation
- Nil component handling
The mock implementation and test structure are well-designed.
internal/azure/interfaces/auth.go (1)
1-206: Well-designed authentication service interface!The interface provides a comprehensive abstraction for Azure authentication with:
- Clear method signatures and documentation
- Support for multiple authentication methods
- Proper error handling utilities
- Cloud environment support
- Configuration management
- Sensible defaults in AuthenticationConfig
This will greatly improve testability and maintainability of the Azure backend.
internal/azure/errors/types.go (1)
1-210: Excellent structured error handling implementation!The package provides:
- Well-structured error types with rich context
- Functional options pattern for flexible error construction
- Comprehensive error classification
- Proper error unwrapping support
- Clear error formatting with operation and resource context
This will greatly improve error diagnostics and telemetry for Azure operations.
internal/azure/azureutil/errorhandling.go (2)
57-59: Ignore missing definitions inerrorhandling.goThe
ErrorMetricstype andClassifyErrorfunction are defined in the sameinternal/azure/azureutilpackage, so no imports are needed and the code will compile as is.•
internal/azure/azureutil/types.go:27—type ErrorMetrics struct { … }
•internal/azure/azureutil/types.go:61—func ClassifyError(err error) ErrorClass { … }No action required here.
Likely an incorrect or invalid review comment.
238-238: containsAny is already defined in this package
ThecontainsAnyhelper is implemented ininternal/azure/azureutil/types.go(lines 46–53) and is accessible toerrorhandling.gowithout any additional imports. You can safely remove this comment about a missing function.Likely an incorrect or invalid review comment.
internal/azure/interfaces/storage.go (1)
1-96: Well-structured interface definitions.The interfaces are properly designed with context support, clear method signatures, and appropriate error handling. The custom
ResourceNotFoundErrortype is well-implemented with proper error interface compliance.internal/azure/azurehelper/azure_blob.go (1)
342-346: Good error handling pattern in defer statement.The defer statement properly handles both the close error and the original error, ensuring that errors are not silently ignored. This is a best practice for resource cleanup.
internal/azure/azurehelper/azure_storage_account_test.go (1)
1-1534: Excellent comprehensive test coverage.The test file provides thorough coverage including:
- Validation tests for all configuration fields
- Edge cases with special characters, Unicode, and boundary conditions
- RBAC configuration and permission error detection
- Proper use of table-driven tests and parallel execution
This is exemplary test code that ensures robust validation and error detection.
internal/remotestate/backend/azurerm/backend_interface_test.go (1)
1-814: Well-structured backend interface compliance tests.The test file thoroughly validates:
- Interface implementation compliance
- Configuration validation for all backend operations
- Proper filtering of Terragrunt-only configuration options
- Edge cases and error scenarios
Good use of table-driven tests and clear test naming conventions.
internal/remotestate/backend/azurerm/backend.go (2)
1773-1845: Well-designed factory configuration management.The implementation provides excellent runtime configuration capabilities with proper validation and structured updates. The separation between GetFactoryConfiguration and UpdateFactoryConfiguration methods enables clean configuration management.
32-73: Excellent container name validation implementation.The validation logic comprehensively covers all Azure container naming rules with clear error messages. The regex patterns correctly enforce the requirements.
internal/azure/azurehelper/azure_storage_account.go (3)
856-893: Well-structured RBAC role assignment implementation.The method effectively handles role assignment with proper error handling, retry logic, and clear separation of concerns through helper methods. The implementation correctly handles service principal vs user scenarios and RBAC propagation delays.
510-518: Good defensive programming for loop variable capture.The code correctly creates a new variable to avoid capturing the loop variable. While this is no longer necessary in Go 1.22+, it's good practice for backward compatibility with older Go versions.
272-276: Excellent user-friendly error message.The error message clearly explains the multiple ways to provide the subscription ID, making it easy for users to understand and fix the issue.
internal/azure/implementations/production.go (2)
824-842: Good implementation of GetResourceGroupService.Unlike other service creation methods, this one properly validates the subscription ID and creates the actual client using the helper package.
554-554: No action required: error constants are presentThe search confirms that
interfaces.ErrInvalidCredentialsandinterfaces.ErrNotImplementedare defined ininternal/azure/interfaces/auth.go(with corresponding tests ininterfaces/interfaces_test.go). No changes needed.
| // TestGetObjectInputValidation tests the validation of GetObjectInput | ||
| func TestGetObjectInputValidation(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Define test cases | ||
| testCases := []struct { | ||
| name string | ||
| input *azurehelper.GetObjectInput | ||
| expectedError string | ||
| }{ | ||
| { | ||
| name: "Valid Input", | ||
| input: &azurehelper.GetObjectInput{ | ||
| Container: azurehelper.StringPtr("container-name"), | ||
| Key: azurehelper.StringPtr("blob-key"), | ||
| }, | ||
| expectedError: "", | ||
| }, | ||
| { | ||
| name: "Missing Container", | ||
| input: &azurehelper.GetObjectInput{ | ||
| Key: azurehelper.StringPtr("blob-key"), | ||
| }, | ||
| expectedError: "container name is required", | ||
| }, | ||
| { | ||
| name: "Empty Container", | ||
| input: &azurehelper.GetObjectInput{ | ||
| Container: azurehelper.StringPtr(""), | ||
| Key: azurehelper.StringPtr("blob-key"), | ||
| }, | ||
| expectedError: "container name is required", | ||
| }, | ||
| { | ||
| name: "Missing Key", | ||
| input: &azurehelper.GetObjectInput{ | ||
| Container: azurehelper.StringPtr("container-name"), | ||
| }, | ||
| expectedError: "blob key is required", | ||
| }, | ||
| { | ||
| name: "Empty Key", | ||
| input: &azurehelper.GetObjectInput{ | ||
| Container: azurehelper.StringPtr("container-name"), | ||
| Key: azurehelper.StringPtr(""), | ||
| }, | ||
| expectedError: "blob key is required", | ||
| }, | ||
| { | ||
| name: "Nil Input", | ||
| input: nil, | ||
| expectedError: "input cannot be nil", | ||
| }, | ||
| } | ||
|
|
||
| // Create a validation test | ||
| // Run test cases | ||
| for _, tc := range testCases { | ||
| tc := tc // Capture range variable | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // We can't actually call GetObject since it requires a real client | ||
| // but we can verify the validation logic separately | ||
| var err error | ||
| switch { | ||
| case tc.input == nil: | ||
| err = errors.New("input cannot be nil") | ||
| case tc.input.Container == nil || *tc.input.Container == "": | ||
| err = errors.New("container name is required") | ||
| case tc.input.Key == nil || *tc.input.Key == "": | ||
| err = errors.New("blob key is required") | ||
| } | ||
|
|
||
| if tc.expectedError == "" { | ||
| assert.NoError(t, err) | ||
| } else { | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), tc.expectedError) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid duplicating validation logic in tests.
The test manually recreates the validation logic (lines 136-143) instead of calling actual validation methods from the GetObjectInput type. This creates maintenance overhead and potential inconsistencies.
The validation logic should be extracted to a method on GetObjectInput and then tested by calling that method:
- // We can't actually call GetObject since it requires a real client
- // but we can verify the validation logic separately
- var err error
- switch {
- case tc.input == nil:
- err = errors.New("input cannot be nil")
- case tc.input.Container == nil || *tc.input.Container == "":
- err = errors.New("container name is required")
- case tc.input.Key == nil || *tc.input.Key == "":
- err = errors.New("blob key is required")
- }
+ err := tc.input.Validate() // Call actual validation methodVerify if a validation method exists on GetObjectInput:
🏁 Script executed:
#!/bin/bash
# Search for validation methods on GetObjectInput
ast-grep --pattern 'func ($_) Validate() error'
rg -A 10 "func.*GetObjectInput.*Validate"Length of output: 15287
Implement GetObjectInput.Validate() and Simplify Tests
The TestGetObjectInputValidation test is duplicating validation logic because GetObjectInput currently lacks a Validate() method. To eliminate maintenance overhead and ensure consistency:
- Add a
Validate() errorreceiver onGetObjectInputininternal/azure/azurehelper/azure_blob.gothat encapsulates the current switch logic. - Update
azure_blob_test.goto calltc.input.Validate()instead of re-implementing the checks.
Suggested diff for the test (internal/azure/azurehelper/azure_blob_test.go):
@@ -136,11 +136,7 @@ t.Run(tc.name, func(t *testing.T) {
- // We can't actually call GetObject since it requires a real client
- // but we can verify the validation logic separately
- var err error
- switch {
- case tc.input == nil:
- err = errors.New("input cannot be nil")
- case tc.input.Container == nil || *tc.input.Container == "":
- err = errors.New("container name is required")
- case tc.input.Key == nil || *tc.input.Key == "":
- err = errors.New("blob key is required")
- }
+ err := tc.input.Validate()And in internal/azure/azurehelper/azure_blob.go, add:
func (i *GetObjectInput) Validate() error {
switch {
case i == nil:
return errors.New("input cannot be nil")
case i.Container == nil || *i.Container == "":
return errors.New("container name is required")
case i.Key == nil || *i.Key == "":
return errors.New("blob key is required")
}
return nil
}🤖 Prompt for AI Agents
In internal/azure/azurehelper/azure_blob.go around the GetObjectInput
definition, add a Validate() method that returns an error if the input is nil,
or if Container or Key fields are nil or empty, replicating the current
validation logic. Then in internal/azure/azurehelper/azure_blob_test.go lines 71
to 153, replace the inline validation switch in TestGetObjectInputValidation
with calls to tc.input.Validate(), removing the duplicated validation code and
using the new Validate() method to check errors.
| // RetryConfig defines retry behavior for Azure operations | ||
| type RetryConfig struct { | ||
| // MaxRetries is the maximum number of retries | ||
| MaxRetries int | ||
|
|
||
| // RetryDelay is the delay between retries in seconds | ||
| RetryDelay int | ||
|
|
||
| // MaxDelay is the maximum delay between retries in seconds | ||
| MaxDelay int | ||
|
|
||
| // RetryableStatusCodes are HTTP status codes that should trigger a retry | ||
| RetryableStatusCodes []int | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using time.Duration for consistency.
The RetryConfig struct uses int fields for delay times, but from the relevant code snippet in internal/remotestate/backend/azurerm/retry.go, other retry configurations use time.Duration fields (like InitialDelay, MaxDelay). This inconsistency could lead to confusion and errors.
Consider updating the fields to use time.Duration:
// RetryConfig defines retry behavior for Azure operations
type RetryConfig struct {
// MaxRetries is the maximum number of retries
MaxRetries int
- // RetryDelay is the delay between retries in seconds
- RetryDelay int
+ // RetryDelay is the delay between retries
+ RetryDelay time.Duration
- // MaxDelay is the maximum delay between retries in seconds
- MaxDelay int
+ // MaxDelay is the maximum delay between retries
+ MaxDelay time.Duration
// RetryableStatusCodes are HTTP status codes that should trigger a retry
RetryableStatusCodes []int
}Don't forget to add the time import:
import (
"context"
+ "time"
)📝 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.
| // RetryConfig defines retry behavior for Azure operations | |
| type RetryConfig struct { | |
| // MaxRetries is the maximum number of retries | |
| MaxRetries int | |
| // RetryDelay is the delay between retries in seconds | |
| RetryDelay int | |
| // MaxDelay is the maximum delay between retries in seconds | |
| MaxDelay int | |
| // RetryableStatusCodes are HTTP status codes that should trigger a retry | |
| RetryableStatusCodes []int | |
| } | |
| import ( | |
| "context" | |
| "time" | |
| ) | |
| // RetryConfig defines retry behavior for Azure operations | |
| type RetryConfig struct { | |
| // MaxRetries is the maximum number of retries | |
| MaxRetries int | |
| // RetryDelay is the delay between retries | |
| RetryDelay time.Duration | |
| // MaxDelay is the maximum delay between retries | |
| MaxDelay time.Duration | |
| // RetryableStatusCodes are HTTP status codes that should trigger a retry | |
| RetryableStatusCodes []int | |
| } |
🤖 Prompt for AI Agents
In internal/azure/interfaces/factory.go lines 31 to 44, the RetryConfig struct
uses int for delay fields which is inconsistent with other retry configurations
that use time.Duration. Change the types of RetryDelay and MaxDelay from int to
time.Duration to ensure consistency and clarity. Also, add the import statement
for the time package at the top of the file to support this change.
| func TestIsAzureError(t *testing.T) { | ||
| // This is more of an integration test and would require real Azure errors | ||
| // We'll just test the function signature here | ||
| assert.NotPanics(t, func() { | ||
| azureauth.IsAzureError(nil, "StorageAccountNotFound") | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace placeholder test with meaningful functionality.
The current test is essentially a no-op that only checks the function doesn't panic. This provides no value and should either be removed or replaced with meaningful tests.
Consider either removing this test entirely or implementing meaningful tests:
func TestIsAzureError(t *testing.T) {
- // This is more of an integration test and would require real Azure errors
- // We'll just test the function signature here
- assert.NotPanics(t, func() {
- azureauth.IsAzureError(nil, "StorageAccountNotFound")
- })
+ tests := []struct {
+ name string
+ err error
+ expectedCode string
+ expectedResult bool
+ }{
+ {
+ name: "nil error",
+ err: nil,
+ expectedCode: "StorageAccountNotFound",
+ expectedResult: false,
+ },
+ // Add more test cases for actual Azure errors
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ result := azureauth.IsAzureError(tt.err, tt.expectedCode)
+ assert.Equal(t, tt.expectedResult, result)
+ })
+ }
}Alternatively, if the function cannot be meaningfully tested without real Azure errors, consider removing the test entirely.
📝 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 TestIsAzureError(t *testing.T) { | |
| // This is more of an integration test and would require real Azure errors | |
| // We'll just test the function signature here | |
| assert.NotPanics(t, func() { | |
| azureauth.IsAzureError(nil, "StorageAccountNotFound") | |
| }) | |
| } | |
| func TestIsAzureError(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| err error | |
| expectedCode string | |
| expectedResult bool | |
| }{ | |
| { | |
| name: "nil error", | |
| err: nil, | |
| expectedCode: "StorageAccountNotFound", | |
| expectedResult: false, | |
| }, | |
| // Add more test cases for actual Azure errors | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| result := azureauth.IsAzureError(tt.err, tt.expectedCode) | |
| assert.Equal(t, tt.expectedResult, result) | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/azure/azureauth/auth_test.go around lines 221 to 227, the current
TestIsAzureError only checks that the function does not panic and does not
provide meaningful test coverage. To fix this, either remove this test entirely
if real Azure errors cannot be simulated, or replace it with tests that verify
the function's behavior using mocked or sample Azure error inputs to assert
expected outputs and error handling.
| // NewMockAzureBackendDependencies creates a new set of mock Azure backend dependencies for testing | ||
| func NewMockAzureBackendDependencies() *AzureBackendDependencies { | ||
| // For testing, mock implementations should be created directly rather than through a factory | ||
| return &AzureBackendDependencies{ | ||
| // Replace these with appropriate mock implementations | ||
| StorageAccountService: nil, // TODO: Add proper mock implementation | ||
| BlobService: nil, // TODO: Add proper mock implementation | ||
| ResourceGroupService: nil, // TODO: Add proper mock implementation, | ||
| } | ||
| } |
There was a problem hiding this comment.
Complete the mock implementation for testing.
The mock function returns nil for all service interfaces, which will cause runtime panics when used in tests. This incomplete implementation could block proper unit testing of the Azure backend.
Do you want me to generate mock implementations for the service interfaces or create a separate issue to track this task?
func NewMockAzureBackendDependencies() *AzureBackendDependencies {
- // For testing, mock implementations should be created directly rather than through a factory
- return &AzureBackendDependencies{
- // Replace these with appropriate mock implementations
- StorageAccountService: nil, // TODO: Add proper mock implementation
- BlobService: nil, // TODO: Add proper mock implementation
- ResourceGroupService: nil, // TODO: Add proper mock implementation,
- }
+ return &AzureBackendDependencies{
+ StorageAccountService: &MockStorageAccountService{},
+ BlobService: &MockBlobService{},
+ ResourceGroupService: &MockResourceGroupService{},
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/remotestate/backend/azurerm/backend_di.go around lines 52 to 61, the
NewMockAzureBackendDependencies function returns nil for all service interfaces,
which will cause runtime panics during tests. To fix this, replace the nil
values with proper mock implementations of StorageAccountService, BlobService,
and ResourceGroupService that satisfy their interfaces. You can either generate
these mocks using a mocking framework or manually implement simple mocks that
return controlled test data. This will ensure the tests run without panics and
can properly validate the Azure backend logic.
| func NewAzureBackendDependencies( | ||
| ctx context.Context, | ||
| l log.Logger, | ||
| opts *options.TerragruntOptions, | ||
| config map[string]interface{}, | ||
| subscriptionID string, | ||
| ) (*AzureBackendDependencies, error) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Remove unused parameter or implement its usage.
The subscriptionID parameter is not used in the function implementation, which suggests either missing functionality or an unnecessary parameter.
Please verify if the subscriptionID should be used in the service creation or if it can be removed from the function signature.
🏁 Script executed:
#!/bin/bash
# Check if subscriptionID is used in the Azure service factory methods
ast-grep --pattern $'func (azureFactory) GetResourceGroupService($$$) {
$$$
}'Length of output: 81
Here’s a deeper look into the function body and all references to subscriptionID:
🏁 Script executed:
#!/bin/bash
# Show the implementation of NewAzureBackendDependencies
rg -n "func NewAzureBackendDependencies" -A 50 internal/remotestate/backend/azurerm/backend_di.go
# List every occurrence of subscriptionID in this file
rg -n "subscriptionID" internal/remotestate/backend/azurerm/backend_di.goLength of output: 1983
To confirm whether subscriptionID is needed by the factory or any of its methods, let’s inspect the factory’s constructor, its struct fields, and each Get*Service signature:
🏁 Script executed:
#!/bin/bash
# Locate the factory constructor
rg -n "func NewAzureServiceFactory" -A 5 internal/remotestate/backend/azurerm
# Show the factory struct to see if it stores subscriptionID
rg -n "type azureFactory" -A 20 internal/remotestate/backend/azurerm
# Inspect each Get*Service method signature for a subscriptionID parameter
rg -n "GetResourceGroupService" -A 3 internal/remotestate/backend/azurerm
rg -n "GetStorageAccountService" -A 3 internal/remotestate/backend/azurerm
rg -n "GetBlobService" -A 3 internal/remotestate/backend/azurermLength of output: 5728
Let’s locate the factory definition to see if it ever consumes subscriptionID:
🏁 Script executed:
#!/bin/bash
# Find the factory constructor signature
rg -n "func NewAzureServiceFactory" -t go
# Find the azureFactory type to inspect its fields
rg -n "type azureFactory" -t goLength of output: 326
Remove the unused subscriptionID parameter from NewAzureBackendDependencies
The subscriptionID argument is never referenced in
internal/remotestate/backend/azurerm/backend_di.go:21–27 and none of the factory methods consume it. Either wire it into the factory (e.g. via NewAzureServiceFactoryWithOptions) or drop it entirely.
• File: internal/remotestate/backend/azurerm/backend_di.go
Lines: 21–27
Suggested diff:
func NewAzureBackendDependencies(
ctx context.Context,
l log.Logger,
opts *options.TerragruntOptions,
config map[string]interface{},
- subscriptionID string,
) (*AzureBackendDependencies, error) {Ensure you update all callers of NewAzureBackendDependencies accordingly.
🤖 Prompt for AI Agents
In internal/remotestate/backend/azurerm/backend_di.go lines 21 to 27, the
subscriptionID parameter in NewAzureBackendDependencies is unused and should be
removed. Edit the function signature to drop the subscriptionID argument and
update all calls to this function to no longer pass this parameter. This will
clean up the code by removing an unnecessary argument.
| defaultAccountKind: "StorageV2", | ||
| defaultAccountTier: "Standard", | ||
| defaultAccountSKU: "Standard_LRS", | ||
| defaultReplicationType: "Standard_LRS", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inconsistent field naming.
The field defaultReplicationType stores a full SKU value ("Standard_LRS") which includes both tier and replication type, not just the replication type.
Either rename the field to match its content or store only the replication type:
- defaultReplicationType: "Standard_LRS",
+ defaultReplicationType: "LRS",📝 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.
| defaultReplicationType: "Standard_LRS", | |
| defaultReplicationType: "LRS", |
🤖 Prompt for AI Agents
In internal/azure/azurehelper/azure_storage_account.go at line 318, the field
name defaultReplicationType is misleading because it stores a full SKU value
including tier and replication type. Rename the field to something like
defaultSku or defaultStorageSku to accurately reflect that it holds the full SKU
value, or alternatively, change the stored value to only the replication type if
you want to keep the field name as is.
| func (b *BlobServiceImpl) GetObject(ctx context.Context, input *types.GetObjectInput) (*types.GetObjectOutput, error) { | ||
| // Delegate to client or implement logic here | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Implement GetObject method or return an error.
The GetObject method returns nil which will cause nil pointer dereferences when callers try to use the result.
func (b *BlobServiceImpl) GetObject(ctx context.Context, input *types.GetObjectInput) (*types.GetObjectOutput, error) {
- // Delegate to client or implement logic here
- return nil, nil
+ return nil, fmt.Errorf("GetObject not yet implemented")
}📝 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 (b *BlobServiceImpl) GetObject(ctx context.Context, input *types.GetObjectInput) (*types.GetObjectOutput, error) { | |
| // Delegate to client or implement logic here | |
| return nil, nil | |
| } | |
| func (b *BlobServiceImpl) GetObject(ctx context.Context, input *types.GetObjectInput) (*types.GetObjectOutput, error) { | |
| return nil, fmt.Errorf("GetObject not yet implemented") | |
| } |
🤖 Prompt for AI Agents
In internal/azure/implementations/production.go at lines 191 to 194, the
GetObject method currently returns nil for both output and error, which can
cause nil pointer dereferences in callers. You should implement the method logic
to return a valid *types.GetObjectOutput or, if not implemented yet, return a
non-nil error indicating the method is not supported or not implemented to
prevent nil dereferences.
| return err | ||
| } | ||
|
|
||
| return r.AssignRole(ctx, l, principalID, roleDefinitionID, storageAccountScope) |
There was a problem hiding this comment.
Fix incorrect parameter order in AssignRole call.
The parameters are in the wrong order. Based on the method signature at line 276, the correct order is (ctx, l, roleName, principalID, scope).
- return r.AssignRole(ctx, l, principalID, roleDefinitionID, storageAccountScope)
+ return r.AssignRole(ctx, l, "Storage Blob Data Owner", principalID, storageAccountScope)🤖 Prompt for AI Agents
In internal/azure/implementations/production.go at line 408, the call to
AssignRole has parameters in the wrong order. Change the argument order to match
the method signature at line 276: pass ctx, l, roleDefinitionID, principalID,
and then storageAccountScope in that sequence.
|
Hey @omattsson Thanks for submitting this. I haven't reviewed this yet, but will as soon as I'm able to get this done: We should have these cloud helper packages nested away in The number of changes is massive, so it's going to take some time to review this, and I think I'll want multiple reviewers to go through this to make sure it's all good before we merge. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (9)
internal/azure/azurehelper/azure_storage_account.go (6)
297-297: Add missing constant definition fordefaultRoleAssignmentAPIVersionThe constant
defaultRoleAssignmentAPIVersionis used but not defined in this file or the constants file.Add this constant definition:
+const defaultRoleAssignmentAPIVersion = "2022-04-01"
332-332: Add missing HTTP status code constantsThe HTTP status code constants are used but not defined.
Add these constant definitions:
+const ( + httpStatusOK = 200 + httpStatusUnauthorized = 401 + httpStatusForbidden = 403 + httpStatusNotFound = 404 + httpStatusConflict = 409 +)Also applies to: 728-728
318-318: Rename field to accurately reflect its contentThe field stores a complete SKU value, not just the replication type.
- defaultReplicationType: "Standard_LRS", + defaultStorageSKU: "Standard_LRS",
418-418: Add missingdefaultLocationconstantThe constant
defaultLocationis used but not defined.Add this constant definition:
+const defaultLocation = "eastus"Also applies to: 525-525
809-809: Add missingdefaultHTTPClientTimeoutconstantThe constant is used but not defined.
+const defaultHTTPClientTimeout = 30 * time.Second
869-869: Add missingstorageBlobDataOwnerRoleIDconstantThe constant is used but not defined.
+const storageBlobDataOwnerRoleID = "b7e6dc6d-f1e8-4753-8033-0f276bb0955b"internal/azure/implementations/production.go (3)
998-1003: Implement storage client creation properlyThe function returns an error instead of creating the client. This needs proper implementation.
func createStorageClient(ctx context.Context, config map[string]interface{}) (*azurehelper.StorageAccountClient, error) { - // This is a stub that will return descriptive error - // Using the existing function requires a logger and configuration - // that may not be available in all contexts - return nil, fmt.Errorf("storage account client creation not initialized: use proper initialization through a service container instead") + return azurehelper.CreateStorageAccountClient(ctx, log.GetLogger(), config) }
1005-1010: Implement blob client creation properlyThe function returns an error instead of creating the client. This needs proper implementation.
func createBlobClient(ctx context.Context, config map[string]interface{}) (*azurehelper.BlobServiceClient, error) { - // This is a stub that will return descriptive error - // Using the existing function requires a logger and configuration - // that may not be available in all contexts - return nil, fmt.Errorf("blob service client creation not initialized: use proper initialization through a service container instead") + return azurehelper.CreateBlobServiceClient(ctx, log.GetLogger(), config) }
422-422: Fix incorrect parameter order in AssignRole callThe parameters are in the wrong order. The method expects
(ctx, l, roleName, principalID, scope).- return r.AssignRole(ctx, l, principalID, roleDefinitionID, storageAccountScope) + return r.AssignRole(ctx, l, "Storage Blob Data Owner", principalID, storageAccountScope)
🧹 Nitpick comments (2)
internal/azure/implementations/storage_account_service_test.go (1)
12-39: Consider consolidating redundant tests.This test overlaps significantly with
TestStorageAccountServiceInterfacewhich covers the same functionality more comprehensively. The second test checks more methods (GetStorageAccount,IsVersioningEnabled) that are missing from this test.Consider removing this test function since
TestStorageAccountServiceInterfaceprovides more complete coverage:-func TestStorageAccountServiceMethodsAreImplemented(t *testing.T) { - t.Run("methods should not return nil by default", func(t *testing.T) { - // Create a mock client for testing - service := &StorageAccountServiceImpl{ - client: nil, // This will cause issues if we actually call the methods, but we're just testing they exist - } - - // Test that methods exist and don't return nil without implementation - - // Test CreateStorageAccount exists - assert.NotNil(t, service.CreateStorageAccount) - - // Test DeleteStorageAccount exists - assert.NotNil(t, service.DeleteStorageAccount) - - // Test GetStorageAccountKeys exists - assert.NotNil(t, service.GetStorageAccountKeys) - - // Test GetStorageAccountSAS exists - assert.NotNil(t, service.GetStorageAccountSAS) - - // Test GetStorageAccountProperties exists - assert.NotNil(t, service.GetStorageAccountProperties) - - // Test that the interface methods are properly implemented - var _ interfaces.StorageAccountService = service - }) -}internal/azure/azureutil/types_test.go (1)
1-9: Consider using the project's custom error package.The project uses a custom error package
github.com/gruntwork-io/terragrunt/internal/errorswhich provides similar functionality tofmt.Errorfbut includes stack traces. Consider using this package's error functions instead of the standard library'serrors.New.import ( - "errors" "testing" "github.com/gruntwork-io/terragrunt/internal/azure/azureutil" + "github.com/gruntwork-io/terragrunt/internal/errors" "github.com/stretchr/testify/assert" )Then update all
errors.New()calls to useerrors.New()from the custom package.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/azure/azurehelper/azure_storage_account.go(1 hunks)internal/azure/azureutil/errorhandling_test.go(1 hunks)internal/azure/azureutil/types.go(1 hunks)internal/azure/azureutil/types_test.go(1 hunks)internal/azure/backend_di.go(1 hunks)internal/azure/implementations/blob_service.go(1 hunks)internal/azure/implementations/blob_service_test.go(1 hunks)internal/azure/implementations/production.go(1 hunks)internal/azure/implementations/storage_account_service_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/azure/backend_di.go
- internal/azure/azureutil/errorhandling_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧠 Learnings (2)
internal/azure/azureutil/types_test.go (1)
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#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.
internal/azure/azurehelper/azure_storage_account.go (2)
Learnt from: wakeful
PR: gruntwork-io/terragrunt#4491
File: test/integration_test.go:4265-4265
Timestamp: 2025-07-03T22:05:07.356Z
Learning: Constants defined in one Go file are accessible from other files in the same package without explicit imports. When reviewing Go code, consider the package-level scope rather than just the individual file scope.
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
🧬 Code Graph Analysis (3)
internal/azure/implementations/blob_service_test.go (2)
internal/azure/implementations/blob_service.go (1)
BlobServiceImpl(17-19)internal/azure/interfaces/storage.go (1)
BlobService(49-62)
internal/azure/implementations/blob_service.go (2)
internal/azure/azurehelper/azure_blob.go (1)
BlobServiceClient(20-23)internal/azure/interfaces/storage.go (1)
BlobService(49-62)
internal/azure/azurehelper/azure_storage_account.go (7)
internal/azure/azurehelper/azure_constants.go (5)
AccessTierHot(28-28)AccessTierCool(29-29)AccessTierPremium(30-30)RbacRetryAttempts(35-35)RbacRetryDelay(33-33)pkg/log/log.go (5)
Warn(37-39)Infof(82-84)Warnf(92-94)Info(27-29)Debugf(72-74)internal/azure/azureauth/auth.go (2)
GetAuthConfig(333-405)GetTokenCredential(418-482)internal/azure/azurehelper/azure_resource_group.go (1)
CreateResourceGroupClient(120-174)internal/azure/azureutil/errors.go (1)
IsPermissionError(17-32)internal/azure/errors/types.go (1)
IsPermissionError(154-169)internal/remotestate/backend/azurerm/errors.go (1)
IsPermissionError(270-287)
🔇 Additional comments (23)
internal/azure/implementations/blob_service_test.go (2)
14-45: LGTM! Comprehensive input validation tests.The test function properly validates all required input parameters for the
GetObjectmethod, including nil input, empty container name, and empty blob name. The error messages are descriptive and the test structure is clear.
47-63: LGTM! Good interface compliance verification.The test properly verifies that
BlobServiceImplimplements theBlobServiceinterface and that all required methods are present and non-nil. This ensures interface contract compliance.internal/azure/implementations/storage_account_service_test.go (1)
41-57: LGTM! Comprehensive interface compliance test.This test thoroughly verifies that
StorageAccountServiceImplimplements theStorageAccountServiceinterface and checks all required methods are present and non-nil.internal/azure/azureutil/types.go (4)
9-24: LGTM! Well-defined operation types.The
OperationTypeconstants are comprehensive and cover all the necessary Azure operations for telemetry tracking. The naming is consistent and descriptive.
26-42: LGTM! Comprehensive error metrics structure.The
ErrorMetricsstruct includes all necessary fields for detailed error tracking and telemetry. The structure supports rich metadata collection for debugging and monitoring.
44-59: LGTM! Useful helper functions.The helper functions
containsAnyandcontainsprovide clean, case-insensitive string matching functionality that's well-suited for error classification.
61-109: LGTM! The past review comment concern has been addressed.The function now consistently returns
ErrorClassUnknownfor both nil errors and empty error messages, which resolves the inconsistency mentioned in the past review. The error classification logic is well-ordered with more specific checks first, and the pattern matching is comprehensive.internal/azure/azureutil/types_test.go (7)
12-81: LGTM! Comprehensive operation type tests.The test thoroughly verifies all operation type constants and their string conversions. The parallel test execution and table-driven approach is well-implemented.
83-97: LGTM! Good error class constant verification.The test ensures all error class constants have the expected string values, providing good coverage of the defined constants.
99-249: LGTM! Excellent error classification test coverage.The test provides comprehensive coverage of the
ClassifyErrorfunction, including all error classes, case-insensitive matching, priority testing, and edge cases. The test cases are well-structured and cover real-world scenarios.
251-284: LGTM! Good struct field verification.The test properly verifies that all fields in the
ErrorMetricsstruct can be assigned and retrieved correctly, including the nested map structure.
286-307: LGTM! Useful default value tests.The test verifies that struct fields have expected zero values, which is important for ensuring proper initialization behavior.
309-346: LGTM! Thorough field assignment tests.The test ensures all struct fields can be properly assigned and retrieved, providing good coverage of the struct's functionality.
348-398: LGTM! Excellent edge case coverage.The test covers important edge cases like empty error messages, whitespace-only errors, mixed case keywords, partial matches, and priority handling. This ensures the classification logic is robust.
internal/azure/implementations/blob_service.go (4)
16-26: LGTM! Clean struct definition and constructor.The
BlobServiceImplstruct is properly defined with a single dependency, and the constructor correctly initializes the struct. The interface return type ensures proper abstraction.
28-81: LGTM! Excellent error handling and resource management.The
GetObjectmethod demonstrates robust error handling with:
- Comprehensive input validation
- Proper error wrapping
- Resource cleanup (stream closing)
- Thorough nil checks
The implementation properly handles the stream lifecycle and converts between different type systems.
83-106: LGTM! Clean delegation pattern.The container and blob management methods properly delegate to the underlying helper client while maintaining the interface contract. The methods are simple and focused.
108-128: LGTM! Well-implemented copy operation.The
CopyBlobToContainermethod correctly implements the copy operation by combiningGetObjectandUploadBlobcalls. The error handling is appropriate and the method properly handles cross-client copying.internal/azure/azurehelper/azure_storage_account.go (4)
229-320: Well-implemented client creation with proper validationThe function properly validates inputs, uses centralized authentication, and provides clear error messages for missing configuration.
346-409: Clean implementation of versioning managementThe versioning methods are well-structured with proper error handling and clear separation of concerns.
1068-1123: Comprehensive permission error detectionExcellent implementation that covers both Azure ResponseError types and string-based error detection with comprehensive keyword matching.
1002-1002: Rbac constants properly definedConfirmed that
RbacMaxRetriesis defined as 5 andRbacRetryAttemptsis correctly set toRbacMaxRetries + 1ininternal/azure/azurehelper/azure_constants.go. No changes required.internal/azure/implementations/production.go (1)
784-784: BlobService implementation exists
TheNewBlobServiceconstructor is defined ininternal/azure/implementations/blob_service.goand returns aBlobServiceImpl(which implements theinterfaces.BlobServiceinterface). No missing implementation—please disregard the original verification request.Likely an incorrect or invalid review comment.
| } | ||
|
|
||
| // Extract configuration | ||
| subscriptionID, _ := config["subscriptionId"].(string) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent configuration key naming
The code uses camelCase "subscriptionId" but other parts of the codebase use snake_case "subscription_id".
- subscriptionID, _ := config["subscriptionId"].(string)
+ subscriptionID, _ := config["subscription_id"].(string)📝 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.
| subscriptionID, _ := config["subscriptionId"].(string) | |
| subscriptionID, _ := config["subscription_id"].(string) |
🤖 Prompt for AI Agents
In internal/azure/implementations/production.go at line 799, the configuration
key "subscriptionId" is used in camelCase, which is inconsistent with the rest
of the codebase that uses snake_case keys. Change the key to "subscription_id"
to maintain consistent naming conventions across the codebase.
|
|
||
| // DeleteStorageAccount deletes a storage account by resource group and account name | ||
| func (s *StorageAccountServiceImpl) DeleteStorageAccount(ctx context.Context, resourceGroupName, accountName string) error { | ||
| return s.client.DeleteStorageAccount(ctx, nil) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pass proper logger instead of nil
The DeleteStorageAccount method passes nil for the logger parameter.
- return s.client.DeleteStorageAccount(ctx, nil)
+ return s.client.DeleteStorageAccount(ctx, log.GetLogger())📝 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.
| return s.client.DeleteStorageAccount(ctx, nil) | |
| return s.client.DeleteStorageAccount(ctx, log.GetLogger()) |
🤖 Prompt for AI Agents
In internal/azure/implementations/production.go at line 55, the call to
DeleteStorageAccount incorrectly passes nil for the logger parameter. Replace
nil with the appropriate logger instance available in the current context to
ensure proper logging during the deletion process.
| func (r *RBACServiceImpl) GetCurrentPrincipal(ctx context.Context) (*interfaces.Principal, error) { | ||
| token, err := r.credential.GetToken(ctx, policy.TokenRequestOptions{ | ||
| Scopes: []string{"https://management.azure.com/.default"}, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &interfaces.Principal{ | ||
| ID: token.Token, | ||
| Type: "ServicePrincipal", // Default to service principal, though this might be a user | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Critical: GetCurrentPrincipal returns token instead of principal ID
The function incorrectly returns the access token as the principal ID. This is a security issue and will cause authentication failures.
The principal ID should be extracted from the token claims or obtained through a separate API call:
func (r *RBACServiceImpl) GetCurrentPrincipal(ctx context.Context) (*interfaces.Principal, error) {
- token, err := r.credential.GetToken(ctx, policy.TokenRequestOptions{
- Scopes: []string{"https://management.azure.com/.default"},
- })
- if err != nil {
- return nil, err
- }
-
- return &interfaces.Principal{
- ID: token.Token,
- Type: "ServicePrincipal", // Default to service principal, though this might be a user
- }, nil
+ // This would require parsing the JWT token or using Microsoft Graph API
+ return nil, fmt.Errorf("GetCurrentPrincipal not properly implemented - requires JWT parsing or Graph API call")
}📝 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 (r *RBACServiceImpl) GetCurrentPrincipal(ctx context.Context) (*interfaces.Principal, error) { | |
| token, err := r.credential.GetToken(ctx, policy.TokenRequestOptions{ | |
| Scopes: []string{"https://management.azure.com/.default"}, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return &interfaces.Principal{ | |
| ID: token.Token, | |
| Type: "ServicePrincipal", // Default to service principal, though this might be a user | |
| }, nil | |
| } | |
| func (r *RBACServiceImpl) GetCurrentPrincipal(ctx context.Context) (*interfaces.Principal, error) { | |
| // This would require parsing the JWT token or using Microsoft Graph API | |
| return nil, fmt.Errorf("GetCurrentPrincipal not properly implemented - requires JWT parsing or Graph API call") | |
| } |
🤖 Prompt for AI Agents
In internal/azure/implementations/production.go around lines 426 to 438, the
GetCurrentPrincipal function incorrectly returns the access token string as the
principal ID, which is a security issue. To fix this, parse the token to extract
the principal ID from its claims or use an appropriate API call to retrieve the
principal's actual ID instead of returning the raw token string. Replace the
current assignment of ID with the extracted principal identifier.
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (35)
docs-starlight/src/content/docs/04-reference/04-experiments.md (1)
191-191: Complete the GitHub Discussions link with a specific discussion.The feedback link should point to a specific GitHub Discussion for the Azure Backend experiment, not just the general discussions page.
-If you use the Azure backend, please share your experience in the [Azure Backend for Remote State](https://github.com/gruntwork-io/terragrunt/discussions) GitHub Discussions. Your feedback will help improve the stability and features of this backend. +If you use the Azure backend, please share your experience in the [Azure Backend for Remote State](https://github.com/gruntwork-io/terragrunt/discussions/XXXX) GitHub Discussions. Your feedback will help improve the stability and features of this backend.Replace
XXXXwith the actual discussion number once created.internal/azure/azurehelper/azure_blob_test.go (1)
71-153: Implement GetObjectInput.Validate() method to eliminate validation logic duplication.The test manually recreates validation logic (lines 135-143) instead of calling actual validation methods from the
GetObjectInputtype. This creates maintenance overhead and potential inconsistencies.Add a
Validate() errormethod toGetObjectInputininternal/azure/azurehelper/azure_blob.go:func (i *GetObjectInput) Validate() error { switch { case i == nil: return errors.New("input cannot be nil") case i.Container == nil || *i.Container == "": return errors.New("container name is required") case i.Key == nil || *i.Key == "": return errors.New("blob key is required") } return nil }Then update the test to call
tc.input.Validate()instead of duplicating the validation logic:- // We can't actually call GetObject since it requires a real client - // but we can verify the validation logic separately - var err error - switch { - case tc.input == nil: - err = errors.New("input cannot be nil") - case tc.input.Container == nil || *tc.input.Container == "": - err = errors.New("container name is required") - case tc.input.Key == nil || *tc.input.Key == "": - err = errors.New("blob key is required") - } + err := tc.input.Validate()internal/azure/interfaces/factory.go (1)
31-44: Use time.Duration for delay fields to ensure consistency.The
RetryConfigstruct usesintfields for delay times, but other retry configurations in the codebase usetime.Durationfields. This inconsistency could lead to confusion and errors.Update the fields to use
time.Durationand add the required import:+import ( + "context" + "time" +) // RetryConfig defines retry behavior for Azure operations type RetryConfig struct { // MaxRetries is the maximum number of retries MaxRetries int - // RetryDelay is the delay between retries in seconds - RetryDelay int + // RetryDelay is the delay between retries + RetryDelay time.Duration - // MaxDelay is the maximum delay between retries in seconds - MaxDelay int + // MaxDelay is the maximum delay between retries + MaxDelay time.Duration // RetryableStatusCodes are HTTP status codes that should trigger a retry RetryableStatusCodes []int }internal/azure/azureauth/auth_test.go (1)
221-227: Replace placeholder test with meaningful functionality.The current test only verifies the function doesn't panic, providing no actual test coverage. This should either be removed or replaced with meaningful tests that verify the function's behavior.
Consider implementing tests that verify actual Azure error detection or remove this test entirely if meaningful testing isn't possible without real Azure errors.
internal/remotestate/backend/azurerm/backend_di.go (2)
52-61: Implement proper mock services to prevent runtime panics.Returning
nilfor all service interfaces will cause runtime panics when used in tests. Proper mock implementations are needed.Would you like me to generate mock implementations for the service interfaces or create an issue to track this task?
21-27: Remove unusedsubscriptionIDparameter.The
subscriptionIDparameter is not used anywhere in the function implementation. Either use it in the service creation or remove it from the function signature.func NewAzureBackendDependencies( ctx context.Context, l log.Logger, opts *options.TerragruntOptions, config map[string]interface{}, - subscriptionID string, ) (*AzureBackendDependencies, error) {internal/azure/azurehelper/azure_blob_service_test.go (2)
72-88: Test implementation concern: Inline validation logic instead of testing actual implementation.The test implements its own validation logic rather than testing the actual
CreateBlobServiceClientfunction. This approach tests assumptions rather than actual behavior and may not catch real bugs in the implementation.
160-176: Same issue: Inline validation logic instead of testing actual implementation.This test suffers from the same problem as the previous function - it implements validation logic inline rather than testing the actual
GetObjectfunction.internal/azure/factory/factory_test.go (1)
407-411: Inline validation logic instead of testing actual implementation.The validation logic is still implemented inline rather than testing actual validation functions. This approach tests assumptions rather than actual behavior.
Consider extracting the validation logic to a separate function that can be tested, or test the actual validation implementation if it exists.
internal/azure/azureutil/errors.go (1)
16-32: Fragile error detection using string pattern matching.The
IsPermissionErrorfunction relies on string pattern matching which is fragile and may not work reliably with structured errors or different error message formats from Azure SDK.Consider using more robust error detection by checking for specific error types or error codes from the Azure SDK instead of relying on error message strings.
internal/azure/azureutil/error_types.go (2)
1-32: Consolidate duplicate error handling implementations.This file introduces an
AzureErrorstruct and error classification system that is nearly identical to the existing implementation ininternal/azure/errors/types.go. Having two separate error handling mechanisms will lead to confusion and maintenance issues.The main differences are:
- This uses
ResourceType stringwhile the existing usesResourceTypeas a typed string- Different package locations (
azureutilvserrors)Consider either:
- Using the existing error types from
internal/azure/errors/types.go- If this package needs its own error types, clearly differentiate them and their use cases
- Consolidating both implementations into a single, shared error handling package
96-118: Export constructor functions to make error types usable.The constructor functions
newGenericErrorandnewPermissionErrorare unexported, making it impossible to createAzureErrorinstances from outside the package. This renders the exportedAzureErrortype and its options effectively unusable.Apply this diff to export the constructors:
-// newGenericError creates a new AzureError -func newGenericError(msg string, opts ...ErrorOption) *AzureError { +// NewGenericError creates a new AzureError +func NewGenericError(msg string, opts ...ErrorOption) *AzureError { err := &AzureError{ Message: msg, Classification: ErrorClassUnknown, } for _, opt := range opts { opt(err) } return err } -// newPermissionError creates a new permission-related AzureError -func newPermissionError(msg string, opts ...ErrorOption) *AzureError { +// NewPermissionError creates a new permission-related AzureError +func NewPermissionError(msg string, opts ...ErrorOption) *AzureError { err := &AzureError{ Message: msg, Classification: ErrorClassPermission, } for _, opt := range opts { opt(err) } return err }internal/azure/interfaces/rbac.go (1)
10-18: Fix inconsistent role constant name.The constant
RoleStorageBlobDataWriterhas a value of"Storage Blob Data Contributor". The name suggests "Writer" but the actual Azure role is "Contributor". This inconsistency could lead to confusion.Apply this diff to fix the constant name:
const ( RoleOwner = "Owner" RoleContributor = "Contributor" RoleReader = "Reader" RoleStorageBlobDataOwner = "Storage Blob Data Owner" RoleStorageBlobDataReader = "Storage Blob Data Reader" - RoleStorageBlobDataWriter = "Storage Blob Data Contributor" + RoleStorageBlobDataContributor = "Storage Blob Data Contributor" )internal/azure/azurehelper/azure_resource_group.go (2)
134-134: Mask subscription ID in logs for security.Logging the full subscription ID could be a security concern. Consider masking or partially redacting it.
Apply this diff to mask the subscription ID:
- l.Infof("Using subscription ID from auth config: %s", subscriptionID) + // Mask subscription ID for security - show only first 8 and last 4 characters + masked := subscriptionID + if len(subscriptionID) > 12 { + masked = subscriptionID[:8] + "****-****-****-" + subscriptionID[len(subscriptionID)-4:] + } + l.Infof("Using subscription ID from auth config: %s", masked)
277-285: Define missing httpStatusNotFound constant.The constant
httpStatusNotFoundis used but not defined in this file, which will cause a compilation error.Add this constant definition near the top of the file:
+const ( + httpStatusNotFound = 404 +) + // IsNotFoundError checks if the error is a "not found" error func IsNotFoundError(err error) bool {Alternatively, use the literal value
404directly or import from a constants package if one exists.internal/azure/azureutil/errorhandling.go (1)
129-130: Missing constant definitions will cause compilation errors.The constants
ErrorClassAuthorizationandOperationAuthenticationare used but not defined or imported in this file.These constants need to be either:
- Imported from the package where they are defined, or
- Defined in the
types.gofile in this packageBased on the codebase structure, you should add these definitions to
internal/azure/azureutil/types.go:const ( ErrorClassAuthorization ErrorClass = "authorization" OperationAuthentication OperationType = "authentication" )internal/azure/interfaces/auth.go (1)
63-65: Consider using PrincipalInfo instead of interface{} for GetCurrentPrincipal.The method returns
interface{}which reduces type safety. Since you've already defined thePrincipalInfostruct, consider using it as the return type for better type safety and consistency.internal/azure/factory/adapters.go (5)
25-44: Use the provided logger instead of Default().The method should accept a logger parameter instead of using
log.Default()to maintain proper log correlation and configuration.-func (s *storageAccountServiceAdapter) CreateStorageAccount(ctx context.Context, cfg *types.StorageAccountConfig) error { +func (s *storageAccountServiceAdapter) CreateStorageAccount(ctx context.Context, l log.Logger, cfg *types.StorageAccountConfig) error { // Convert the types.StorageAccountConfig to azurehelper.StorageAccountConfig azureConfig := azurehelper.StorageAccountConfig{ StorageAccountName: cfg.Name, ResourceGroupName: cfg.ResourceGroupName, Location: cfg.Location, EnableVersioning: cfg.EnableVersioning, AllowBlobPublicAccess: cfg.AllowBlobPublicAccess, AccountKind: string(cfg.AccountKind), AccountTier: string(cfg.AccountTier), AccessTier: string(cfg.AccessTier), ReplicationType: string(cfg.ReplicationType), Tags: cfg.Tags, } // Use CreateStorageAccountIfNecessary since there's no direct Create method - logger := log.Default() - return s.client.CreateStorageAccountIfNecessary(ctx, logger, azureConfig) + return s.client.CreateStorageAccountIfNecessary(ctx, l, azureConfig) }
46-50: Method parameters are not being used.The
DeleteStorageAccountmethod acceptsresourceGroupNameandaccountNameparameters but doesn't use them. This could lead to deleting the wrong storage account.
67-67: Potential nil pointer dereference.Dereferencing
account.Locationwithout checking if it's nil could cause a panic.- Location: *account.Location, + Location: getStringValue(account.Location),Consider adding a helper function at the package level:
func getStringValue(s *string) string { if s != nil { return *s } return "" }
88-101: Return errors for unimplemented methods instead of empty values.These placeholder methods return empty values which could mask implementation issues. They should return proper errors.
func (s *storageAccountServiceAdapter) GetStorageAccountKeys(ctx context.Context, resourceGroupName, accountName string) ([]string, error) { - // The azurehelper client doesn't expose this method yet - // This would need to be implemented in the azurehelper client - // For now, return a placeholder implementation - return []string{}, nil + return nil, fmt.Errorf("GetStorageAccountKeys is not implemented") } func (s *storageAccountServiceAdapter) GetStorageAccountSAS(ctx context.Context, resourceGroupName, accountName string) (string, error) { - // This would need to be implemented in the azurehelper client - // For now, return a placeholder implementation - return "", nil + return "", fmt.Errorf("GetStorageAccountSAS is not implemented") }
115-119: Config parameter is not being used.The
Existsmethod accepts aconfigparameter but doesn't use it. This could lead to checking the wrong storage account.internal/azure/factory/factory.go (4)
62-76: TODO: Implement options handling.The factory options are not being applied. This TODO should be addressed to enable retry configuration and other options.
func NewAzureServiceFactoryWithOptions(options *interfaces.FactoryOptions) interfaces.AzureServiceContainer { factory := NewAzureServiceFactory() if options != nil { // Apply options to the factory if factoryImpl, ok := factory.(*AzureServiceFactory); ok { - // For now, we'll store the options in the factory for later use - // The actual implementation would need to be enhanced to use these options - _ = factoryImpl - _ = options - // TODO: Enhance factory to use retry config and other options + // Store the options in the factory + factoryImpl.options = options } } return factory }Note: You'll need to add an
optionsfield to theAzureServiceFactorystruct.
84-90: Return actual factory options instead of hardcoded values.The
Optionsmethod returns empty/default values instead of the actual factory options.func (f *AzureServiceFactory) Options() *interfaces.FactoryOptions { - return &interfaces.FactoryOptions{ - DefaultConfig: make(map[string]interface{}), - RetryConfig: &interfaces.RetryConfig{}, - } + // Return the actual options stored in the factory + if f.options != nil { + return f.options + } + // Return default options if none were set + return &interfaces.FactoryOptions{ + DefaultConfig: make(map[string]interface{}), + RetryConfig: &interfaces.RetryConfig{}, + } }
141-149: Improve cache key generation to avoid collisions.Simple string concatenation with hyphens could cause cache key collisions if the values contain hyphens.
func (f *AzureServiceFactory) getCacheKey(config map[string]interface{}) string { storageAccount, _ := config["storage_account_name"].(string) subscriptionID, _ := config["subscription_id"].(string) resourceGroup, _ := config["resource_group_name"].(string) - return fmt.Sprintf("%s-%s-%s", storageAccount, subscriptionID, resourceGroup) + // Use a more robust key generation to avoid collisions + return fmt.Sprintf("storage:%s|subscription:%s|rg:%s", + url.QueryEscape(storageAccount), + url.QueryEscape(subscriptionID), + url.QueryEscape(resourceGroup)) }Note: You'll need to import
"net/url".
307-311: Validate subscription ID earlier.The subscription ID validation happens after acquiring locks. Moving it earlier would fail fast and avoid unnecessary locking.
func (f *AzureServiceFactory) GetResourceGroupService(ctx context.Context, l log.Logger, config map[string]interface{}) (interfaces.ResourceGroupService, error) { + // Validate required fields early + subscriptionID, _ := config["subscription_id"].(string) + if subscriptionID == "" { + return nil, fmt.Errorf("subscription_id is required in the configuration") + } + f.cacheMutex.RLock() // Check if a custom service is registered if f.customResourceGroupService != nil { f.cacheMutex.RUnlock() return f.customResourceGroupService, nil } // ... rest of the method - // Extract the subscription ID from config - subscriptionID, _ := config["subscription_id"].(string) - if subscriptionID == "" { - return nil, fmt.Errorf("subscription_id is required in the configuration") - }internal/remotestate/backend/azurerm/backend.go (3)
270-474: Function complexity is too high - consider decomposition.The
Bootstrapfunction is over 200 lines long and handles multiple responsibilities: configuration validation, authentication setup, storage account creation, and container creation. While the logic is correct, this complexity makes the function difficult to maintain and test.
1130-1266: Function complexity is too high - consider decomposition.The
bootstrapStorageAccountfunction is 130+ lines and handles multiple concerns: validation, client creation, storage account operations, role assignment, and versioning. This makes it difficult to test and maintain.
856-862: Consider streaming approach for large state files.Reading the entire blob into memory with
io.ReadAllcould cause memory issues for large Terraform state files.internal/azure/azurehelper/azure_storage_account.go (2)
318-318: Fix inconsistent field naming.The field
defaultReplicationTypestores a full SKU value ("Standard_LRS") which includes both tier and replication type, not just the replication type.
223-223: Add missing constant definitions.The code references several undefined constants that will cause compilation errors. Add these constant definitions at the package level.
Also applies to: 297-297, 418-418, 809-809, 834-834, 869-869, 1002-1002
internal/azure/implementations/production.go (4)
55-55: Pass proper logger instead of nilThe DeleteStorageAccount method passes nil for the logger parameter.
422-422: Fix incorrect parameter order in AssignRole call.The parameters are in the wrong order. Based on the method signature at line 276, the correct order is
(ctx, l, roleName, principalID, scope).
426-438: Critical: GetCurrentPrincipal returns token instead of principal IDThe function incorrectly returns the access token as the principal ID. This is a security issue and will cause authentication failures.
799-799: Use consistent configuration key namingThe code uses camelCase "subscriptionId" but other parts of the codebase use snake_case "subscription_id".
🧹 Nitpick comments (25)
internal/azure/azurehelper/test_helpers.go (1)
4-9: Well-implemented utility function.The
StringPtrhelper function is correctly implemented and well-documented. This is a common pattern when working with Azure SDK APIs that expect string pointers. The function serves a legitimate purpose and follows Go conventions.Consider adding a brief usage example in the documentation to make it even clearer:
// StringPtr returns a pointer to the given string value. // This is a helper function commonly used in tests and when working with Azure SDK functions // that expect string pointers. +// Example: StringPtr("example") returns &"example" func StringPtr(s string) *string { return &s }docs/_docs/04_reference/02-cli-options.md (1)
2240-2245: Mirror the wording update in thedocs-starlighttree.Nice catch adding Azure support here. Per the ongoing docs-migration guidelines, please confirm that the corresponding flag description in
docs-starlight/src/data/flags/dependency-fetch-output-from-state.mdx(and any other starlight files) now contains the exact same Azure wording to keep both doc sets in sync.internal/azure/azurehelper/storage_account_adapter.go (1)
1-5: Add a direct pointer to the new replacement typeThe deprecation notice is clear, but adding a concrete package + type reference will help users migrate faster, e.g.:
// Deprecated: StorageAccountServiceAdapter has been removed. // Use internal/azure/interfaces.StorageAccountService instead.docs/_docs/05_troubleshooting/03-performance.md (2)
48-49: Tighten phrasing for clarity-...fetch the backend state file from the supported storage backend (AWS S3 or Azure Storage) and parse it directly... +...fetch the state file directly from AWS S3 or Azure Storage and parse it...
58-59: Simplify style and fix passive wording-The first thing you need to be aware of when considering usage of the `--dependency-fetch-output-from-state` flag is that it only works for AWS S3 and Azure Storage backends. +Note that the `--dependency-fetch-output-from-state` flag currently works only with AWS S3 and Azure Storage backends.internal/azure/implementations/production_test.go (1)
9-35: Consider enhancing test coverage for better validation.While the current tests correctly verify that
NewProductionServiceContainerreturns a non-nil container, they could be more comprehensive:
- Interface validation: Verify that the returned container actually implements the
AzureServiceContainerinterface- Configuration handling: Test that the container properly handles and stores the provided configuration
- Basic functionality: Test that key methods on the container work as expected
Consider adding assertions like:
// Verify interface implementation _, ok := container.(interfaces.AzureServiceContainer) if !ok { t.Fatal("Container does not implement AzureServiceContainer interface") } // Verify configuration is properly stored/handled // (depending on what methods are available on the interface)This would provide better confidence that the container is properly instantiated and functional.
cli/commands/backend/migrate/migrate_test.go (1)
12-42: Basic smoke test serves its purpose but could be enhanced.The test correctly verifies that the migration function is callable and handles error scenarios. However, consider these improvements:
- More specific error assertions: Instead of just checking for any error, verify the specific error type or message
- Better test isolation: Consider using temporary directories for test paths
- Mock dependencies: For more comprehensive testing, consider mocking the underlying dependencies
- // We expect an error due to missing files, but the function should be callable - require.Error(t, err) + // We expect a specific error due to missing files + require.Error(t, err) + assert.Contains(t, err.Error(), "not found") // or specific error messageinternal/azure/azurehelper/azure_rbac_constants_test.go (2)
35-50: Remove redundant arithmetic verification test cases.The test cases starting at line 36 are not testing any actual code behavior - they're just verifying that
maxRetries + 1 == expectedAttemptsfor various hardcoded values. This provides no value to the test suite.- // Test different values of the relationship - testCases := []struct { - maxRetries int - expectedAttempts int - }{ - {3, 4}, - {5, 6}, // Current value - {10, 11}, - {0, 1}, // Edge case: no retries means 1 attempt - } - - for _, tc := range testCases { - calculatedAttempts := tc.maxRetries + 1 - assert.Equal(t, tc.expectedAttempts, calculatedAttempts, - "When max retries is %d, attempts should be %d", tc.maxRetries, tc.expectedAttempts) - }
54-54: Remove unnecessary nolint directive.The
// nolint: govetcomment appears unnecessary as there's no apparent govet issue in this function.-// nolint: govet func TestIsPermissionError(t *testing.T) {internal/azure/backend_di.go (1)
13-28: Consider using maps.Copy for more efficient map merging.The current implementation is correct but could be simplified and made more efficient using Go 1.21+'s
mapspackage.+import ( + "maps" +) // Helper function to merge configuration maps func mergeConfig(base, override map[string]interface{}) map[string]interface{} { - result := make(map[string]interface{}) - - // Copy base config - for k, v := range base { - result[k] = v - } - - // Override with specific config - for k, v := range override { - result[k] = v - } - - return result + result := make(map[string]interface{}, len(base)) + maps.Copy(result, base) + maps.Copy(result, override) + return result }internal/azure/interfaces/container.go (1)
27-118: Consider type-safe configuration for service factories.All service factory methods accept
config map[string]interface{}, which provides flexibility but sacrifices compile-time type safety. Consider defining specific configuration structs for each service type to improve type safety and documentation.Example approach:
// Define typed configuration structs type StorageAccountServiceConfig struct { SubscriptionID string ResourceGroup string // ... other fields } // Alternative interface method signature GetStorageAccountService(ctx context.Context, l log.Logger, config StorageAccountServiceConfig) (StorageAccountService, error)This would make the API more self-documenting and catch configuration errors at compile time rather than runtime.
internal/azure/azurehelper/azure_resource_group_test.go (2)
11-11: Remove commented import statement.The commented import for the errors package should be removed to keep the code clean.
- // "github.com/gruntwork-io/terragrunt/internal/errors"
502-519: Remove or implement the commented test stubs.The commented test function stubs should either be implemented with proper mocks or removed entirely. Keeping commented code reduces maintainability.
Either implement these tests with proper mocks or remove them:
-// In a real test environment with Azure credentials, we could test the actual client -// but for unit tests, we would need to mock the Azure SDK clients. -// Below is an example of how we might structure those tests if we had mocks: - -/* -func TestCreateResourceGroupClient(t *testing.T) { - // This would be implemented with mocks -} - -func TestResourceGroupExists(t *testing.T) { - // This would be implemented with mocks -} - -func TestCreateResourceGroupIfNotExists(t *testing.T) { - // This would be implemented with mocks -} -*/internal/azure/azurehelper/azure_storage_account_test.go (1)
954-961: Consider using slices.Contains from the standard library.If the project uses Go 1.21+, replace this custom implementation with
slices.Containsfrom the standard library for better maintainability.-// Helper function to check if a slice contains a string -func contains(slice []string, str string) bool { - for _, v := range slice { - if v == str { - return true - } - } - return false -} +import "slices" + +// Then use slices.Contains(slice, str) directly in the testsinternal/azure/azurehelper/azure_blob.go (2)
83-105: Consider extracting storage account verification logic.The storage account existence verification logic could be extracted into a separate method to improve readability and reusability of the
CreateBlobServiceClientfunction.+func verifyStorageAccountExists(ctx context.Context, l log.Logger, config map[string]interface{}) error { + subscriptionID, _ := config["subscription_id"].(string) + resourceGroupName, _ := config["resource_group_name"].(string) + storageAccountName, _ := config["storage_account_name"].(string) + + if subscriptionID == "" || resourceGroupName == "" { + return nil // Skip verification if we don't have the required info + } + + saClient, err := CreateStorageAccountClient(ctx, l, config) + if err != nil { + return errors.Errorf("error creating storage account client: %w", err) + } + + exists, _, err := saClient.StorageAccountExists(ctx) + if err != nil { + return errors.Errorf("error checking if storage account exists: %w", err) + } + + if !exists { + return errors.Errorf("storage account %s does not exist in resource group %s", + storageAccountName, resourceGroupName) + } + + l.Infof("Verified storage account %s exists", storageAccountName) + return nil +} // If we have subscription ID and resource group, verify storage account exists using Management API -if subscriptionID != "" && resourceGroupName != "" { - // Create storage account client to verify the storage account exists - var saClient *StorageAccountClient - saClient, err = CreateStorageAccountClient(ctx, l, config) - if err != nil { - return nil, errors.Errorf("error creating storage account client: %w", err) - } - - // Check if the storage account exists - var exists bool - exists, _, err = saClient.StorageAccountExists(ctx) - if err != nil { - return nil, errors.Errorf("error checking if storage account exists: %w", err) - } - - if !exists { - return nil, errors.Errorf("storage account %s does not exist in resource group %s", - storageAccountName, resourceGroupName) - } - - l.Infof("Verified storage account %s exists", storageAccountName) -} +if err = verifyStorageAccountExists(ctx, l, config); err != nil { + return nil, err +}
107-107: Consider supporting custom domains and different Azure environments.The hardcoded URL format assumes Azure public cloud. Consider making this configurable to support custom domains and other Azure environments (China, Government).
- url := fmt.Sprintf("https://%s.blob.core.windows.net", storageAccountName) + // Support custom endpoints from config, default to public cloud + endpoint, ok := config["endpoint_suffix"].(string) + if !ok || endpoint == "" { + endpoint = "core.windows.net" + } + url := fmt.Sprintf("https://%s.blob.%s", storageAccountName, endpoint)internal/remotestate/backend/azurerm/backend_interface_test.go (1)
375-385: Consider making error assertions more specific.The test checks for multiple possible error messages, which could indicate inconsistent error behavior. Consider making the backend return more predictable errors to improve test reliability.
- assert.True(t, - strings.Contains(err.Error(), "subscription_id") || - strings.Contains(err.Error(), "storage account") || - strings.Contains(err.Error(), "credential") || - strings.Contains(err.Error(), "auth") || - strings.Contains(err.Error(), "no such host"), - "Error should be related to authentication/connectivity, got: %v", err) + // Consider checking for a specific error type or message + assert.Contains(t, err.Error(), "subscription_id", + "Expected error about missing subscription_id, got: %v", err)internal/azure/factory/factory.go (1)
333-343: Consider implementing RBAC and Authentication services.Both GetRBACService and GetAuthenticationService return "not implemented" errors. These services appear to be important for the Azure backend functionality.
Would you like me to help implement these services or create issues to track their implementation?
internal/remotestate/backend/azurerm/backend.go (3)
905-913: Remove redundant nil check.The nil check is unnecessary since
azErrors.IsPermissionErrorshould handle nil inputs internally. This simplifies the code without losing functionality.func (backend *Backend) isPermissionError(err error) bool { - // First check if it's a nil error - if err == nil { - return false - } - - // Fall back to the error utilities return azErrors.IsPermissionError(err) }
1586-1700: Add documentation to clarify factory implementations.Both
enhancedServiceFactoryandsimpleServiceFactoryimplement the same interface but serve different purposes. Add documentation to clarify when each should be used to improve maintainability.-// enhancedServiceFactory wraps a service container with enhanced configuration +// enhancedServiceFactory wraps a service container with enhanced configuration. +// Use this factory when you need telemetry collection, authentication settings, +// and runtime configuration updates for production scenarios. type enhancedServiceFactory struct { container interfaces.AzureServiceContainer telemetrySettings *TelemetrySettings authSettings *AuthSettings } ... -// simpleServiceFactory is a basic wrapper around an AzureServiceContainer +// simpleServiceFactory is a basic wrapper around an AzureServiceContainer. +// Use this factory for testing or when enhanced features like telemetry +// and runtime configuration updates are not needed. type simpleServiceFactory struct { container interfaces.AzureServiceContainer }
200-236: Function always returns nil error.The
extractBackendConfigFromRemoteStatefunction signature includes an error return, but the implementation always returnsnil. Either add validation logic that could return errors or simplify the signature.-func extractBackendConfigFromRemoteState(remoteStateConfig backend.Config, cfg *BackendConfig, opts *options.TerragruntOptions) error { +func extractBackendConfigFromRemoteState(remoteStateConfig backend.Config, cfg *BackendConfig, opts *options.TerragruntOptions) { // Set retry configuration based on environment or defaults if retryCount := opts.RetryMaxAttempts; retryCount > 0 { cfg.RetryConfig = &interfaces.RetryConfig{ MaxRetries: retryCount, RetryDelay: 1, MaxDelay: 30, RetryableStatusCodes: []int{408, 429, 500, 502, 503, 504}, } } // Configure telemetry settings based on options cfg.TelemetrySettings = &TelemetrySettings{ EnableDetailedMetrics: true, // Default to enabled EnableErrorTracking: true, MetricsBufferSize: 1000, FlushInterval: 30, } // Configure cache settings cfg.CacheSettings = &CacheSettings{ EnableCaching: true, CacheTimeout: 300, MaxCacheSize: 100, EnableCacheMetrics: false, } // Configure auth settings based on remote state config cfg.AuthSettings = &AuthSettings{ PreferredAuthMethod: extractPreferredAuthMethod(remoteStateConfig), EnableAuthCaching: true, AuthCacheTimeout: 3600, EnableAuthRetry: true, } - - return nil }Then update the caller:
- if err := extractBackendConfigFromRemoteState(remoteStateConfig, cfg, opts); err != nil { - return nil, fmt.Errorf("failed to extract backend configuration: %w", err) - } + extractBackendConfigFromRemoteState(remoteStateConfig, cfg, opts)internal/azure/azurehelper/azure_storage_account.go (1)
1000-1065: Simplify retry logic with early returns.The
waitForRBACPermissionsfunction has deeply nested conditionals and duplicate retry logic. Consider simplifying with early returns and extracting the retry delay logic.func (c *StorageAccountClient) waitForRBACPermissions(ctx context.Context, l log.Logger) error { l.Infof("Waiting for RBAC permissions to propagate (up to %d attempts with %v delays)", RbacRetryAttempts, RbacRetryDelay) for attempt := 1; attempt <= RbacRetryAttempts; attempt++ { l.Debugf("RBAC permission test attempt %d/%d", attempt, RbacRetryAttempts) // Test permissions by trying to get storage account properties _, err := c.client.GetProperties(ctx, c.resourceGroupName, c.storageAccountName, nil) if err == nil { l.Infof("RBAC permissions verified successfully on attempt %d", attempt) return nil } + // Check if we've exhausted retries + if attempt >= RbacRetryAttempts { + l.Warnf("Permission still denied after %d attempts. RBAC may need more time to propagate", RbacRetryAttempts) + return fmt.Errorf("RBAC permissions not available after %d attempts: %w", RbacRetryAttempts, err) + } + // Check if this is a permission-related error var respErr *azcore.ResponseError - if errors.As(err, &respErr) { - if isPermissionError(respErr) { - if attempt < RbacRetryAttempts { - l.Debugf("Permission denied on attempt %d, waiting %v before retry (Error: %s)", - attempt, RbacRetryDelay, respErr.Error()) - - // Wait before retrying - select { - case <-ctx.Done(): - return ctx.Err() - case <-time.After(RbacRetryDelay): - continue - } - } else { - l.Warnf("Permission still denied after %d attempts. RBAC may need more time to propagate", RbacRetryAttempts) - return fmt.Errorf("RBAC permissions not available after %d attempts: %w", RbacRetryAttempts, err) - } - } else { - // Non-permission error, permissions might be working but other issue - l.Debugf("Non-permission error during RBAC test (attempt %d): %v", attempt, err) - - if attempt < RbacRetryAttempts { - select { - case <-ctx.Done(): - return ctx.Err() - case <-time.After(RbacRetryDelay): - continue - } - } - } - } else { - // Unknown error type - l.Debugf("Unknown error during RBAC test (attempt %d): %v", attempt, err) - - if attempt < RbacRetryAttempts { - select { - case <-ctx.Done(): - return ctx.Err() - case <-time.After(RbacRetryDelay): - continue - } - } + if errors.As(err, &respErr) && isPermissionError(respErr) { + l.Debugf("Permission denied on attempt %d, waiting %v before retry (Error: %s)", + attempt, RbacRetryDelay, respErr.Error()) + } else { + l.Debugf("Non-permission error during RBAC test (attempt %d): %v", attempt, err) } + + // Wait before retrying + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(RbacRetryDelay): + continue + } } return fmt.Errorf("RBAC permissions verification failed after %d attempts", RbacRetryAttempts) }internal/azure/implementations/production.go (1)
985-994: Consider improving role name extraction logicThe current implementation just returns the last part of the role definition ID, which may not always provide a meaningful role name. Consider mapping well-known role definition IDs to human-readable names.
func extractRoleNameFromDefinitionID(roleDefinitionID string) string { + // Map well-known role definition IDs to readable names + wellKnownRoles := map[string]string{ + "b24988ac-6180-42a0-ab88-20f7382dd24c": "Contributor", + "acdd72a7-3385-48ef-bd42-f606fba81ae7": "Reader", + "8e3af657-a8ff-443c-a75c-2fe8c4bcb635": "Owner", + "ba92f5b4-2d11-453d-a403-e96b0029c9fe": "Storage Blob Data Contributor", + "b7e6dc6d-f1e8-4753-8033-0f276bb0955b": "Storage Blob Data Owner", + } + // Role definition ID format is usually: // /subscriptions/{subscriptionId}/providers/Microsoft.Authorization/roleDefinitions/{definitionID} - // For simplicity, just return the last part parts := strings.Split(roleDefinitionID, "/") if len(parts) > 0 { - return parts[len(parts)-1] + roleID := parts[len(parts)-1] + if name, ok := wellKnownRoles[roleID]; ok { + return name + } + return roleID } return roleDefinitionID }internal/azure/types/storage_types_test.go (2)
150-200: Consider if aString()method is needed forAccountKind.The test is simply casting the type to string, which suggests
AccountKindis just a string alias. If string conversion behavior is important to test, consider implementing a properString()method.If
AccountKindneeds custom string behavior, implement aString()method:// In the types file, add: +func (k AccountKind) String() string { + return string(k) +} // In the test, replace casting with method call: - result := string(tc.kind) + result := tc.kind.String()Otherwise, this test may be unnecessary since it's just testing Go's built-in type conversion.
202-247: Consider if aString()method is needed forAccessTier.Similar to
AccountKind, this test is just casting to string. If no custom string behavior is needed, this test may be redundant.If
AccessTierneeds custom string behavior, implement aString()method:// In the types file, add: +func (t AccessTier) String() string { + return string(t) +} // In the test, replace casting with method call: - result := string(tc.tier) + result := tc.tier.String()Otherwise, consider removing this test as it's testing Go's built-in type conversion rather than custom behavior.
| return nil, err | ||
| } | ||
| if !exists { | ||
| return nil, fmt.Errorf("Azure container %s does not exist", containerName) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the project's custom error package for better debugging.
Replace fmt.Errorf with the project's custom error package github.com/gruntwork-io/terragrunt/internal/errors which provides stack traces for better debugging.
First, add the import:
import (
"context"
"encoding/json"
- "fmt"
"io"
// ... other imports
+ "github.com/gruntwork-io/terragrunt/internal/errors"Then update the error handling:
- return nil, fmt.Errorf("Azure container %s does not exist", containerName)
+ return nil, errors.Errorf("Azure container %s does not exist", containerName)- return nil, fmt.Errorf("error reading terraform state blob %s from container %s: %w", key, containerName, err)
+ return nil, errors.Errorf("error reading terraform state blob %s from container %s: %w", key, containerName, err)- return nil, fmt.Errorf("error reading response body: %w", err)
+ return nil, errors.Errorf("error reading response body: %w", err)Also applies to: 292-292, 298-298
🤖 Prompt for AI Agents
In config/dependency_test.go at lines 278, 292, and 298, replace the use of
fmt.Errorf with the project's custom error package
github.com/gruntwork-io/terragrunt/internal/errors to improve debugging with
stack traces. First, add the import for the errors package, then update the
error creation calls to use errors.WithStack or the appropriate function from
that package instead of fmt.Errorf.
| // WrapContainerDoesNotExistError wraps a container not found error with context | ||
| func WrapContainerDoesNotExistError(err error, containerName string) error { | ||
| return NewGenericError( | ||
| fmt.Sprintf("Container %s does not exist", containerName), | ||
| WithError(err), | ||
| WithClassification(ErrorClassNotFound), | ||
| WithResourceType(ResourceTypeContainer), | ||
| WithResourceName(containerName), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add nil error check for consistency.
Unlike other wrapper functions, this doesn't check if the error is nil before wrapping.
func WrapContainerDoesNotExistError(err error, containerName string) error {
+ if err == nil {
+ return nil
+ }
return NewGenericError(📝 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.
| // WrapContainerDoesNotExistError wraps a container not found error with context | |
| func WrapContainerDoesNotExistError(err error, containerName string) error { | |
| return NewGenericError( | |
| fmt.Sprintf("Container %s does not exist", containerName), | |
| WithError(err), | |
| WithClassification(ErrorClassNotFound), | |
| WithResourceType(ResourceTypeContainer), | |
| WithResourceName(containerName), | |
| ) | |
| } | |
| // WrapContainerDoesNotExistError wraps a container not found error with context | |
| func WrapContainerDoesNotExistError(err error, containerName string) error { | |
| if err == nil { | |
| return nil | |
| } | |
| return NewGenericError( | |
| fmt.Sprintf("Container %s does not exist", containerName), | |
| WithError(err), | |
| WithClassification(ErrorClassNotFound), | |
| WithResourceType(ResourceTypeContainer), | |
| WithResourceName(containerName), | |
| ) | |
| } |
🤖 Prompt for AI Agents
In internal/azure/errors/wrappers.go around lines 111 to 120, the
WrapContainerDoesNotExistError function lacks a nil check for the input error,
unlike other wrapper functions. Add a check at the start of the function to
return nil immediately if the input error is nil, ensuring consistent behavior
across all wrapper functions.
| import ( | ||
| "fmt" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the custom errors package for better error handling.
According to the project conventions, the custom github.com/gruntwork-io/terragrunt/internal/errors package should be used instead of fmt for error formatting as it provides stack traces.
import (
- "fmt"
+ terr "github.com/gruntwork-io/terragrunt/internal/errors"
)Then update error message formatting:
-fmt.Sprintf("Permission denied while accessing blob %s in container %s", key, container)
+terr.Errorf("Permission denied while accessing blob %s in container %s", key, container).Error()🤖 Prompt for AI Agents
In internal/azure/errors/wrappers.go around lines 3 to 5, replace the import of
the standard "fmt" package with the custom
"github.com/gruntwork-io/terragrunt/internal/errors" package. Then update all
error message formatting in this file to use the custom errors package functions
that provide stack traces, instead of fmt functions, to align with project
conventions for better error handling.
| // WrapAuthenticationError wraps an authentication error with context | ||
| func WrapAuthenticationError(err error, method string) error { | ||
| return NewGenericError( | ||
| fmt.Sprintf("Failed to authenticate using %s", method), | ||
| WithError(err), | ||
| WithClassification(ErrorClassAuthentication), | ||
| WithSuggestion("Check your credentials and ensure you have the necessary permissions."), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add nil error check for consistency.
This function should also check for nil errors like the other wrapper functions.
func WrapAuthenticationError(err error, method string) error {
+ if err == nil {
+ return nil
+ }
return NewGenericError(🤖 Prompt for AI Agents
In internal/azure/errors/wrappers.go around lines 122 to 130, the
WrapAuthenticationError function lacks a nil error check, unlike other wrapper
functions. Add a check at the start of the function to return nil immediately if
the input error is nil, ensuring consistent behavior across all error wrapper
functions.
| ## Configuration Options | ||
|
|
||
| ### Required Configuration | ||
|
|
||
| | Option | Description | | ||
| |--------|-------------| | ||
| | `storage_account_name` | Name of the Azure Storage Account to store state files | | ||
| | `container_name` | Name of the Blob Container to store state files | | ||
| | `resource_group_name` | Name of the Resource Group containing the Storage Account | | ||
| | `subscription_id` | Azure Subscription ID | | ||
| | `key` | Path to the state file within the container | | ||
|
|
||
| ### Optional Configuration | ||
|
|
||
| | Option | Default | Description | | ||
| |--------|---------|-------------| | ||
| | `enable_versioning` | `false` | Enable blob versioning for state files | | ||
| | `use_azuread_auth` | `false` | Use Azure AD authentication (recommended) | | ||
| | `use_msi` | `false` | Use Managed Service Identity for authentication | | ||
| | `sas_token` | `""` | SAS token for Storage Account access | | ||
| | `snapshot` | `""` | Use a specific state file snapshot version | | ||
| | `client_id` | `""` | Service Principal Client ID | | ||
| | `client_secret` | `""` | Service Principal Client Secret | | ||
| | `tenant_id` | `""` | Azure AD Tenant ID | | ||
|
|
||
| ### Storage Account Creation Options | ||
|
|
||
| These options are only used when `create_storage_account_if_not_exists = true`: | ||
|
|
||
| | Option | Default | Description | | ||
| |--------|---------|-------------| | ||
| | `location` | Required | Azure region for the Storage Account | | ||
| | `account_tier` | `"Standard"` | Performance tier (`Standard` or `Premium`) | | ||
| | `replication_type` | `"LRS"` | Replication type (`LRS`, `GRS`, `RAGRS`, etc.) | | ||
| | `account_kind` | `"StorageV2"` | Storage Account kind | | ||
| | `access_tier` | `"Hot"` | Access tier (`Hot` or `Cool`) | | ||
| | `min_tls_version` | `"TLS1_2"` | Minimum TLS version | | ||
| | `allow_blob_public_access` | `false` | Allow public access to blobs | | ||
| | `enable_https_traffic_only` | `true` | Allow only HTTPS traffic | | ||
|
|
||
| ## Authentication Methods | ||
|
|
||
| There are several ways to authenticate with Azure. Below are the recommended approaches in order of preference: | ||
|
|
||
| 1. **Azure AD (Recommended)** |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate documentation sections.
The documentation contains duplicate sections:
- "Configuration Options" appears at lines 50 and 237
- "Authentication Methods" appears at lines 90 and 401
This duplication makes the documentation confusing and harder to maintain. Consider consolidating these into single, comprehensive sections.
Also applies to: 237-453
🧰 Tools
🪛 LanguageTool
[style] ~88-~88: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...le_https_traffic_only|true` | Allow only HTTPS traffic | ## Authentication Meth...
(ADVERB_REPETITION_PREMIUM)
🤖 Prompt for AI Agents
In internal/azure/CONFIGURATION.md between lines 50-94 and also in lines
237-453, there are duplicate sections titled "Configuration Options" and
"Authentication Methods." To fix this, consolidate these duplicate sections into
single, comprehensive ones by merging their content and removing the redundant
copies. Ensure the final documentation is clear, non-repetitive, and easier to
maintain.
| Tags: cfg.Tags, | ||
| } | ||
|
|
||
| return s.client.CreateStorageAccountIfNecessary(ctx, nil, helperConfig) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pass proper logger instead of nil
The CreateStorageAccountIfNecessary method is being called with nil for the logger parameter, which is inconsistent with logging practices in other parts of the codebase.
- return s.client.CreateStorageAccountIfNecessary(ctx, nil, helperConfig)
+ return s.client.CreateStorageAccountIfNecessary(ctx, log.GetLogger(), helperConfig)📝 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.
| return s.client.CreateStorageAccountIfNecessary(ctx, nil, helperConfig) | |
| return s.client.CreateStorageAccountIfNecessary(ctx, log.GetLogger(), helperConfig) |
🤖 Prompt for AI Agents
In internal/azure/implementations/production.go at line 50, the call to
CreateStorageAccountIfNecessary passes nil for the logger argument, which is
inconsistent with the codebase's logging practices. Replace the nil argument
with the appropriate logger instance available in the current context to ensure
proper logging.
| func (s *StorageAccountServiceImpl) GetStorageAccountSAS(ctx context.Context, resourceGroupName, accountName string) (string, error) { | ||
| return s.client.GetStorageAccountSAS(ctx, "", nil) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement GetStorageAccountSAS properly or document limitations
The method passes empty string and nil parameters to the underlying client method, which might not generate a valid SAS token.
Either implement the method properly by passing the required parameters:
-func (s *StorageAccountServiceImpl) GetStorageAccountSAS(ctx context.Context, resourceGroupName, accountName string) (string, error) {
- return s.client.GetStorageAccountSAS(ctx, "", nil)
-}
+func (s *StorageAccountServiceImpl) GetStorageAccountSAS(ctx context.Context, resourceGroupName, accountName string) (string, error) {
+ // Configure SAS permissions and expiry as needed
+ permissions := "r" // Read permission, adjust as needed
+ expiry := time.Now().Add(time.Hour) // 1 hour expiry, adjust as needed
+ return s.client.GetStorageAccountSAS(ctx, permissions, &expiry)
+}Or return an error indicating the method is not fully implemented:
-func (s *StorageAccountServiceImpl) GetStorageAccountSAS(ctx context.Context, resourceGroupName, accountName string) (string, error) {
- return s.client.GetStorageAccountSAS(ctx, "", nil)
-}
+func (s *StorageAccountServiceImpl) GetStorageAccountSAS(ctx context.Context, resourceGroupName, accountName string) (string, error) {
+ return "", fmt.Errorf("GetStorageAccountSAS not fully implemented: SAS permissions and expiry configuration required")
+}📝 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 (s *StorageAccountServiceImpl) GetStorageAccountSAS(ctx context.Context, resourceGroupName, accountName string) (string, error) { | |
| return s.client.GetStorageAccountSAS(ctx, "", nil) | |
| } | |
| func (s *StorageAccountServiceImpl) GetStorageAccountSAS(ctx context.Context, resourceGroupName, accountName string) (string, error) { | |
| // Configure SAS permissions and expiry as needed | |
| permissions := "r" // Read permission; adjust as needed | |
| expiry := time.Now().Add(time.Hour) // 1 hour expiry; adjust as needed | |
| return s.client.GetStorageAccountSAS(ctx, permissions, &expiry) | |
| } |
🤖 Prompt for AI Agents
In internal/azure/implementations/production.go at lines 172 to 174, the
GetStorageAccountSAS method calls the client with empty string and nil
parameters, which likely results in an invalid SAS token. To fix this, either
implement the method properly by passing the correct resourceGroupName and
accountName parameters to s.client.GetStorageAccountSAS, or modify the method to
return an explicit error indicating that the functionality is not yet
implemented.
| // TestStorageAccountProperties_HasEndpoints tests whether storage account has endpoints configured | ||
| func TestStorageAccountProperties_HasEndpoints(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| properties StorageAccountProperties | ||
| hasPrimary bool | ||
| hasSecond bool | ||
| }{ | ||
| { | ||
| name: "no endpoints", | ||
| properties: StorageAccountProperties{}, | ||
| hasPrimary: false, | ||
| hasSecond: false, | ||
| }, | ||
| { | ||
| name: "only primary endpoints", | ||
| properties: StorageAccountProperties{ | ||
| PrimaryEndpoints: StorageEndpoints{ | ||
| Blob: "https://test.blob.core.windows.net/", | ||
| }, | ||
| }, | ||
| hasPrimary: true, | ||
| hasSecond: false, | ||
| }, | ||
| { | ||
| name: "both endpoints", | ||
| properties: StorageAccountProperties{ | ||
| PrimaryEndpoints: StorageEndpoints{ | ||
| Blob: "https://test.blob.core.windows.net/", | ||
| }, | ||
| SecondaryEndpoints: StorageEndpoints{ | ||
| Blob: "https://test-secondary.blob.core.windows.net/", | ||
| }, | ||
| }, | ||
| hasPrimary: true, | ||
| hasSecond: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| tc := tc | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| hasPrimary := tc.properties.PrimaryEndpoints.Blob != "" || | ||
| tc.properties.PrimaryEndpoints.Queue != "" || | ||
| tc.properties.PrimaryEndpoints.Table != "" || | ||
| tc.properties.PrimaryEndpoints.File != "" | ||
|
|
||
| hasSecondary := tc.properties.SecondaryEndpoints.Blob != "" || | ||
| tc.properties.SecondaryEndpoints.Queue != "" || | ||
| tc.properties.SecondaryEndpoints.Table != "" || | ||
| tc.properties.SecondaryEndpoints.File != "" | ||
|
|
||
| assert.Equal(t, tc.hasPrimary, hasPrimary) | ||
| assert.Equal(t, tc.hasSecond, hasSecondary) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement actual methods for endpoint presence checking.
The test manually checks endpoint presence with duplicated logic. Consider implementing HasPrimaryEndpoints() and HasSecondaryEndpoints() methods on StorageAccountProperties.
Add methods to encapsulate this logic:
// In the types file, add:
+func (p *StorageAccountProperties) HasPrimaryEndpoints() bool {
+ return !p.PrimaryEndpoints.IsEmpty()
+}
+
+func (p *StorageAccountProperties) HasSecondaryEndpoints() bool {
+ return !p.SecondaryEndpoints.IsEmpty()
+}
// In the test, replace manual logic with:
- hasPrimary := tc.properties.PrimaryEndpoints.Blob != "" ||
- tc.properties.PrimaryEndpoints.Queue != "" ||
- tc.properties.PrimaryEndpoints.Table != "" ||
- tc.properties.PrimaryEndpoints.File != ""
-
- hasSecondary := tc.properties.SecondaryEndpoints.Blob != "" ||
- tc.properties.SecondaryEndpoints.Queue != "" ||
- tc.properties.SecondaryEndpoints.Table != "" ||
- tc.properties.SecondaryEndpoints.File != ""
-
- assert.Equal(t, tc.hasPrimary, hasPrimary)
- assert.Equal(t, tc.hasSecond, hasSecondary)
+ assert.Equal(t, tc.hasPrimary, tc.properties.HasPrimaryEndpoints())
+ assert.Equal(t, tc.hasSecond, tc.properties.HasSecondaryEndpoints())Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/azure/types/storage_types_test.go around lines 249 to 309, the test
manually duplicates the logic to check for the presence of primary and secondary
endpoints. To fix this, implement two methods on the StorageAccountProperties
type: HasPrimaryEndpoints() and HasSecondaryEndpoints(), which encapsulate the
logic of checking if any of the Blob, Queue, Table, or File endpoints are
non-empty. Then update the test to call these methods instead of duplicating the
checks.
| // TestStorageEndpoints_IsEmpty tests whether StorageEndpoints can be considered empty | ||
| func TestStorageEndpoints_IsEmpty(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| endpoints StorageEndpoints | ||
| isEmpty bool | ||
| }{ | ||
| { | ||
| name: "all empty", | ||
| endpoints: StorageEndpoints{}, | ||
| isEmpty: true, | ||
| }, | ||
| { | ||
| name: "only blob endpoint", | ||
| endpoints: StorageEndpoints{ | ||
| Blob: "https://test.blob.core.windows.net/", | ||
| }, | ||
| isEmpty: false, | ||
| }, | ||
| { | ||
| name: "all endpoints populated", | ||
| endpoints: StorageEndpoints{ | ||
| Blob: "https://test.blob.core.windows.net/", | ||
| Queue: "https://test.queue.core.windows.net/", | ||
| Table: "https://test.table.core.windows.net/", | ||
| File: "https://test.file.core.windows.net/", | ||
| }, | ||
| isEmpty: false, | ||
| }, | ||
| { | ||
| name: "partial endpoints", | ||
| endpoints: StorageEndpoints{ | ||
| Blob: "https://test.blob.core.windows.net/", | ||
| File: "https://test.file.core.windows.net/", | ||
| }, | ||
| isEmpty: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| tc := tc | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| isEmpty := tc.endpoints.Blob == "" && | ||
| tc.endpoints.Queue == "" && | ||
| tc.endpoints.Table == "" && | ||
| tc.endpoints.File == "" | ||
|
|
||
| assert.Equal(t, tc.isEmpty, isEmpty) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement an actual IsEmpty() method on StorageEndpoints.
The test duplicates logic that should be encapsulated in the StorageEndpoints type. This makes the code harder to maintain and doesn't test the actual API users would interact with.
Consider implementing an IsEmpty() method on StorageEndpoints:
// In the types file, add:
+func (e *StorageEndpoints) IsEmpty() bool {
+ return e.Blob == "" && e.Queue == "" && e.Table == "" && e.File == ""
+}
// In the test, replace manual logic with:
- isEmpty := tc.endpoints.Blob == "" &&
- tc.endpoints.Queue == "" &&
- tc.endpoints.Table == "" &&
- tc.endpoints.File == ""
-
- assert.Equal(t, tc.isEmpty, isEmpty)
+ assert.Equal(t, tc.isEmpty, tc.endpoints.IsEmpty())🤖 Prompt for AI Agents
In internal/azure/types/storage_types_test.go around lines 94 to 148, the test
duplicates the logic to check if StorageEndpoints is empty instead of using a
method on the StorageEndpoints type. Implement an IsEmpty() method on the
StorageEndpoints struct that returns true if all endpoint fields (Blob, Queue,
Table, File) are empty strings. Then update the test to call this IsEmpty()
method instead of duplicating the empty check logic.
| // TestStorageAccountConfig_Validation tests the StorageAccountConfig struct validation | ||
| func TestStorageAccountConfig_Validation(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| config StorageAccountConfig | ||
| valid bool | ||
| }{ | ||
| { | ||
| name: "valid minimal config", | ||
| config: StorageAccountConfig{ | ||
| Name: "validstorageaccount", | ||
| ResourceGroupName: "test-rg", | ||
| Location: "eastus", | ||
| }, | ||
| valid: true, | ||
| }, | ||
| { | ||
| name: "valid full config", | ||
| config: StorageAccountConfig{ | ||
| Name: "validstorageaccount", | ||
| ResourceGroupName: "test-rg", | ||
| Location: "eastus", | ||
| EnableVersioning: true, | ||
| AllowBlobPublicAccess: false, | ||
| AccountKind: AccountKind("StorageV2"), | ||
| AccountTier: AccountTier("Standard"), | ||
| AccessTier: AccessTier("Hot"), | ||
| ReplicationType: ReplicationType("LRS"), | ||
| Tags: map[string]string{ | ||
| "Environment": "test", | ||
| "Owner": "terraform", | ||
| }, | ||
| }, | ||
| valid: true, | ||
| }, | ||
| { | ||
| name: "empty name", | ||
| config: StorageAccountConfig{ | ||
| Name: "", | ||
| ResourceGroupName: "test-rg", | ||
| Location: "eastus", | ||
| }, | ||
| valid: false, | ||
| }, | ||
| { | ||
| name: "empty resource group", | ||
| config: StorageAccountConfig{ | ||
| Name: "validstorageaccount", | ||
| ResourceGroupName: "", | ||
| Location: "eastus", | ||
| }, | ||
| valid: false, | ||
| }, | ||
| { | ||
| name: "empty location", | ||
| config: StorageAccountConfig{ | ||
| Name: "validstorageaccount", | ||
| ResourceGroupName: "test-rg", | ||
| Location: "", | ||
| }, | ||
| valid: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| tc := tc | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Test basic field presence validation | ||
| hasRequiredFields := tc.config.Name != "" && | ||
| tc.config.ResourceGroupName != "" && | ||
| tc.config.Location != "" | ||
|
|
||
| if tc.valid { | ||
| assert.True(t, hasRequiredFields, "Valid config should have required fields") | ||
| } else { | ||
| assert.False(t, hasRequiredFields, "Invalid config should be missing required fields") | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing actual validation methods instead of testing manual logic.
The test is manually checking field presence rather than calling a Validate() method on the StorageAccountConfig struct. This approach duplicates validation logic in tests and doesn't test the actual behavior users would experience.
Consider implementing a Validate() method on StorageAccountConfig and testing that instead:
// In the types file, add:
+func (c *StorageAccountConfig) Validate() error {
+ if c.Name == "" {
+ return errors.New("storage account name is required")
+ }
+ if c.ResourceGroupName == "" {
+ return errors.New("resource group name is required")
+ }
+ if c.Location == "" {
+ return errors.New("location is required")
+ }
+ return nil
+}
// In the test, replace manual logic with:
- hasRequiredFields := tc.config.Name != "" &&
- tc.config.ResourceGroupName != "" &&
- tc.config.Location != ""
-
- if tc.valid {
- assert.True(t, hasRequiredFields, "Valid config should have required fields")
- } else {
- assert.False(t, hasRequiredFields, "Invalid config should be missing required fields")
- }
+ err := tc.config.Validate()
+ if tc.valid {
+ assert.NoError(t, err, "Valid config should not return error")
+ } else {
+ assert.Error(t, err, "Invalid config should return error")
+ }🤖 Prompt for AI Agents
In internal/azure/types/storage_types_test.go around lines 9 to 92, the test
manually checks field presence instead of calling a Validate() method on
StorageAccountConfig, duplicating validation logic and not testing real
behavior. Implement a Validate() method on StorageAccountConfig that
encapsulates the validation rules, then update the test to call this Validate()
method and assert its results, ensuring the test verifies the actual validation
logic used by users.
Description
Terragrunt supports using Azure Storage accounts as a backend for Terraform/OpenTofu remote state. With this feature, Terragrunt can automatically create and manage Azure Storage accounts and containers for storing state files, using secure defaults such as Azure AD authentication, blob versioning, and RBAC access control. The backend supports configuration options for storage account details, container name, state file key, and advanced settings like tags, replication, and access tier. Authentication is handled via Azure AD (default), Managed Identity, or Service Principal. This enables secure, automated, and reproducible management of remote state on Azure.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added experimental support to manage Azure Storage as remote state backend.
Migration Guide
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
Refactor