Adding Azure Keyvault support for Stores #963
Conversation
📝 WalkthroughWalkthroughThis pull request introduces Azure Key Vault as a new secret store implementation for the project. The changes include adding Azure SDK dependencies to the Changes
Suggested Labels
Suggested Reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
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: 3
🧹 Nitpick comments (4)
pkg/store/azure_keyvault_store.go (3)
83-89: Refine secret name normalization for consistency.Replacing
"--"with"-"may not handle cases with more than two consecutive dashes. Using a regular expression to replace multiple dashes ensures consistent normalization of thesecretName.Apply this diff to enhance the handling:
- secretName = strings.ReplaceAll(secretName, "--", "-") + secretName = regexp.MustCompile("-+").ReplaceAllString(secretName, "-")
162-179: Improve the exponential backoff strategy in retries.The current backoff increases linearly, which might not be optimal for handling transient errors. Implementing an exponential backoff can provide better performance and resource utilization during retries.
Modify the sleep duration to implement exponential backoff:
// Add exponential backoff for retries - time.Sleep(time.Duration(i+1) * time.Second) + sleepDuration := time.Duration(math.Pow(2, float64(i))) * time.Second + time.Sleep(sleepDuration)Don't forget to import the
mathpackage:+ "math"
125-138: Consider adding context with timeout to theSetmethod.Including a context with a timeout, similar to the
Getmethod, can prevent potential hangs and improve responsiveness during secret storage operations.Apply this diff to incorporate context with timeout:
func (s *KeyVaultStore) Set(stack string, component string, key string, value interface{}) error { // Existing code... + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + _, err = s.client.SetSecret(ctx, secretName, params, nil) if err != nil { var respErr *azcore.ResponseError // Error handling...website/docs/core-concepts/projects/configuration/stores.mdx (1)
203-206: Consider these typographical improvements:
- Use typographic quotes (") instead of straight quotes (") for the parameters.
- Use an en dash (–) instead of a hyphen (-) in the range "1-127".
Also applies to: 216-216
🧰 Tools
🪛 LanguageTool
[typographical] ~203-~203: Consider using a typographic opening quote here.
Context: ...ample, with these parameters: - prefix: "myapp" - stack: "prod-us-west" - compone...(EN_QUOTES)
[typographical] ~203-~203: Consider using a typographic close quote here.
Context: ... with these parameters: - prefix: "myapp" - stack: "prod-us-west" - component: "d...(EN_QUOTES)
[typographical] ~204-~204: Consider using a typographic opening quote here.
Context: ... parameters: - prefix: "myapp" - stack: "prod-us-west" - component: "database/pos...(EN_QUOTES)
[typographical] ~204-~204: Consider using a typographic close quote here.
Context: ...- prefix: "myapp" - stack: "prod-us-west" - component: "database/postgres" - key:...(EN_QUOTES)
[typographical] ~205-~205: Consider using a typographic opening quote here.
Context: ...p" - stack: "prod-us-west" - component: "database/postgres" - key: "password" Th...(EN_QUOTES)
[typographical] ~205-~205: Consider using a typographic close quote here.
Context: ...us-west" - component: "database/postgres" - key: "password" The resulting Key Va...(EN_QUOTES)
[typographical] ~206-~206: Consider using typographic quotation marks here.
Context: ...- component: "database/postgres" - key: "password" The resulting Key Vault secret name wo...(EN_QUOTES)
📜 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 (5)
go.mod(5 hunks)pkg/store/azure_keyvault_store.go(1 hunks)pkg/store/azure_keyvault_store_test.go(1 hunks)pkg/store/registry.go(1 hunks)website/docs/core-concepts/projects/configuration/stores.mdx(2 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/projects/configuration/stores.mdx
[typographical] ~203-~203: Consider using a typographic opening quote here.
Context: ...ample, with these parameters: - prefix: "myapp" - stack: "prod-us-west" - compone...
(EN_QUOTES)
[typographical] ~203-~203: Consider using a typographic close quote here.
Context: ... with these parameters: - prefix: "myapp" - stack: "prod-us-west" - component: "d...
(EN_QUOTES)
[typographical] ~204-~204: Consider using a typographic opening quote here.
Context: ... parameters: - prefix: "myapp" - stack: "prod-us-west" - component: "database/pos...
(EN_QUOTES)
[typographical] ~204-~204: Consider using a typographic close quote here.
Context: ...- prefix: "myapp" - stack: "prod-us-west" - component: "database/postgres" - key:...
(EN_QUOTES)
[typographical] ~205-~205: Consider using a typographic opening quote here.
Context: ...p" - stack: "prod-us-west" - component: "database/postgres" - key: "password" Th...
(EN_QUOTES)
[typographical] ~205-~205: Consider using a typographic close quote here.
Context: ...us-west" - component: "database/postgres" - key: "password" The resulting Key Va...
(EN_QUOTES)
[typographical] ~206-~206: Consider using typographic quotation marks here.
Context: ...- component: "database/postgres" - key: "password" The resulting Key Vault secret name wo...
(EN_QUOTES)
[typographical] ~216-~216: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...dashes - Are case-insensitive - Must be 1-127 characters long The store automaticall...
(HYPHEN_TO_EN)
🪛 GitHub Actions: Dependency Review
go.mod
[error] Incompatible license detected: Package 'github.com/pkg/[email protected]' uses BSD-2-Clause license which is not compatible with project requirements
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
pkg/store/registry.go (1)
42-53: Integration looks solid.The addition of the Azure Key Vault case in the registry is clear and follows the existing pattern.
website/docs/core-concepts/projects/configuration/stores.mdx (4)
20-21: LGTM! Clear and concise introduction of Azure Key Vault support.The configuration example is well-structured and includes all necessary fields with proper formatting.
Also applies to: 119-131
155-170: Well-documented authentication methods!The authentication documentation clearly outlines all supported methods and provides appropriate recommendations for different environments.
171-193: Excellent examples that demonstrate practical usage!The examples effectively cover both basic secret retrieval and component output storage scenarios.
195-235: Comprehensive documentation of key naming and error handling!The documentation clearly explains the key name construction rules and provides helpful error messages.
🧰 Tools
🪛 LanguageTool
[typographical] ~203-~203: Consider using a typographic opening quote here.
Context: ...ample, with these parameters: - prefix: "myapp" - stack: "prod-us-west" - compone...(EN_QUOTES)
[typographical] ~203-~203: Consider using a typographic close quote here.
Context: ... with these parameters: - prefix: "myapp" - stack: "prod-us-west" - component: "d...(EN_QUOTES)
[typographical] ~204-~204: Consider using a typographic opening quote here.
Context: ... parameters: - prefix: "myapp" - stack: "prod-us-west" - component: "database/pos...(EN_QUOTES)
[typographical] ~204-~204: Consider using a typographic close quote here.
Context: ...- prefix: "myapp" - stack: "prod-us-west" - component: "database/postgres" - key:...(EN_QUOTES)
[typographical] ~205-~205: Consider using a typographic opening quote here.
Context: ...p" - stack: "prod-us-west" - component: "database/postgres" - key: "password" Th...(EN_QUOTES)
[typographical] ~205-~205: Consider using a typographic close quote here.
Context: ...us-west" - component: "database/postgres" - key: "password" The resulting Key Va...(EN_QUOTES)
[typographical] ~206-~206: Consider using typographic quotation marks here.
Context: ...- component: "database/postgres" - key: "password" The resulting Key Vault secret name wo...(EN_QUOTES)
[typographical] ~216-~216: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...dashes - Are case-insensitive - Must be 1-127 characters long The store automaticall...(HYPHEN_TO_EN)
| func TestKeyVaultStore_InputValidation(t *testing.T) { | ||
| store := &KeyVaultStore{ | ||
| prefix: "prefix", | ||
| stackDelimiter: new(string), | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| stack string | ||
| component string | ||
| key string | ||
| value interface{} | ||
| operation string | ||
| wantError bool | ||
| }{ | ||
| { | ||
| name: "empty stack", | ||
| stack: "", | ||
| component: "app", | ||
| key: "config", | ||
| value: "test", | ||
| operation: "set", | ||
| wantError: true, | ||
| }, | ||
| { | ||
| name: "empty component", | ||
| stack: "dev", | ||
| component: "", | ||
| key: "config", | ||
| value: "test", | ||
| operation: "set", | ||
| wantError: true, | ||
| }, | ||
| { | ||
| name: "empty key", | ||
| stack: "dev", | ||
| component: "app", | ||
| key: "", | ||
| value: "test", | ||
| operation: "set", | ||
| wantError: true, | ||
| }, | ||
| { | ||
| name: "non-string value", | ||
| stack: "dev", | ||
| component: "app", | ||
| key: "config", | ||
| value: 123, | ||
| operation: "set", | ||
| wantError: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| var err error | ||
| if tt.operation == "set" { | ||
| err = store.Set(tt.stack, tt.component, tt.key, tt.value) | ||
| } else { | ||
| _, err = store.Get(tt.stack, tt.component, tt.key) | ||
| } | ||
|
|
||
| if tt.wantError { | ||
| assert.Error(t, err) | ||
| } else { | ||
| assert.NoError(t, err) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Initialize the Key Vault client or mock it in tests.
In the TestKeyVaultStore_InputValidation, the store lacks an initialized client, leading to potential nil pointer dereferences during tests. Incorporating a mock client or properly initializing it is essential for reliable testing.
Consider mocking the client or initializing it appropriately in your test setup.
| result := store.getKey(tt.stack, tt.component, tt.key) | ||
| assert.Equal(t, tt.expected, result) | ||
| }) |
There was a problem hiding this comment.
Handle errors returned by getKey in tests.
The getKey function returns an error, but the test doesn't check for it. Capturing and asserting the error ensures thorough test coverage.
Apply this diff to handle the error:
- result := store.getKey(tt.stack, tt.component, tt.key)
+ result, err := store.getKey(tt.stack, tt.component, tt.key)
+ assert.NoError(t, err)Committable suggestion skipped: line range outside the PR's diff.
| @@ -235,6 +243,7 @@ require ( | |||
| github.com/pierrec/lz4 v2.6.1+incompatible // indirect | |||
| github.com/pierrec/lz4/v4 v4.1.21 // indirect | |||
| github.com/pjbgf/sha1cd v0.3.0 // indirect | |||
| github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c // indirect | |||
There was a problem hiding this comment.
Address the license compatibility issue.
The github.com/pkg/browser package uses the BSD-2-Clause license, which is incompatible with the project requirements according to the dependency review.
Consider:
- Finding an alternative package with a compatible license
- Implementing the required browser functionality internally
- Getting legal approval for using the BSD-2-Clause license
🧰 Tools
🪛 GitHub Actions: Dependency Review
[error] Incompatible license detected: Package 'github.com/pkg/[email protected]' uses BSD-2-Clause license which is not compatible with project requirements
There was a problem hiding this comment.
@jamengual thanks for taking a stab at this!
The pluggable store MUST implement the store interface in order to be used as a store. It looks like you changed the interface that it's implementing here. Could you take a shot at implementing that?
Also, please take a look at one of the other store implementations to see how we mock the calls to the underlying cloud provider in tests so we don't need to have each setup.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/store/azure_keyvault_store_test.go (1)
112-114:⚠️ Potential issueHandle errors returned by
getKeyfunctionThe
getKeyfunction returns a(string, error), but the error isn't checked in the test. Capturing and asserting the error enhances test robustness.Consider applying this change:
- result := store.getKey(tt.stack, tt.component, tt.key) - assert.Equal(t, tt.expected, result) + result, err := store.getKey(tt.stack, tt.component, tt.key) + assert.NoError(t, err) + assert.Equal(t, tt.expected, result)
🧹 Nitpick comments (3)
pkg/store/azure_keyvault_store.go (3)
77-101: Simplify the replacement of multiple dashes in secret namesIn the
getKeyfunction, replacing multiple consecutive dashes usingstrings.ReplaceAllmight not handle more than two dashes effectively. Consider using a regular expression to replace any sequence of dashes with a single dash.Here's a possible improvement:
- secretName = strings.ReplaceAll(secretName, "--", "-") + re := regexp.MustCompile("-+") + secretName = re.ReplaceAllString(secretName, "-")
149-193: Usedeferto ensure the context is canceled properlyIn the
Getmethod, callingcancel()immediately afterGetSecretmight not cover all execution paths, especially if future modifications are made. Usingdefer cancel()ensures the context is canceled when the function returns.Here's how you might adjust the code:
- ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel()
114-118: Enhance value type handling inSetmethodCurrently, the
Setmethod only accepts string values. If there's a need to store other types, consider converting them to strings or handling different types appropriately.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/store/azure_keyvault_store.go(1 hunks)pkg/store/azure_keyvault_store_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/store/azure_keyvault_store_test.go (1)
122-125: Great job initializing the mock clientIn
TestKeyVaultStore_InputValidation, initializing theclientwith a mock ensures the tests run without nil pointer dereferences.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
pkg/store/azure_keyvault_store.go (2)
23-29: Consider adding validation for stackDelimiter.The
KeyVaultStorestruct looks good, but consider adding validation in the constructor to ensurestackDelimiteris not empty when initialized.
125-146: Add retry mechanism for transient failures.The Get operation should implement a retry mechanism for handling transient failures in Azure Key Vault operations.
+func (s *KeyVaultStore) Get(stack string, component string, key string) (interface{}, error) { + const maxRetries = 3 + var lastErr error + + for i := 0; i < maxRetries; i++ { + value, err := s.tryGet(stack, component, key) + if err == nil { + return value, nil + } + + var respErr *azcore.ResponseError + if errors.As(err, &respErr) && respErr.StatusCode >= 500 { + lastErr = err + time.Sleep(time.Duration(i+1) * time.Second) + continue + } + return nil, err + } + return nil, fmt.Errorf("failed after %d retries: %w", maxRetries, lastErr) +}pkg/store/azure_keyvault_store_test.go (2)
18-21: Return mock response data in SetSecret.The mock
SetSecretmethod should return meaningful response data for better test coverage.func (m *MockKeyVaultClient) SetSecret(ctx context.Context, name string, parameters azsecrets.SetSecretParameters, options *azsecrets.SetSecretOptions) (azsecrets.SetSecretResponse, error) { args := m.Called(ctx, name, parameters) - return azsecrets.SetSecretResponse{}, args.Error(1) + if args.Get(0) == nil { + return azsecrets.SetSecretResponse{}, args.Error(1) + } + return args.Get(0).(azsecrets.SetSecretResponse), args.Error(1) }
66-110: Add test cases for complex stack paths.The
TestKeyVaultStore_getKeyshould include additional test cases for:
- Stack paths with multiple segments
- Component paths with nested structures
- Keys containing special characters
+ { + name: "complex path", + stack: "prod-east", + component: "service/api/v1", + key: "database.credentials", + expected: "prefix-prod-east-service-api-v1-database-credentials", + wantErr: false, + },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/store/azure_keyvault_store.go(1 hunks)pkg/store/azure_keyvault_store_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/store/azure_keyvault_store.go (1)
17-21: LGTM! Well-defined interface for mocking.The
KeyVaultClientinterface is well-designed for testing purposes, making it easy to mock Azure Key Vault operations.
| func (s *KeyVaultStore) Set(stack string, component string, key string, value interface{}) error { | ||
| secretName, err := s.getKey(stack, component, key) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| strValue, ok := value.(string) | ||
| if !ok { | ||
| return fmt.Errorf("value must be a string") | ||
| } | ||
|
|
||
| params := azsecrets.SetSecretParameters{ | ||
| Value: &strValue, | ||
| } | ||
|
|
||
| _, err = s.client.SetSecret(context.Background(), secretName, params, nil) | ||
| if err != nil { | ||
| var respErr *azcore.ResponseError | ||
| if errors.As(err, &respErr) && respErr.StatusCode == 403 { | ||
| return fmt.Errorf("failed to set secret '%s': access denied. Please check your Azure credentials and permissions: %w", secretName, err) | ||
| } | ||
| return fmt.Errorf("failed to set secret '%s': %w", secretName, err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add context timeout for Azure operations.
The Set operation uses context.Background() without a timeout. Consider adding a reasonable timeout to prevent hanging operations.
- _, err = s.client.SetSecret(context.Background(), secretName, params, nil)
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
+ _, err = s.client.SetSecret(ctx, secretName, params, nil)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *KeyVaultStore) Set(stack string, component string, key string, value interface{}) error { | |
| secretName, err := s.getKey(stack, component, key) | |
| if err != nil { | |
| return err | |
| } | |
| strValue, ok := value.(string) | |
| if !ok { | |
| return fmt.Errorf("value must be a string") | |
| } | |
| params := azsecrets.SetSecretParameters{ | |
| Value: &strValue, | |
| } | |
| _, err = s.client.SetSecret(context.Background(), secretName, params, nil) | |
| if err != nil { | |
| var respErr *azcore.ResponseError | |
| if errors.As(err, &respErr) && respErr.StatusCode == 403 { | |
| return fmt.Errorf("failed to set secret '%s': access denied. Please check your Azure credentials and permissions: %w", secretName, err) | |
| } | |
| return fmt.Errorf("failed to set secret '%s': %w", secretName, err) | |
| } | |
| return nil | |
| } | |
| func (s *KeyVaultStore) Set(stack string, component string, key string, value interface{}) error { | |
| secretName, err := s.getKey(stack, component, key) | |
| if err != nil { | |
| return err | |
| } | |
| strValue, ok := value.(string) | |
| if !ok { | |
| return fmt.Errorf("value must be a string") | |
| } | |
| params := azsecrets.SetSecretParameters{ | |
| Value: &strValue, | |
| } | |
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| defer cancel() | |
| _, err = s.client.SetSecret(ctx, secretName, params, nil) | |
| if err != nil { | |
| var respErr *azcore.ResponseError | |
| if errors.As(err, &respErr) && respErr.StatusCode == 403 { | |
| return fmt.Errorf("failed to set secret '%s': access denied. Please check your Azure credentials and permissions: %w", secretName, err) | |
| } | |
| return fmt.Errorf("failed to set secret '%s': %w", secretName, err) | |
| } | |
| return nil | |
| } |
| func (s *KeyVaultStore) getKey(stack string, component string, key string) (string, error) { | ||
| if stack == "" || component == "" || key == "" { | ||
| return "", fmt.Errorf("stack, component, and key cannot be empty") | ||
| } | ||
|
|
||
| stackParts := strings.Split(stack, *s.stackDelimiter) | ||
| componentParts := strings.Split(component, "/") | ||
|
|
||
| parts := append([]string{s.prefix}, stackParts...) | ||
| parts = append(parts, componentParts...) | ||
| parts = append(parts, key) | ||
|
|
||
| // Azure Key Vault secret names can only contain alphanumeric characters and dashes | ||
| secretName := strings.ToLower(strings.Join(parts, "-")) | ||
| secretName = strings.Trim(secretName, "-") | ||
|
|
||
| if len(secretName) > 127 { | ||
| return "", fmt.Errorf("generated secret name exceeds Azure Key Vault's 127-character limit: %s (%d characters)", secretName, len(secretName)) | ||
| } | ||
|
|
||
| return secretName, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance key validation for Azure Key Vault naming rules.
While the function correctly handles length validation, Azure Key Vault has additional naming rules that should be validated:
- Characters should be limited to 0-9, a-z, A-Z, and hyphen
- Secret name cannot start with a number
func (s *KeyVaultStore) getKey(stack string, component string, key string) (string, error) {
if stack == "" || component == "" || key == "" {
return "", fmt.Errorf("stack, component, and key cannot be empty")
}
+ // Validate input components against Azure Key Vault naming rules
+ if match := regexp.MustCompile(`^[0-9]`).MatchString(stack); match {
+ return "", fmt.Errorf("stack cannot start with a number")
+ }Committable suggestion skipped: line range outside the PR's diff.
| func TestKeyVaultStore_Set(t *testing.T) { | ||
| mockClient := new(MockKeyVaultClient) | ||
| delimiter := "-" | ||
| store := &KeyVaultStore{ | ||
| client: mockClient, | ||
| prefix: "prefix", | ||
| stackDelimiter: &delimiter, | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| stack string | ||
| component string | ||
| key string | ||
| value interface{} | ||
| mockFn func() | ||
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "valid set", | ||
| stack: "dev", | ||
| component: "app", | ||
| key: "config", | ||
| value: "test-value", | ||
| mockFn: func() { | ||
| params := azsecrets.SetSecretParameters{Value: stringPtr("test-value")} | ||
| mockClient.On("SetSecret", mock.Anything, "prefix-dev-app-config", params). | ||
| Return(azsecrets.SetSecretResponse{}, nil) | ||
| }, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "empty stack", | ||
| stack: "", | ||
| component: "app", | ||
| key: "config", | ||
| value: "test", | ||
| mockFn: func() {}, | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "non-string value", | ||
| stack: "dev", | ||
| component: "app", | ||
| key: "config", | ||
| value: 123, | ||
| mockFn: func() {}, | ||
| wantErr: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| mockClient.ExpectedCalls = nil | ||
| mockClient.Calls = nil | ||
| tt.mockFn() | ||
|
|
||
| err := store.Set(tt.stack, tt.component, tt.key, tt.value) | ||
| if tt.wantErr { | ||
| assert.Error(t, err) | ||
| } else { | ||
| assert.NoError(t, err) | ||
| } | ||
| mockClient.AssertExpectations(t) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for Azure-specific errors.
The TestKeyVaultStore_Set should include test cases for Azure-specific error scenarios:
- Rate limiting (429)
- Service unavailable (503)
- Network timeout
osterman
added
minor
This pull request introduces support for Azure Key Vault as a new store type in the project, along with the necessary documentation and tests. The most important changes include updating dependencies, adding the Azure Key Vault store implementation, and updating the documentation to reflect the new store type.
Azure Key Vault Store Implementation:
pkg/store/azure_keyvault_store.go: Added a newKeyVaultStoreimplementation to interact with Azure Key Vault, including methods for setting and getting secrets.pkg/store/azure_keyvault_store_test.go: Added unit tests for theKeyVaultStoreto ensure correct behavior and input validation.pkg/store/registry.go: Updated the store registry to include the newazure-key-vaultstore type and its configuration options.Dependency Updates:
go.mod: Added several Azure SDK dependencies to support the new Azure Key Vault functionality. [1] [2] [3] [4]Documentation Updates:
website/docs/core-concepts/projects/configuration/stores.mdx: Updated the documentation to include details on configuring and using the Azure Key Vault store, including examples and authentication methods. [1] [2]Summary by CodeRabbit
New Features
Dependencies
Documentation
Tests