Add support for atmos terraform providers commands
#866
Conversation
📝 WalkthroughWalkthroughThe changes in Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (1)internal/exec/help.go (1)
By merging both component types into the same conditional branch, you reduce duplication of help statements. This is clear and consistent with the rest of the code. 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: 0
🧹 Nitpick comments (1)
internal/exec/utils.go (1)
322-322: Consider providing extra context in returned errors.Returning the error as-is is perfectly fine for now, but adding more context can help with debugging in complex scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/utils.go(2 hunks)
🔇 Additional comments (1)
internal/exec/utils.go (1)
438-441: Great job preventing nil pointer issues here!
Initializing the map when it’s nil effectively prevents potential runtime panics and clarifies the intent of your error handling.
📝 WalkthroughWalkthroughThe pull request addresses a critical issue in the Changes
Assessment against linked issues
Possibly related issues
Suggested labels
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exec/utils.go (1)
322-322: Consider error wrapping for better traceability
Although returning the error here is correct, consider adding more context to make debugging easier. Silently propagating the error can make it tougher to trace where it originated.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/utils.go(2 hunks)
🔇 Additional comments (1)
internal/exec/utils.go (1)
438-441: Great safeguard to avoid nil map panic
Initializing the component section ensures that subsequent operations are safe from nil pointer dereferences. This effectively prevents panics in corner cases.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (16)
pkg/store/aws_ssm_param_store.go (2)
32-50: Recommended check for AWS session reuse.The code properly loads AWS configuration and sets up an SSM client. Consider reusing or caching the session if performance across multiple calls becomes a concern.
53-75: Validate parameter value is not empty.While it is correct to only handle string inputs, a quick check for blank or overly large values may help catch invalid usage.
pkg/store/store.go (1)
9-10: Clear extension point for store creation.StoreFactory provides a flexible mechanism for injecting different store backends. Infuse helpful logging or metrics if needed.
pkg/store/config.go (1)
12-14: Surface mapstructure errors gracefully.It might be helpful to wrap the error with more context about which config field is causing issues.
pkg/hooks/hooks.go (1)
18-28: Consider adding validation for hook eventsThe
Hookstructure looks solid, but consider adding validation to ensure that only valid event types fromHookTypeconstants are used in theEventsslice.type Hook struct { Events []string `yaml:"events"` 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 + + // Consider adding: + // ValidateEvents() error }pkg/store/registry.go (1)
7-36: Consider enhancing store type validation and error handlingThe store registry implementation is solid, but could benefit from a few improvements:
- Consider using constants for store types instead of string literals
- Add validation for required fields in store configurations before processing
- Consider implementing a cleanup method for proper resource management
+// Store types +const ( + StoreTypeSSM = "aws-ssm-parameter-store" + StoreTypeInMemory = "in-memory" +) func NewStoreRegistry(config *StoresConfig) (StoreRegistry, error) { registry := make(StoreRegistry) + if config == nil { + return nil, fmt.Errorf("store config cannot be nil") + } for key, storeConfig := range *config { + if err := validateStoreConfig(storeConfig); err != nil { + return nil, fmt.Errorf("invalid store config for %s: %w", key, err) + } switch storeConfig.Type { - case "aws-ssm-parameter-store": + case StoreTypeSSM: // ... rest of the code - case "in-memory": + case StoreTypeInMemory: // ... rest of the codepkg/store/in_memory_store.go (1)
22-28: Consider adding nil value validation in Set methodThe Set method should validate the input parameters to prevent potential panics. This aligns with the PR's goal of panic prevention.
func (m *InMemoryStore) Set(key string, value interface{}) error { + if key == "" { + return fmt.Errorf("key cannot be empty") + } m.mu.Lock() defer m.mu.Unlock() m.data[key] = value return nil }internal/exec/yaml_func_store.go (1)
12-41: Strengthen error handling and remove unused parameterThe implementation looks solid, but there are a few improvements to consider:
- The
currentStackparameter is unused- Consider adding more context to error messages
-func processTagStore(cliConfig schema.CliConfiguration, input string, currentStack string) any { +func processTagStore(cliConfig schema.CliConfiguration, input string) any { log.Debug("Executing Atmos YAML function store", "input", input) str, err := getStringAfterTag(input, u.AtmosYamlFuncStore) if err != nil { - u.LogErrorAndExit(cliConfig, err) + u.LogErrorAndExit(cliConfig, fmt.Errorf("failed to process store tag: %w", err)) }pkg/utils/yaml_utils.go (1)
11-17: Well structured constants, warrior! Consider adding documentation.The constants are well-organized and improve maintainability. A brief comment for each constant explaining its purpose would make it even better.
Consider adding specific documentation for each constant:
const ( // Atmos YAML functions - AtmosYamlFuncExec = "!exec" - AtmosYamlFuncStore = "!store" - AtmosYamlFuncTemplate = "!template" - AtmosYamlFuncTerraformOutput = "!terraform.output" + // AtmosYamlFuncExec executes shell commands + AtmosYamlFuncExec = "!exec" + // AtmosYamlFuncStore manages store operations + AtmosYamlFuncStore = "!store" + // AtmosYamlFuncTemplate processes template strings + AtmosYamlFuncTemplate = "!template" + // AtmosYamlFuncTerraformOutput retrieves terraform outputs + AtmosYamlFuncTerraformOutput = "!terraform.output" )pkg/store/in_memory_store_test.go (3)
8-16: Strong test for store creation, warrior!The test effectively verifies store initialization. Consider adding a test case for configuration validation.
Consider adding a test case with invalid configuration:
func TestNewMemoryStoreWithInvalidConfig(t *testing.T) { invalidConfig := map[string]interface{}{ "invalid": "config", } store, err := NewInMemoryStore(invalidConfig) if err == nil { t.Error("NewMemoryStore() expected error with invalid config") } }
18-64: Well-structured test cases for Set operations!The table-driven tests cover various data types effectively. Consider adding error cases and concurrent access tests.
Consider adding:
- Test case for nil value
- Concurrent access test to verify thread safety
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) err := store.Set(key, id) if err != nil { t.Errorf("Concurrent Set() error = %v", err) } }(i) } wg.Wait() }
66-108: Solid test coverage for Get operations!The tests handle both existing and non-existing keys well. Consider adding type assertion tests.
Consider adding test cases for type assertions:
{ name: "type assertion", key: "number-key", value: 42, want: 42, wantError: false, },internal/exec/yaml_func_terraform_output.go (1)
Line range hint
52-63: Effective caching mechanism!The cache implementation using sync.Map is thread-safe and prevents redundant terraform output calls.
Consider implementing a cache expiration mechanism for long-running processes to prevent stale data.
pkg/store/aws_ssm_param_store_test.go (2)
36-93: Consider adding more edge cases to strengthen test coverage.The current test cases cover the basic scenarios well. Consider adding tests for:
- Empty key/value
- Very long parameter values (AWS SSM has size limits)
- Special characters in parameter names
95-148: Add test cases for common SSM parameter retrieval scenarios.Consider adding test cases for:
- Parameter not found
- Parameter with different types (String, StringList, SecureString)
- Decryption failures for SecureString parameters
pkg/config/utils.go (1)
491-502: Clean implementation of store configuration processing!The function is well-structured and handles errors appropriately. Consider enhancing the error context for better debugging.
Consider wrapping the error with additional context:
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 { - return err + return fmt.Errorf("failed to create store registry: %w", 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(4 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.
🪛 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 (16)
pkg/store/store.go (1)
3-8: Interface design looks solid.
The Store interface is succinct, matching general usage patterns. Encourage thorough validation or type checks within implementations.
pkg/store/config.go (1)
5-8: All good here.
The StoreConfig structure is straightforward and can handle typical store config.
internal/exec/yaml_func_template.go (1)
19-19: Validate input content.
Ensure the string retrieved by getStringAfterTag is not empty or malformed before unmarshalling. It prevents potential decoding of incomplete JSON.
internal/exec/yaml_func_exec.go (1)
19-19: Verify the impact of removing cliConfig parameter
The removal of cliConfig from getStringAfterTag call appears to be part of a broader refactoring. While this change simplifies the API, we should ensure it doesn't affect error handling or logging capabilities.
pkg/hooks/hooks.go (1)
30-32: LGTM! Clean and straightforward hooks registry
The Hooks structure provides a clean way to manage multiple hooks. The map implementation allows for efficient lookup by name.
pkg/store/in_memory_store.go (1)
8-12: LGTM! Thread-safe implementation with RWMutex
The InMemoryStore implementation properly uses sync.RWMutex for concurrent access protection. This is a solid foundation for preventing race conditions.
internal/exec/yaml_func_store.go (1)
29-38: Consider adding store type validation
The store retrieval and key lookup are properly implemented, but we might want to add type validation for the returned value to prevent potential panics in the calling code.
internal/exec/yaml_func_utils.go (1)
63-75: LGTM! Clean switch statement implementation
The switch statement improves readability and maintainability. The default case properly handles unsupported YAML types, preventing potential panics.
pkg/utils/yaml_utils.go (1)
21-24: Strong move using constants in AtmosYamlTags!
The update to use constants instead of string literals enhances maintainability and reduces potential typos.
internal/exec/yaml_func_terraform_output.go (2)
24-27: Robust error handling for tag processing!
The error handling ensures proper validation of the tag format, preventing panics.
Line range hint 35-44: Strong parameter validation and stack handling!
The code properly handles both 2 and 3 parameter cases, with clear logging when using the current stack.
Let's verify the parameter handling:
✅ Verification successful
Let me verify the parameter handling further by checking how the code processes these parameters.
Parameter validation is robust and well-implemented!
The code demonstrates strong parameter validation with proper error handling:
- Validates both 2 and 3 parameter cases correctly
- Provides clear error messages for invalid parameter counts
- Implements thread-safe caching using sync.Map
- Handles both static remote state and terraform output scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential parameter parsing edge cases
rg -l "!terraform\.output\s+\S+\s+\S+\s+\S+" | while read -r file; do
echo "Checking $file for terraform.output usage:"
rg "!terraform\.output\s+\S+\s+\S+\s+\S+" "$file"
done
Length of output: 3675
Script:
#!/bin/bash
# Check the implementation of terraform.output function
ast-grep --pattern 'func terraformOutput($$$) $$$'
# Check for error handling and validation
rg "terraformOutput" -A 10 -B 10
Length of output: 9314
pkg/store/aws_ssm_param_store_test.go (1)
15-34: Well-structured mock implementation!
The mock implementation follows best practices:
- Proper error handling for nil returns
- Consistent method signatures matching the AWS SDK
- Clean use of testify/mock for assertions
pkg/config/config.go (1)
249-253: LGTM! Verify store configuration error handling.
The addition of store configuration processing is clean and follows existing patterns. Let's verify the error handling in processStoreConfig.
✅ Verification successful
Error handling in processStoreConfig looks solid!
The function properly handles errors from store.NewStoreRegistry and returns them to the caller. The implementation follows a clean pattern:
- Initializes store registry with config
- Returns any initialization errors
- Sets the registry in config when successful
- Returns nil on success
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in processStoreConfig
ast-grep --pattern 'func processStoreConfig($config) $_ {
$$$
if err != nil {
$$$
}
$$$
}'
Length of output: 104
Script:
#!/bin/bash
# Let's try a different approach to find processStoreConfig implementation
rg -U "func processStoreConfig" -A 10
Length of output: 583
pkg/schema/schema.go (3)
3-3: Well-structured store configuration implementation!
The separation between StoresConfig for configuration and Stores for the registry instance is clean and well-documented. The YAML/JSON tags are properly configured, and the documentation clearly explains the purpose of the Stores field.
Also applies to: 20-20, 34-37
204-204: LGTM: ComponentHooksSection follows established patterns
The addition of ComponentHooksSection maintains consistency with other component sections in the struct.
488-488: LGTM: BaseComponentHooks maintains structural consistency
The addition of BaseComponentHooks aligns with the existing base component configuration pattern.
|
@Listener430 looks like a bad merge. Can no longer see you contributions to this PR. |
|
💥 This pull request now has conflicts. Could you fix it @Listener430? 🙏 |
Listener430
force-pushed
the
DEV-2767
branch
from
b75be94 to
c78ab33
Compare
@osterman thanks, my bad, fixed the bad mergre is that ok now? |
aknysh
added
minor
aknysh
changed the title
atmos terraform providers commands
aknysh
changed the title
atmos terraform providers commandsatmos terraform providers commands
aknysh
changed the title
atmos terraform providers commandsatmos terraform providers commands
|
These changes were released in v1.132.0. |
what
atmos terraform providerscommandsatmos terraform providers lock <component> --stack <stack>command when a non existing component or stack were specifiedatmos validate component <component> --stack <stack>commandwhy
atmos terraform providers lock <component> --stack<stack>atmos terraform providers schema <component> --stack<stack> -- -jsonatmos terraform providers mirror <component> --stack<stack> -- <directory>atmos validate component <component> --stack <stack>commandreferences
Summary by CodeRabbit
New Features
Bug Fixes
Chores