feat: allow plan persistence also for Azure environments through Azure storage accounts #1933
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR adds Azure plan persistence functionality by integrating Azure Blob Storage support and updating configuration options and documentation accordingly.
- Updated
/action.ymlto include new inputs:upload-plan-destination-azure-containerandupload-plan-destination-azure-storage-account. - Modified documentation in
/docs/ce/reference/action-inputs.mdxand/docs/ce/howto/store-plans-in-a-bucket.mdx; note naming mismatch (uses "azure-bucket" in docs). - Extended
/libs/storage/plan_storage.goto include Azure support and added/libs/storage/azure_plan_storage.gowith azblob integration. - Updated Go module dependencies in
/libs/go.modand/go.work.sumfor Azure SDK compatibility.
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
7 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
WalkthroughThe changes integrate Azure Blob Storage as an additional destination for uploading Terraform plan outputs. The modifications update configuration inputs in the action, enhance documentation to include Azure-specific instructions, and adjust dependency versions to support Azure SDK features. New code files provide methods to interact with Azure Blob Storage for plan existence checks, uploads, retrievals, and deletions. Additionally, the plan storage logic now retrieves the necessary Azure configuration from the environment and establishes connections with Azure services. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PS as PlanStorage (NewPlanStorage)
participant Env as Environment
participant AZCred as Azure SDK (azidentity)
participant AZClient as Azure SDK (azblob)
User->>PS: Request new storage instance ("azure")
PS->>Env: Retrieve container & account environment variables
PS->>AZCred: Create default Azure credentials
AZCred-->>PS: Return credentials
PS->>AZClient: Initialize Azure Blob client with account & credentials
AZClient-->>PS: Return blob client
PS->>PS: Instantiate PlanStorageAzure with client & container
PS-->>User: Return PlanStorageAzure instance
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
libs/storage/plan_storage.go (1)
396-429: Azure storage implementation follows consistent patterns.The Azure storage implementation correctly follows the same patterns as the existing providers:
- Validates required parameters are present
- Sets up authentication credentials
- Creates the client connection
- Initializes the appropriate storage implementation
The implementation properly handles error conditions with clear error messages and logging.
However, there's a minor naming convention issue:
- sa_name := strings.ToLower(os.Getenv("PLAN_UPLOAD_AZURE_STORAGE_ACCOUNT_NAME")) + accountName := strings.ToLower(os.Getenv("PLAN_UPLOAD_AZURE_STORAGE_ACCOUNT_NAME"))Consider using camelCase for variable names to be consistent with Go conventions and the rest of the codebase.
libs/storage/azure_plan_storage.go (4)
19-44: Fix context inconsistency in PlanExists methodThe method uses
context.TODO()instead of the context from the struct (psa.Context). For consistency with other methods, you should use the struct's context.- resp, err := blobClient.GetProperties(context.TODO(), nil) + resp, err := blobClient.GetProperties(psa.Context, nil)
1-147: Consider adding retry logic for cloud operationsCloud operations can be prone to transient failures. Consider implementing retry logic for Azure Blob Storage operations to improve reliability.
You could use the Azure SDK's built-in retry policy or implement a simple exponential backoff:
// Example of how to configure retry options with Azure SDK client, err := azblob.NewClientWithSharedKeyCredential(serviceURL, credential, &azblob.ClientOptions{ Retry: azcore.RetryOptions{ MaxRetries: 3, RetryDelay: 2 * time.Second, MaxRetryDelay: 10 * time.Second, }, })
75-119: Consider validating the file path before creating the local fileIt would be beneficial to validate that the local file path directory exists or create it if it doesn't before attempting to create the file.
+ // Ensure the directory exists + dir := filepath.Dir(localPlanFilePath) + if err := os.MkdirAll(dir, 0755); err != nil { + slog.Error("Unable to create directory for local file", + "directory", dir, + "error", err) + return nil, fmt.Errorf("unable to create directory: %w", err) + } localFile, err := os.Create(localPlanFilePath)
46-73: Enhance error messages with error wrappingUse
%wformat verb withfmt.Errorfto preserve the original error for better error handling up the call stack.Example for improvement:
- return nil, fmt.Errorf("unable to read data from blob: %v", err) + return nil, fmt.Errorf("unable to read data from blob: %w", err)Also applies to: 75-119, 121-146
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumlibs/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
action.yml(4 hunks)docs/ce/howto/store-plans-in-a-bucket.mdx(1 hunks)docs/ce/reference/action-inputs.mdx(1 hunks)libs/go.mod(4 hunks)libs/storage/azure_plan_storage.go(1 hunks)libs/storage/plan_storage.go(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
libs/storage/plan_storage.go (1)
libs/storage/azure_plan_storage.go (1)
PlanStorageAzure(13-17)
🔇 Additional comments (16)
docs/ce/howto/store-plans-in-a-bucket.mdx (1)
34-44: Good addition of Azure storage documentation.The Azure section follows the same format as the other cloud providers and provides clear instructions for users to configure plan persistence with Azure storage accounts. The parameters are correctly specified according to the changes in action.yml and align with the PR objectives to enable plan persistence for Azure environments.
docs/ce/reference/action-inputs.mdx (2)
87-87: Updated description to include Azure as a supported destination.The description now accurately reflects the addition of Azure as a supported destination for plan uploads.
92-97: Azure storage parameters documented consistently.The new Azure parameters are appropriately defined with clear descriptions that instruct users when these parameters should be provided. The documentation follows the same pattern as existing parameters for other cloud providers.
action.yml (4)
98-98: Updated description to include Azure as a supported destination.The description now correctly includes Azure in the list of supported destinations for plan uploads.
114-119: Azure storage parameters correctly defined.The new input parameters for Azure blob storage are well defined with appropriate descriptions. The parameters follow the naming convention used for other cloud providers.
391-392: Environment variables for Azure correctly mapped.The Azure storage container name and account name from inputs are properly mapped to environment variables that will be used by the implementation code.
434-435: Environment variables for Azure correctly mapped (for versioned action).The Azure environment variables are properly mapped in both build and run digger sections, ensuring consistent behavior regardless of how the action is invoked.
libs/storage/plan_storage.go (1)
18-19: Azure SDK imports correctly added.The required Azure SDK imports for blob storage and identity authentication are properly included to support the new Azure storage functionality.
libs/go.mod (4)
7-9: Dependencies properly updated for Azure Blob Storage integrationThe changes appropriately add and update Azure SDK dependencies necessary for Azure Blob Storage support. The addition of
github.com/Azure/azure-sdk-for-go/sdk/storage/azblob v1.6.0is essential for the new Azure plan persistence functionality.
57-58: Azure SDK core dependencies properly updatedThe Azure SDK core dependencies have been updated to compatible versions to support the new Azure Blob Storage functionality.
69-69: Microsoft Authentication Library correctly updatedThe Microsoft Authentication Library has been properly updated to work with the latest Azure SDK versions.
37-37: Test library version update looks goodThe update to the testify library is a minor version update and should not cause any compatibility issues.
libs/storage/azure_plan_storage.go (4)
13-17: Implementation of Azure storage structure looks goodThe
PlanStorageAzurestruct properly includes the necessary components for Azure Blob Storage interaction: a service client, container name, and context.
46-73: StorePlanFile implementation is correct and efficientThe method correctly uses the Azure Blob SDK to upload the file contents as a buffer, with proper error handling and logging.
75-119: RetrievePlan method includes proper file handlingThe implementation correctly creates a local file, downloads the blob content, handles errors appropriately, and cleans up resources with
defer.
121-146: DeleteStoredPlan implementation is correctThe method properly handles the deletion of blobs with appropriate error handling and logging.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
libs/storage/azure_plan_storage.go (4)
1-9: Consider streamlined import grouping and version pinning
It's good to see the Azure SDK imports, but if your organization enforces pinned versions, consider explicitly pinning them in yourgo.modfile to guard against breaking changes in newer releases.
46-73: Validate plan file name or path
To strengthen security and avoid path injection, consider validating or sanitizingfileNameandartifactNamebefore using them in upload operations, especially if they can be externally influenced.
75-119: Beware of partial downloads on large plan files
UsingDownloadFileinto anos.Createoutput can block if the file is very large or network conditions degrade. If plan files can be large, consider a streaming approach or chunked reads, plus handling partial downloads with retries or resumable transfers.
121-146: Deletion grace checks
Consider verifying whether a plan file exists prior to attemptingDeleteStoredPlanto avoid confusion or potential false alarms in logs. Though your current code simply attempts deletion, you might provide more user-friendly messages if the file does not exist.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/storage/azure_plan_storage.go(1 hunks)libs/storage/plan_storage.go(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
libs/storage/plan_storage.go (1)
libs/storage/azure_plan_storage.go (1)
PlanStorageAzure(13-17)
🔇 Additional comments (2)
libs/storage/azure_plan_storage.go (1)
13-17: Struct usage is clear and well-defined
Storing theServiceClient,ContainerName, andContextin a single struct is straightforward and fosters maintainability by consolidating Azure-related references.libs/storage/plan_storage.go (1)
18-19: Imports for Azure SDK
Adding the Azure SDK aligns well with the new Azure plan storage feature. Confirm that yourgo.modand CI pipelines reflect these dependencies so they remain stable over time.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/storage/azure_plan_storage.go (2)
55-82: Storage implementation looks good, minor style suggestions.The implementation correctly uses the Azure Blob Storage API for uploading plan files. The logging is thorough and provides good context for debugging.
There's some extra whitespace at line 69 that could be removed for consistency.
) - if err != nil {
84-128: Consider adding input validation for file paths.The implementation correctly downloads a plan from Azure Blob Storage to a local file. However, consider adding validation for the input paths before attempting to create or access files.
func (psa *PlanStorageAzure) RetrievePlan(localPlanFilePath string, artifactName string, storedPlanFilePath string) (*string, error) { + if localPlanFilePath == "" || storedPlanFilePath == "" { + return nil, fmt.Errorf("local or stored plan file path cannot be empty") + } + slog.Debug("Retrieving plan from Azure Blob Storage", "container", psa.ContainerName, "path", storedPlanFilePath, "artifactName", artifactName, "localPath", localPlanFilePath)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/go.mod(4 hunks)libs/storage/azure_plan_storage.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/go.mod
🔇 Additional comments (4)
libs/storage/azure_plan_storage.go (4)
1-13: The imports look appropriate for Azure Blob Storage operations.The imports include all necessary Azure SDK packages and standard Go libraries required for the implementation. Good job including the specific Azure blob error package for more precise error handling.
15-19: Well-structured type declaration for Azure integration.The
PlanStorageAzurestruct contains all the necessary fields for Azure Blob Storage operations: a service client for API interactions, a container name for targeting the correct storage location, and a context for operation management.
21-53: Consider usingerrors.As()for safer error type checking.The current implementation uses type assertion (
err.(*azcore.ResponseError)) which can panic if the error is not of the expected type. Consider using the safererrors.As()method instead.if err != nil { - if azErr, ok := err.(*azcore.ResponseError); ok && string(azErr.ErrorCode) == string(bloberror.BlobNotFound) { + var azErr *azcore.ResponseError + if errors.As(err, &azErr) && string(azErr.ErrorCode) == string(bloberror.BlobNotFound) { slog.Debug("Blob not found", "container", psa.ContainerName, "path", storedPlanFilePath, "artifactName", artifactName) return false, nil }Also, don't forget to add the "errors" package to the imports.
130-155: Delete operation implementation is robust.The delete method properly handles the deletion of a plan from Azure Blob Storage with adequate error handling and informative logging at each step. The error message provides clear context about what went wrong if the operation fails.
|
@motatoes, could you look at this change? |
|
Thanks @wenzel-felix ! Very valuable contribution 💙 |
This PR implements the functionality of plan persistence for the Azure provider. Currently this is only supported for AWS and GCP.
It adds two new configuration options:
Summary by CodeRabbit