atmos read/write storage interface #865
Conversation
mcalhoun
changed the base branch from
main
to
chore/refactor-checkatmosconfig
|
💥 This pull request now has conflicts. Could you fix it @mcalhoun? 🙏 |
|
Warning Rate limit exceeded@mcalhoun has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces significant enhancements to the Atmos configuration management system, focusing on store functionality, configuration handling, and dependency management. The changes include adding new store implementations (AWS SSM Parameter Store and In-Memory Store), updating dependency versions, and refactoring various utility functions to support more flexible configuration processing. The modifications aim to improve the system's extensibility and configuration management capabilities. Changes
Suggested Labels
Possibly related PRs
Suggested reviewers
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 (18)
pkg/store/aws_ssm_param_store.go (2)
31-31: Correct the function comment to match 'NewSSMStore'.The comment on line 31 incorrectly refers to 'NewInMemoryStore'. It should be 'NewSSMStore' to reflect the actual function.
Apply this diff to fix the issue:
-// NewInMemoryStore initializes a new MemoryStore. +// NewSSMStore initializes a new SSMStore.
50-50: Pass context from the caller instead of using 'context.TODO()'.Using 'context.TODO()' is a placeholder. It's better to accept a context parameter in your methods to support cancellation and timeouts.
Apply this diff to implement the suggestion:
- func (s *SSMStore) Set(key string, value interface{}) error { + func (s *SSMStore) Set(ctx context.Context, key string, value interface{}) error { ... - func (s *SSMStore) Get(key string) (interface{}, error) { + func (s *SSMStore) Get(ctx context.Context, key string) (interface{}, error) { ... - ctx := context.TODO()Note: This change will require updating any callers of these methods to pass a context.
Also applies to: 75-75
pkg/store/store.go (1)
6-6: Clarify or remove the comment regarding default values.The comment
// Default values if it doesn't exist?seems like a placeholder or reminder. Consider removing it or providing additional context.Apply this diff to fix the issue:
pkg/hooks/hooks.go (2)
12-16: Consider future extensibility for Command type.The Command type is well-positioned for future expansion beyond store operations.
Consider documenting potential future command types in comments to guide future development.
18-28: Enhance documentation for Hook struct fields.While the structure is solid, the field documentation could be more descriptive.
Consider enhancing the comments:
type Hook struct { - Events []string `yaml:"events"` - Command string `yaml:"command"` + // Events defines when this hook should be triggered + Events []string `yaml:"events"` + // Command specifies the type of operation to perform (e.g., "store") + Command string `yaml:"command"` // Dynamic command-specific properties // store command - Name string `yaml:"name,omitempty"` // for store command - Values map[string]interface{} `yaml:"values,omitempty"` // for store command + // Name identifies the store to use for this operation + Name string `yaml:"name,omitempty"` + // Values contains the key-value pairs to be stored + Values map[string]interface{} `yaml:"values,omitempty"` }pkg/store/registry.go (1)
7-36: Consider using constants for store types.The store initialization logic is solid, but string literals for store types could be replaced with constants.
Consider adding constants:
+const ( + StoreTypeSSM = "aws-ssm-parameter-store" + StoreTypeInMemory = "in-memory" +) func NewStoreRegistry(config *StoresConfig) (StoreRegistry, error) { registry := make(StoreRegistry) for key, storeConfig := range *config { switch storeConfig.Type { - case "aws-ssm-parameter-store": + case StoreTypeSSM: var opts SSMStoreOptions if err := parseOptions(storeConfig.Options, &opts); err != nil { return nil, fmt.Errorf("failed to parse SSM store options: %w", err) } store, err := NewSSMStore(opts) if err != nil { return nil, err } registry[key] = store - case "in-memory": + case StoreTypeInMemory:internal/exec/yaml_func_exec.go (1)
Line range hint
1-36: Consider consistent parameter passing.The function still uses cliConfig for logging and shell execution but removes it from getStringAfterTag. This creates an inconsistent pattern.
Consider either:
- Removing cliConfig entirely and using a different logging mechanism
- Maintaining consistency by keeping cliConfig in getStringAfterTag
pkg/store/in_memory_store.go (2)
17-20: Document the options parameter usageThe constructor accepts an
optionsparameter but doesn't use it. If it's for future extensibility or interface consistency, please document this intention.-// NewInMemoryStore initializes a new MemoryStore. +// NewInMemoryStore initializes a new MemoryStore. +// The options parameter is reserved for future extensibility but is currently unused. func NewInMemoryStore(options map[string]interface{}) (Store, error) {
22-28: Consider adding input validationThe Set method could benefit from basic input validation to prevent edge cases:
func (m *InMemoryStore) Set(key string, value interface{}) error { + if key == "" { + return fmt.Errorf("key cannot be empty") + } + if value == nil { + return fmt.Errorf("value cannot be nil") + } m.mu.Lock() defer m.mu.Unlock() m.data[key] = value return nil }internal/exec/yaml_func_store.go (2)
12-12: Add function documentationPlease add documentation describing the function's purpose, parameters, and return value.
+// processTagStore processes store tags in YAML configuration, retrieving values from the specified store. +// Parameters: +// - cliConfig: The CLI configuration containing store definitions +// - input: The raw input string containing the store tag +// - currentStack: The current stack being processed +// Returns: The value retrieved from the store or exits on error func processTagStore(cliConfig schema.CliConfiguration, input string, currentStack string) any {
21-24: Consider more flexible parameter parsingThe current implementation requires exactly 2 space-separated parameters. Consider:
- Supporting quoted values for keys with spaces
- Using a more robust parsing mechanism
- parts := strings.Split(str, " ") - if len(parts) != 2 { + const expectedParts = 2 + parts := strings.Fields(str) + if len(parts) != expectedParts {pkg/store/in_memory_store_test.go (2)
18-64: Consider adding concurrent access testsThe test coverage for Set operations is thorough, but since this is a store that might be accessed concurrently, we should add tests to verify thread safety.
Here's a suggested concurrent test:
func TestMemoryStoreConcurrentAccess(t *testing.T) { store := &InMemoryStore{ data: make(map[string]interface{}), } const goroutines = 10 var wg sync.WaitGroup wg.Add(goroutines) for i := 0; i < goroutines; i++ { go func(id int) { defer wg.Done() key := fmt.Sprintf("key-%d", id) value := fmt.Sprintf("value-%d", id) if err := store.Set(key, value); err != nil { t.Errorf("Set() error = %v", err) } got, err := store.Get(key) if err != nil { t.Errorf("Get() error = %v", err) } if got != value { t.Errorf("Get() = %v, want %v", got, value) } }(i) } wg.Wait() }
66-108: Consider adding type assertion testsThe Get operation tests could be enhanced to verify behavior with different value types and type assertions.
Add these test cases:
tests := []struct { name string key string want interface{} wantError bool }{ + { + name: "integer value", + key: "int-key", + want: 42, + wantError: false, + }, + { + name: "map value", + key: "map-key", + want: map[string]string{"foo": "bar"}, + wantError: false, + },internal/exec/yaml_func_terraform_output.go (2)
Line range hint
34-44: Consider enhancing error message for stack parameterWhen using the current stack as default, the trace message is helpful but we could make it more visible for debugging.
- u.LogTrace(cliConfig, fmt.Sprintf("Atmos YAML function `%s` is called with two parameters 'component' and 'output'. "+ - "Using the current stack '%s' as the 'stack' parameter", input, currentStack)) + u.LogDebug(cliConfig, fmt.Sprintf("Atmos YAML function `%s` is using current stack '%s' as default", + input, currentStack))
Line range hint
52-63: Consider adding cache expiration mechanismThe cache implementation using sync.Map is thread-safe, but consider adding a cache expiration mechanism to prevent stale data.
Consider implementing a cache wrapper with TTL support or using a caching library that provides this functionality out of the box.
pkg/store/aws_ssm_param_store_test.go (2)
36-93: Consider adding more edge cases to the test suite.While the current test cases cover the basic scenarios well, consider adding tests for:
- Empty key validation
- Maximum key length validation
- Special characters in parameter names
- Different parameter types (SecureString, StringList)
95-148: Add test cases for common SSM Get scenarios.Consider adding test cases for:
- Parameter not found (ParameterNotFound error)
- Decryption failure for SecureString parameters
- Invalid parameter version
pkg/config/utils.go (1)
491-502: LGTM! Consider enhancing error logging.The implementation is clean and focused. However, when the store registry creation fails, it might be helpful to log the error before returning it.
func processStoreConfig(cliConfig *schema.CliConfiguration) error { log.Debug("processStoreConfig", "cliConfig.StoresConfig", fmt.Sprintf("%v", cliConfig.StoresConfig)) storeRegistry, err := store.NewStoreRegistry(&cliConfig.StoresConfig) if err != nil { + log.Error("failed to create store registry", "error", err) return err } cliConfig.Stores = storeRegistry return nil }
📜 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 (18)
go.mod(3 hunks)internal/exec/yaml_func_exec.go(1 hunks)internal/exec/yaml_func_store.go(1 hunks)internal/exec/yaml_func_template.go(1 hunks)internal/exec/yaml_func_terraform_output.go(1 hunks)internal/exec/yaml_func_utils.go(2 hunks)pkg/config/config.go(1 hunks)pkg/config/utils.go(2 hunks)pkg/hooks/hooks.go(1 hunks)pkg/schema/schema.go(3 hunks)pkg/store/aws_ssm_param_store.go(1 hunks)pkg/store/aws_ssm_param_store_test.go(1 hunks)pkg/store/config.go(1 hunks)pkg/store/in_memory_store.go(1 hunks)pkg/store/in_memory_store_test.go(1 hunks)pkg/store/registry.go(1 hunks)pkg/store/store.go(1 hunks)pkg/utils/yaml_utils.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/yaml_func_terraform_output.go (1)
Learnt from: aknysh
PR: cloudposse/atmos#863
File: internal/exec/yaml_func_terraform_output.go:34-38
Timestamp: 2024-12-17T07:08:41.288Z
Learning: In the `processTagTerraformOutput` function within `internal/exec/yaml_func_terraform_output.go`, parameters are separated by spaces and do not contain spaces. Therefore, using `strings.Fields()` for parsing is acceptable, and there's no need to handle parameters with spaces.
🔇 Additional comments (17)
pkg/store/config.go (1)
1-14: Looks good!
The configuration structures are well-defined, and the code is clear.
internal/exec/yaml_func_template.go (1)
19-19: Change reflects the updated function signature appropriately.
The update to remove cliConfig from the getStringAfterTag call aligns with the modified function signature.
pkg/hooks/hooks.go (1)
3-10: LGTM! Well-defined hook points for Terraform operations.
The hook types provide clear integration points for store operations before and after critical Terraform operations.
pkg/store/registry.go (1)
5-5: LGTM! Clean registry type definition.
The StoreRegistry type provides a simple but effective way to manage multiple stores.
internal/exec/yaml_func_exec.go (1)
19-19: Verify impact of removing cliConfig parameter.
The removal of cliConfig from getStringAfterTag while retaining it in other function calls seems inconsistent.
Let's check other usages of getStringAfterTag:
pkg/store/in_memory_store.go (2)
8-16: Strong foundation for the store implementation!
The InMemoryStore implementation follows Go best practices with:
- Thread-safe data storage using sync.RWMutex
- Clear interface compliance check
- Good encapsulation of internal state
30-39: Solid Get implementation!
The Get method is well-implemented with:
- Proper read lock usage
- Clear error handling
- Descriptive error messages
internal/exec/yaml_func_utils.go (2)
64-75: Clean switch statement refactoring!
The switch statement improves code readability and maintainability. The default case properly handles unsupported YAML types.
Line range hint 79-89: Clean utility function implementation!
The getStringAfterTag function is well-focused and handles its single responsibility effectively.
pkg/utils/yaml_utils.go (2)
11-17: Well structured constants for YAML functions!
The introduction of these constants aligns perfectly with the store functionality objectives and follows good coding practices by eliminating magic strings.
21-24: Clean transition to using constant references!
The update to use constants in AtmosYamlTags array improves maintainability and reduces the risk of typos.
internal/exec/yaml_func_terraform_output.go (1)
24-24: Verify tag constant usage
The function now uses the constant from config package, which is good. However, let's verify this constant is properly defined.
✅ Verification successful
Tag constant is properly defined and imported
The constant AtmosYamlFuncTerraformOutput is correctly defined in the config package and properly imported in the file. The usage is consistent with the codebase standards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the terraform output tag constant is properly defined
# Test: Search for the constant definition
rg -A 1 "AtmosYamlFuncTerraformOutput.*=.*\"!terraform.output\""
Length of output: 263
Script:
#!/bin/bash
# Check if the constant is imported correctly from config package
rg -A 2 "import.*\".*config\".*" internal/exec/yaml_func_terraform_output.go
Length of output: 76
Script:
#!/bin/bash
# Let's check all imports in the file
rg "^import \(" -A 10 internal/exec/yaml_func_terraform_output.go
Length of output: 239
pkg/store/aws_ssm_param_store_test.go (1)
15-34: LGTM! Well-structured mock implementation.
The mock implementation follows best practices with proper error handling and type assertions.
pkg/config/config.go (1)
249-254: LGTM! Verify store configuration processing.
The integration of store configuration processing is clean and follows existing patterns.
✅ Verification successful
Store configuration processing implementation is properly integrated
The implementation is well-structured and follows a clean design:
- Processes store configuration through a dedicated function in
pkg/config/utils.go - Creates a store registry using the configuration from
StoresConfig - Properly documented in schema with clear purpose for hooks and yaml parsing
- Uses appropriate error handling and logging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify store configuration processing implementation
# Test: Search for store configuration processing implementation
ast-grep --pattern 'func processStoreConfig($config *schema.CliConfiguration) error {
$$$
}'
Length of output: 96
Script:
#!/bin/bash
# Let's try a broader search for processStoreConfig function
rg "func processStoreConfig" -A 5
# Also search for any references to store configuration
rg "processStoreConfig" -A 2
# Let's also check for store-related configuration structures
rg "type.*Store.*Config" -A 3
Length of output: 1891
pkg/schema/schema.go (3)
3-3: Well-structured store integration!
The store-related fields are well-integrated into the configuration structure with clear documentation explaining the runtime-only nature of the Stores field.
Also applies to: 20-21, 34-37
204-204: LGTM! Consistent with existing patterns.
The ComponentHooksSection field follows the established pattern of using AtmosSectionMapType for component sections.
488-488: LGTM! Maintains consistency with base component structure.
The BaseComponentHooks field aligns with the existing pattern of base component configuration fields.
|
💥 This pull request now has conflicts. Could you fix it @mcalhoun? 🙏 |
|
@coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
go.mod (3)
19-19: New logging library.This new library (github.com/charmbracelet/log) can simplify structured logging. Confirm that it is applied consistently across the codebase for a cohesive logging experience.
92-95: Expanded AWS SSO and STS dependencies.Since SSO and STS logic can introduce complexities in cross-account or session-based operations, confirm that the updated versions do not break any current usage patterns.
132-132: Minimal feedback on go-logfmt/logfmt addition.No issues here. If you integrate this formatting approach, consider standardizing logs to maintain consistent style across the project.
pkg/store/aws_ssm_param_store.go (1)
62-69: Parameter writing.Overwriting existing keys simplifies usage. If you anticipate needing a "merge" or "append" approach, consider future extension here.
📜 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 (2)
go.mod(4 hunks)pkg/store/aws_ssm_param_store.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/store/aws_ssm_param_store.go
1-1: : # github.com/cloudposse/atmos/pkg/store [github.com/cloudposse/atmos/pkg/store.test]
pkg/store/aws_ssm_param_store_test.go:53:17: cannot use true (untyped bool constant) as *bool value in struct literal
pkg/store/aws_ssm_param_store_test.go:109:22: cannot use true (untyped bool constant) as *bool value in struct literal
(typecheck)
🔇 Additional comments (20)
go.mod (5)
10-12: Added AWS SDK dependencies look aligned with PR objectives.
These additions for AWS SDK v2 and its SSM module align with the introduction of the new AWS SSM store functionality. Be mindful to keep them up-to-date to take advantage of any relevant bug fixes or enhancements.
80-81: Updated AWS credentials and IMDS dependencies.
These updates appear necessary for supporting the new store functionality. If possible, confirm that references to the credentials constructor or IMDS usage follow the updated methods and type signatures introduced in these versions.
83-85: Keep an eye on internal config changes.
The internal configsources, endpoints, and INI modules are central to AWS configuration resolution. Verify that existing unit tests cover edge cases like missing or malformed config files.
87-89: Internal S3-related modules updated.
While these changes likely come in indirectly, ensure that the presigned URL usage is tested so that your code references remain compatible with the updated library.
251-251: golang.org/x/exp updated.
This experimental package can be a moving target. Validate that your code is not reliant on APIs that may change or break in future releases.
pkg/store/aws_ssm_param_store.go (15)
1-2: New package declaration looks good.
This package is well-named to target AWS SSM Parameter Store. No immediate concerns noted.
🧰 Tools
🪛 golangci-lint (1.62.2)
1-1: : # github.com/cloudposse/atmos/pkg/store [github.com/cloudposse/atmos/pkg/store.test]
pkg/store/aws_ssm_param_store_test.go:53:17: cannot use true (untyped bool constant) as *bool value in struct literal
pkg/store/aws_ssm_param_store_test.go:109:22: cannot use true (untyped bool constant) as *bool value in struct literal
(typecheck)
3-6: Imports appear valid and minimal.
These imports cover the AWS SDK and standard Go libraries for context and formatting. Good job keeping them concise.
7-11: AWS and SSM references.
The chosen modules from the AWS SDK are exactly what you need to support SSM. Great alignment with your PR objectives.
13-16: SSMStore struct.
The struct definition is clear, focusing only on what’s needed for the SSM client. This keeps the store abstraction straightforward.
18-20: Region specified in SSMStoreOptions.
Mapping the region in the struct is good. This fosters explicit user configuration and reduces mistakes from default region assumptions.
22-24: Store interface satisfaction.
Declaring the interface conformance is neat and ensures that the store pattern is standardized throughout the codebase.
25-29: Interface for SSMClient.
By introducing SSMClient as an interface, it’s easier to mock AWS interactions for testing. This design choice is flexible.
31-32: NewSSMStore constructor.
Straightforward name. The function returns the store interface, which is a good factory pattern.
35-39: Robust AWS config loading.
Appropriately handling errors from config loading ensures the application fails fast if AWS credentials or config are missing.
41-45: Enforcement of a non-empty region.
Requiring a region is a good design choice. Minimizes runtime confusion by failing early if misconfigured.
47-50: SSM client creation.
Constructing the SSM client from the validated config is straightforward and robust. Nice approach.
52-60: Set method enforces string value.
The design ensures type consistency in SSM parameters. However, be sure to note in your docs that only string values are supported.
70-72: Error contextualization.
Appending the key name in the error message helps with debugging. Nicely done.
77-88: Get method with decryption.
Allowing secure parameter decryption is essential for handling secrets. Great that you return an error if retrieval fails instead of ignoring it.
90-91: Returning string data.
Returning the parameter value as a string is consistent with the approach used in the Set method.
|
These changes were released in v1.131.0. |
what
Add the concept of atmos stores to the app. This PR adds only the
readside of stores. Thewriteside will be implemented in a separate PR.why
Atmos stores allow values to be written to an external store (i.e., AWS SSM Param Store) after apply-time and read from at plan/before apply time.
Stores are pluggable and configurable and multiple may be enabled at the same time (i.e. for prod/non-prod, east/west, etc).
The initial set of stores we are implementing as part of this PR are:
AWS SSM Parameter Store
Summary by CodeRabbit
New Features
Storeinterface with methods for storing and retrieving data.Bug Fixes
Documentation
Tests