add hooks and store write functionality #874
Conversation
mcalhoun
added
the
no-release
mcalhoun
force-pushed
the
feature/add-store-writes
branch
from
b478a34 to
50afe8e
Compare
📝 WalkthroughWalkthroughThis pull request introduces comprehensive enhancements to Atmos' command execution, configuration management, and store functionality. The changes focus on improving how Terraform commands are processed, introducing hooks support, refactoring store implementations, and adding more robust context management across various components. The modifications span multiple files and packages, introducing new methods for handling configuration, outputs, and component interactions. Changes
Sequence DiagramsequenceDiagram
participant CLI as Atmos CLI
participant PreRun as PreRun Function
participant TerraformCmd as Terraform Command
participant PostRun as PostRunE Function
CLI->>PreRun: Process Command Line Args
PreRun-->>CLI: Set Context
CLI->>TerraformCmd: Execute Command
TerraformCmd->>PostRun: Trigger Post-Execution
PostRun-->>CLI: Handle Hooks and Outputs
Possibly related PRs
Suggested Labels
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: 1
🧹 Nitpick comments (14)
pkg/store/registry.go (1)
24-24: Validate removal of options.Removing the
optionsparameter changes the behavior of the in-memory store. Confirm that no dynamic configurations were intended for the in-memory store.pkg/store/in_memory_store.go (2)
7-7: Consider concurrency if needed.An in-memory map without synchronization may cause data races in concurrent contexts. Evaluate if concurrency is expected.
24-37: Avoid repetitive validation.All the checks for empty strings might be centralized in a helper to reduce code duplication.
func (s *InMemoryStore) Set(stack, component, key string, value interface{}) error { - if stack == "" { ... } - if component == "" { ... } - if key == "" { ... } + if err := s.validateParams(stack, component, key); err != nil { + return err + } // ... }pkg/store/aws_ssm_param_store.go (3)
22-24: Region enforcement is good, but consider default or fallback.Currently, it errors out if region is missing. Evaluate if an environment-based fallback or a default region is desired.
54-69: Check performance overhead.Loading AWS config on each store initialization can be expensive if done repeatedly. Evaluate if a shared config approach is more efficient.
Line range hint
82-113: Type restriction on value.Limiting
valuetostringis fine but might reduce flexibility. Consider supporting other data types if needed.pkg/store/aws_ssm_param_store_test.go (3)
38-39: Clarify testing of delimiter usage
Currently,testDelimiteris set to"-". The code suggests that it splits the stack (e.g.,"dev-usw2-prod") on the hyphen and reconstructs it in the parameter name. If the implementation merges all segments with"/", it’s helpful to explicitly document this behavior or ensure the delimiter is used as intended so that future maintainers understand how the multiple hyphens are handled.
131-144: Test complex stack naming
The “complex stack name with multiple delimiters” scenario is well done. It ensures that splitting and reconstructing the stack name with multi-part tokens is tested properly. Consider also verifying an edge case with trailing delimiters or empty tokens in the stack name to ensure robust handling.
214-252: Refine error cases
The tests for empty stack/component/key and not found parameters cover various failure modes thoroughly. As a future enhancement, you might want to distinguish between different AWS error types or add specialized assertions for the “parameter not found” path to confirm that the method returns an error type or message consistent with user expectations.internal/exec/yaml_func_store.go (3)
19-42: Validate multiple input formats more thoroughly.Consider stricter checks to confirm each parameter (stack, component, key) is non-empty before returning. This would prevent accidental references to empty values.
52-54: Ensure consistent error handling.The error message is helpful. Double-check that displays or logs won't leak sensitive information.
57-66: Enhance store retrieval logic.The retrieval logic is straightforward. Investigate if parallel lookups or caching could enhance performance, depending on usage frequency.
internal/exec/terraform_generate_backends.go (1)
151-153: Initialize hooks properly.Creating a fallback map ensures hooks do not cause nil-pointer checks. Confirm dynamic merges for user-defined hooks remain correct.
internal/exec/terraform.go (1)
37-53: Consider returning a struct or single value instead of multiple booleans.
Returning two separate booleans might reduce clarity. Condensing them into a single struct or well-named enum would enhance readability and maintainability.
📜 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 (21)
cmd/terraform.go(3 hunks)go.mod(4 hunks)internal/exec/describe_stacks.go(7 hunks)internal/exec/stack_processor_utils.go(12 hunks)internal/exec/template_funcs_component.go(1 hunks)internal/exec/terraform.go(5 hunks)internal/exec/terraform_generate_backends.go(4 hunks)internal/exec/terraform_outputs.go(1 hunks)internal/exec/terraform_utils.go(0 hunks)internal/exec/utils.go(3 hunks)internal/exec/yaml_func_store.go(1 hunks)internal/exec/yaml_func_terraform_output.go(1 hunks)pkg/config/const.go(1 hunks)pkg/hooks/hooks.go(2 hunks)pkg/hooks/store.go(1 hunks)pkg/store/aws_ssm_param_store.go(4 hunks)pkg/store/aws_ssm_param_store_test.go(5 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)
💤 Files with no reviewable changes (1)
- internal/exec/terraform_utils.go
✅ Files skipped from review due to trivial changes (1)
- pkg/hooks/store.go
🧰 Additional context used
📓 Learnings (1)
internal/exec/terraform_outputs.go (1)
Learnt from: aknysh
PR: cloudposse/atmos#810
File: internal/exec/terraform_utils.go:40-213
Timestamp: 2024-12-03T05:18:49.169Z
Learning: In the context of the Atmos project, it's acceptable for functions like `execTerraformOutput` to remain as single functions if they perform a single purpose, such as retrieving Terraform outputs for a component in a stack, even if the function is lengthy.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (77)
pkg/store/store.go (1)
5-6: Ensure backward compatibility or versioning.The addition of
stackandcomponentto the interface might break existing callers. Ensure downstream consumers are updated or versioned properly.Would you like a script to search for references to the old interface usage across the codebase?
pkg/store/in_memory_store.go (2)
12-17: Constructor is simplified.Eliminating extra parameters improves usage. Nice approach for a straightforward in-memory store!
19-21: getKey approach looks good.This method clarifies how keys are computed in a consistent format.
pkg/store/in_memory_store_test.go (2)
9-74: TestInMemoryStore_Set is solid and comprehensive.Covers normal and edge cases thoroughly—good job ensuring negative tests for empty fields.
80-168: TestInMemoryStore_Get covers a wide range of scenarios.Thorough check of existence, empties, and mismatched keys/stack/components. The test suite is well-scoped.
pkg/store/aws_ssm_param_store.go (3)
16-18: Field expansions look reasonable.Storing
prefixandstackDelimiteroffers flexibility for the SSM structure. Keep them well documented to guide future maintainers.
71-78: getKey logic is consistent.Combining prefix, stack, component, and key is well-strategized for clarity and uniqueness.
120-142: Consistent validations.Similar to
Set, empty parameter checks ensure clarity in error messages. This uniform approach avoids silent failures.pkg/store/aws_ssm_param_store_test.go (2)
42-48: Good coverage with structured test cases
Definingname,stack,component,key,value,mockFn, andwantErrin a single struct is an excellent way to keep these test scenarios concise, consistent, and easily extendable in the future.
115-129: Confirmnil stack delimitertest scenario
Even thoughnameindicates a “nil stack delimiter,” the test still usestestDelimiter = "-". Verify that the actual SSMStore code handles a truly nil delimiter or falls back to a default. Consider adding another test scenario to explicitly setstore.stackDelimiter = nilto confirm correct behavior.internal/exec/yaml_func_terraform_output.go (1)
53-54: Simplified retrieval approach
This direct retrieval viaGetTerraformOutputremoves interlaced logic and is easier to maintain.cmd/terraform.go (6)
4-7: Useful imports
These imports, especiallycontextandstrings, are necessary for the newly introduced logic.
12-13: Hooks package import
Bringing the hooks package into this file cleanly supports the hooking functionality below.
18-19: Context key definition
Defining a dedicated type for the key helps distinguish context values and avoid collisions.
Line range hint
27-47: PreRun context initialization
Storinginfoin context here makes the data easily accessible in subsequent lifecycle steps.
Line range hint
48-66: Run function
The main logic is concise. It delegates execution toExecuteTerraformand exits cleanly on error.
66-109: PostRun hook
Processing hooks after Terraform work finishes is pragmatic. Storing outputs is well-handled with additional logging.internal/exec/terraform_outputs.go (4)
22-196: execTerraformOutput function
It carefully handles backend files, providers, workspace creation, and JSON output conversion. This comprehensive approach is appropriate for managing Terraform outputs in a single function.
198-244: GetTerraformOutput function
Centralizing the caching logic here is a clean design choice. It ensures consistent retrieval and uses static remote state when available.
246-264: getTerraformOutput helper
Extracting this detail helps isolate map lookups and validations, promoting clarity.
266-285: getStaticRemoteStateOutput helper
Delegation to separate handling for static remote state is well-structured. Errors are properly flagged.internal/exec/stack_processor_utils.go (10)
559-559: terraformHooks map
Including a dedicated hooks map clarifies the approach for Terraform-specific hooks.
664-670: Global Terraform hooks section
Fetching hooks from the global Terraform section ensures they can be merged into each component.
807-814: Per-component hooks
IntroducingcomponentHooksallows each component to define its own hook logic, which is then merged.
875-875: componentOverridesHooks
This placeholder sets the stage for overriding hooks at the component level.
912-917: componentOverridesHooks validation
A brief validation and a map assignment is consistent with other overrides logic.
1092-1103: finalComponentHooks merge
Merging global, base, current, and override hooks fosters a unified final hook set.
1292-1292: Assigning final hooks
AttachingfinalComponentHooksto the component's configuration is straightforward.
1998-1998: baseComponentHooks definition
Defining hooks in the base component structure ensures inheritance consistency.
2122-2128: baseComponentHooks extraction
Fetching hooks from the base component and validating them extends the existing pattern for inheritable settings.
2199-2205: Hooks merge in base components
Allowing hooks to merge throughout the inheritance chain ensures components properly accumulate hook settings.pkg/hooks/hooks.go (4)
3-7: New imports
Usingfmtandyaml.v2is appropriate for input marshalling and error formatting.
32-33: Rename and restructure
RenamingValuestoOutputsclarifies the intention for storing command output.
36-37: Hooks as a map
DefiningHooksas a map of hook names toHooksimplifies usage.
38-50: ConvertToHooks function
Marshalling to YAML and back is an efficient approach for flexible type conversions.internal/exec/yaml_func_store.go (1)
12-17: Solid approach with a dedicated struct.Defining a
paramsstruct provides clarity and maintainability. This helps group store-related parameters coherently.internal/exec/template_funcs_component.go (1)
60-60: Passing config by pointer is convenient.Switching to
&atmosConfigcould enable more straightforward reuse of the same config object, but confirm no concurrency side effects occur if multiple goroutines share the config.pkg/config/const.go (1)
47-47: Hooks section constant added.Defining
HooksSectionNamemakes referencing hooks more consistent. This is a clean extension.internal/exec/terraform_generate_backends.go (3)
90-90: New variable for the hooks section.Explicitly naming
hooksSectionclarifies reading. No concurrency concerns are apparent here.
181-181: Hooks added to config struct.Adding
ComponentHooksSectionpaves the way for flexible automation. Consider cross-component references if needed.
191-191: Include hooks in component map.Placing
hooksSectionwithinComponentSectionaligns with other sections. Double-check for consistent usage elsewhere.internal/exec/terraform.go (7)
55-77: Implementation is solid.
The function logically checks for an auto-generate configuration and writes it as JSON if necessary. No concerns.
79-94: No issues with provider override generation.
This approach is clear and consistent with the rest of the code.
134-136: Clever logic to skip stack processing.
This conditional check is straightforward and reduces complexity for clean operations.
143-143: Appropriate error handling for missing stack.
This prevents confusion when users forget to specify a stack under certain flags.
158-158: No issues noted.
The logic around component path validation looks correct.
242-242: Validation error handling is correct.
Returning an error early keeps the flow clear.
250-257: Modular approach to backend and provider overrides is neat.
Separating out these generation calls makes maintenance simpler. Good job.internal/exec/describe_stacks.go (5)
143-143: Initialization of hooksSection adds needed flexibility.
This ensures hooks do not panic when uninitialized.
230-233: Graceful fallback for missing hooks.
This guard block ensures hooksSection won't break when missing from config.
Line range hint
254-264: Hook storage in ConfigAndStacksInfo is consistent.
The hooks are now part of the shared context, which aids in uniform processing across the system.
429-432: Identical treatment of hooks in Helmfile section.
Maintains consistency between Terraform and Helmfile components.
Line range hint
453-463: Adding hooks to ConfigAndStacksInfo for Helmfile.
Keeps the logic unified and straightforward.internal/exec/utils.go (20)
72-72: Good handling of hooks block.
Declaring the variable early helps keep the code well-organized.
84-84: Helpful early exit for missing stack.
Prevents deeper runtime issues.
88-88: Component name check is fine.
Ensures we do not operate on an empty component.
92-92: Safeguard for component type.
Clear error message benefits users.
96-96: Early detection for unrecognized stack.
Consistent with prior checks.
100-100: Crisp error messaging for missing 'components' section.
This is user-friendly.
104-104: Failure path if componentTypeSection is missing.
Logical approach that clarifies usage.
108-108: No config for specified component.
This keeps stacks from referencing non-existent components.
112-112:varssection baseline requirement.
The user is alerted appropriately if it’s empty.
116-116: Providers fallback to empty map.
This ensures the rest of the code can count on a valid map.
117-119: Protecting hooks from nil references.
Prevents unexpected panics for uninitialized hooks.
120-120: No issues here.
The logic remains aligned with prior sections.
124-124: No concerns.
Backend references are managed well.
128-128: Defaulting backend type to empty string is appropriate.
Ensures the rest of the flow remains stable.
132-132: Import references fallback is safe.
Avoids nil checks later on.
136-136: Command fallback to empty string is fine.
Clean separation of logic.
140-140: env fallback approach is consistent.
Retains a valid map for environment variables.
144-144: Similar fallback strategy in settings.
No concerns.
148-148: Likewise for overrides.
Maintains uniform fallback logic.
173-173: Hook assignment to configAndStacksInfo.
Centralizes the data for further processing.go.mod (4)
54-54: YAML library addition is good.
Use caution with version-specific quirks.
183-184: New indirect dependencies for 'kr/pretty' & 'kr/text'.
They offer improvements for text and data structure handling.
272-272:gopkg.in/check.v1is a classic testing library.
No major issues identified.
215-215:go-internalv1.13.1 introduced.
No immediate concerns. Additional verification for licensing is recommended.✅ Verification successful
License verification completed successfully
Thego-internalpackage uses the BSD-3-Clause license, which is permissive and widely accepted in the Go ecosystem.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Checking for the go-internal licensing within the vendor directory fd -I go-internal vendor | xargs cat | grep -i licenseLength of output: 150
Script:
#!/bin/bash # First, try to find the module in GOPATH cache go env GOPATH 2>/dev/null && find "$(go env GOPATH)/pkg/mod/github.com/rogpeppe" -type f -iname "license*" 2>/dev/null # Then, use GitHub API to check the repository gh api repos/rogpeppe/go-internal/license --jq '.license.spdx_id' 2>/dev/null || echo "Failed to fetch license info" # As a fallback, check for license files in the repository gh api repos/rogpeppe/go-internal/contents --jq '.[].name' 2>/dev/null | grep -i licenseLength of output: 181
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
pkg/store/in_memory_store.go (1)
Line range hint
41-58: ENHANCE YOUR ERROR MESSAGES WITH FULL BATTLE CONTEXT!The error message on key not found only includes the key portion. Include the full context:
-return nil, fmt.Errorf("key '%s' not found", key) +return nil, fmt.Errorf("key '%s' not found in stack '%s', component '%s'", fullKey, stack, component)
🧹 Nitpick comments (2)
pkg/store/in_memory_store.go (1)
19-22: SHIELD YOUR PATHS, BRAVE ONE!The
getKeymethod should sanitize inputs to prevent path traversal attacks. Consider escaping or validating special characters in stack, component, and key parameters.pkg/merge/merge_test.go (1)
97-100: LGTM! Consider enhancing test readability.The pointer type change is solid. Since this test handles complex map merging, consider adding a table-driven test case to make different merge scenarios more maintainable.
Example enhancement:
func TestMergeListMerge(t *testing.T) { tests := []struct { name string map1 map[string]any map2 map[string]any expected map[string]any }{ { name: "complex map merge", map1: map[string]any{ "list": []map[string]string{{ "1": "1", "2": "2", "3": "3", "4": "4", }}, }, map2: map[string]any{ "list": []map[string]string{{ "1": "1b", "2": "2", "3": "3b", "5": "5", }}, }, expected: map[string]any{ "list": []map[string]any{{ "1": "1b", "2": "2", "3": "3b", "4": "4", "5": "5", }}, }, }, // Add more test cases here } // ... test execution }
📜 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)
pkg/merge/merge_test.go(3 hunks)pkg/store/in_memory_store.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
pkg/store/in_memory_store.go (1)
24-39: STRENGTHEN YOUR ERROR DEFENSES!Previous review wisdom about typed errors remains valid. Consider:
- Define custom error types for better error handling downstream
- Include the provided value in error messages for better context
Example implementation:
type EmptyFieldError struct { Field string } func (e *EmptyFieldError) Error() string { return fmt.Sprintf("%s cannot be empty", e.Field) }pkg/merge/merge_test.go (3)
43-46: LGTM! Clean pointer type conversion.The change to pointer type for AtmosSettings follows idiomatic Go patterns. The test logic remains solid, warrior! 💪
70-73: LGTM! Consistent pointer type usage.The pointer type change maintains consistency with the other test functions. The test's purpose of verifying list append behavior remains clear and focused.
Line range hint
43-100: Verify consistent pointer usage across the codebase.The pointer type changes look great! Let's verify this pattern is applied consistently.
|
💥 This pull request now has conflicts. Could you fix it @mcalhoun? 🙏 |
|
@mcalhoun please resolve the conflicts |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/exec/utils.go (1)
118-120: Consider adding structure validation for hooks.While the hooks section is properly initialized, adding validation for the expected structure of hook configurations would prevent invalid configurations from propagating through the system.
Example validation approach:
if componentHooksSection, ok = componentSection[cfg.HooksSectionName].(map[string]any); !ok { componentHooksSection = map[string]any{} +} else { + // Validate hook structure + for hookName, hookConfig := range componentHooksSection { + if hookMap, ok := hookConfig.(map[string]any); ok { + if _, hasCommand := hookMap["command"]; !hasCommand { + return fmt.Errorf("hook '%s' is missing required 'command' field", hookName) + } + } else { + return fmt.Errorf("invalid configuration for hook '%s': expected map[string]any", hookName) + } + } }cmd/terraform.go (2)
39-59: STRENGTHEN YOUR DEFENSES, WARRIOR! 🛡️The PreRun implementation handles argument processing well, but let's fortify it further:
- Consider validating
finalArgsis not empty before processing- The context key could be made into a constant to prevent typos
+ const ( + atmosInfoKey contextKey = "atmos_info" + ) PreRun: func(cmd *cobra.Command, args []string) { + if len(args) == 0 { + u.LogErrorAndExit(schema.AtmosConfiguration{}, fmt.Errorf("no arguments provided")) + } // ... rest of the code ... - ctx := context.WithValue(context.Background(), contextKey("atmos_info"), info) + ctx := context.WithValue(context.Background(), atmosInfoKey, info)
Line range hint
103-190: ENHANCE YOUR ERROR HANDLING STRATEGY, WARRIOR! 🎯The
Containshelper function and error handling look solid, but here are some tactical improvements:
- Consider using
lo.Containsfrom the already importedlopackage instead of the customContainsfunction- Add context to error messages for better debugging
- // Contains checks if a slice of strings contains an exact match for the target string. - func Contains(slice []string, target string) bool { - for _, item := range slice { - if item == target { - return true - } - } - return false - } // In the error handling code: - fmt.Printf("Unknown command: '%s'\n\nDid you mean this?\n", args[0]) + fmt.Printf("Unknown terraform command: '%s'\n\nDid you mean one of these?\n", args[0])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/terraform.go(2 hunks)go.mod(1 hunks)internal/exec/utils.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/terraform.go
4-4: missing import path
(typecheck)
8-8: missing import path
(typecheck)
10-10: missing import path
(typecheck)
🪛 GitHub Check: Build (macos-latest, macos)
cmd/terraform.go
[failure] 4-4:
missing import path
🪛 GitHub Check: Build (ubuntu-latest, linux)
cmd/terraform.go
[failure] 4-4:
missing import path
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
internal/exec/utils.go (3)
73-73: LGTM! Strong error handling.The new variable declaration and error checks are well-structured. The early validation pattern helps prevent downstream issues.
Also applies to: 85-93
118-120: LGTM! Consistent section handling.The hooks section processing follows the established pattern and safely initializes an empty map to prevent nil pointer dereferences.
174-174: LGTM! Clean integration.The hooks section is properly integrated into the config info struct, maintaining consistency with other section assignments.
cmd/terraform.go (1)
Line range hint
27-38: SOLID COMMAND STRUCTURE, WARRIOR! 💪The addition of
contextKeytype and the restructuring of the terraform command with PreRun and PostRunE hooks creates a robust battle formation. This allows for proper setup before execution and cleanup after the main operation.🧰 Tools
🪛 golangci-lint (1.62.2)
4-4: missing import path
(typecheck)
8-8: missing import path
(typecheck)
10-10: missing import path
(typecheck)
🪛 GitHub Check: Build (macos-latest, macos)
[failure] 4-4:
missing import path🪛 GitHub Check: Build (ubuntu-latest, linux)
[failure] 4-4:
missing import path
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmd/terraform.go (3)
20-20: WARRIOR! YOUR TYPE DECLARATION NEEDS MORE STRENGTH! 💪While the
contextKeytype provides type safety, it should be unexported to prevent external usage. Also, consider adding constants for the key values to prevent typos.-type contextKey string +type atmosContextKey string + +const ( + atmosInfoKey atmosContextKey = "atmos_info" +)
32-52: SOLID BATTLE STRATEGY FOR ARGUMENT PROCESSING, WARRIOR! 🛡️The argument processing logic is robust and well-structured. However, let's fortify the context handling:
- ctx := context.WithValue(context.Background(), contextKey("atmos_info"), info) + ctx := context.WithValue(context.Background(), atmosInfoKey, info)Also, consider extracting the double-dash handling logic into a separate function since it's duplicated in
terraformRun.
85-91: STRATEGIC ADVICE FOR RESILIENT OPERATIONS, WARRIOR! 🏰Consider adding retry logic for store operations to handle transient failures when writing to external systems like AWS SSM or Artifactory.
Example implementation:
func withRetry(operation func() error) error { maxRetries := 3 backoff := time.Second var err error for i := 0; i < maxRetries; i++ { if err = operation(); err == nil { return nil } time.Sleep(backoff * time.Duration(i+1)) } return fmt.Errorf("operation failed after %d retries: %w", maxRetries, err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/terraform.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@aknysh this should be good now |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exec/terraform.go (2)
55-77: Strong backend configuration generation, warrior!Consider adding file existence check before writing to prevent accidental overwrites.
func generateBackendConfig(atmosConfig *schema.AtmosConfiguration, info *schema.ConfigAndStacksInfo, workingDir string) error { if atmosConfig.Components.Terraform.AutoGenerateBackendFile { backendFileName := filepath.Join(workingDir, "backend.tf.json") + if !info.DryRun { + if _, err := os.Stat(backendFileName); err == nil { + u.LogDebug(*atmosConfig, fmt.Sprintf("Backend config file already exists: %s", backendFileName)) + } + }
79-94: Solid provider overrides implementation, warrior!Consider adding the same file existence check as suggested for generateBackendConfig.
func generateProviderOverrides(atmosConfig *schema.AtmosConfiguration, info *schema.ConfigAndStacksInfo, workingDir string) error { if len(info.ComponentProvidersSection) > 0 { providerOverrideFileName := filepath.Join(workingDir, "providers_override.tf.json") + if !info.DryRun { + if _, err := os.Stat(providerOverrideFileName); err == nil { + u.LogDebug(*atmosConfig, fmt.Sprintf("Provider overrides file already exists: %s", providerOverrideFileName)) + } + }
📜 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)
cmd/terraform.go(2 hunks)internal/exec/terraform.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (6)
cmd/terraform.go (4)
4-18: LGTM! The imports are well-organized, warrior!The new imports for context, config, and hooks support the added functionality.
20-22: Well done, warrior! The context key implementation follows best practices!Using a custom type for context keys prevents key collisions in the context.Value space.
34-54: Strong implementation of the PreRun function, warrior!The function effectively:
- Processes command line arguments with proper handling of the double dash separator
- Sets up the context with command information
- Includes appropriate error handling
55-109: Excellent implementation of the PostRunE function, warrior!The function demonstrates:
- Safe context value retrieval with proper type checking
- Thorough hook processing with appropriate error handling
- Proper validation of store configurations and outputs
internal/exec/terraform.go (2)
37-53: Well-crafted function, warrior!The shouldProcessStacks function cleanly encapsulates the stack processing decision logic.
250-257: Clean integration of the new generation functions, warrior!The error handling is thorough and the function calls are well-placed in the execution flow.
|
@aknysh I am going to merge this now, but please still review and let me know if there's anything I need to address. |
|
These changes were released in v1.140.0. |
what
why
In order to share data between components and to decouple the Terraform permissions context from the shared values permissions context.
Summary by CodeRabbit
Based on the comprehensive changes across multiple files, here are the release notes:
Release Notes
New Features
Improvements
Bug Fixes
Changes
These changes introduce more robust configuration management and enhanced flexibility in component and stack interactions.