Update Atmos YAML functions #1038
Conversation
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
📝 WalkthroughWalkthroughThis change updates version numbers in the Dockerfile and go.mod, introduces new spinner management functions, and refactors spinner usage in the validate stacks command. It also improves environment variable handling and logging in Terraform outputs and workspace cleanup, and adds a new environment variable section in a YAML configuration file. Changes
Sequence Diagram(s)sequenceDiagram
participant Cmd as ExecuteValidateStacksCmd
participant SpinnerUtil as SpinnerUtils
participant TeaProg as tea.Program
Cmd->>SpinnerUtil: NewSpinner(message)
SpinnerUtil-->>Cmd: spinner instance
Cmd->>SpinnerUtil: RunSpinner(spinner, spinnerChan, message)
par Spinner Running...
TeaProg-->>SpinnerUtil: Spinner active
end
Cmd->>SpinnerUtil: StopSpinner(spinner, spinnerChan)
SpinnerUtil-->>Cmd: Spinner stopped
Possibly related PRs
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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exec/spinner_utils.go (1)
65-75: Consider adding context for cancellation.While the current implementation is solid, consider adding context support for better control over spinner lifecycle.
-func RunSpinner(p *tea.Program, spinnerChan chan struct{}, message string) { +func RunSpinner(ctx context.Context, p *tea.Program, spinnerChan chan struct{}, message string) { go func() { defer close(spinnerChan) + done := make(chan error, 1) + go func() { + _, err := p.Run() + done <- err + }() + + select { + case <-ctx.Done(): + p.Quit() + <-done + case err := <-done: + if err != nil { + fmt.Println(message) + l.Error("Failed to run spinner:", "error", err) + } + } - if _, err := p.Run(); err != nil { - fmt.Println(message) - l.Error("Failed to run spinner:", "error", err) - } }() }internal/exec/terraform_outputs.go (1)
234-250: Consider using defer for environment cleanup.While the current implementation works, using defer would ensure cleanup even in case of early returns.
func execTerraformOutput(...) (map[string]any, error) { outputProcessed := map[string]any{} envVarsSaved := make(map[string]any) + defer func() { + for k, v := range envVarsSaved { + if v != nil { + vStr := fmt.Sprint(v) + l.Debug(fmt.Sprintf("Restoring environment variable %s=%s", k, vStr)) + _ = os.Setenv(k, fmt.Sprintf("%v", vStr)) + } else { + l.Debug(fmt.Sprintf("Unsetting environment variable %s", k)) + _ = os.Unsetenv(k) + } + } + }()
📜 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 (7)
examples/quick-start-advanced/Dockerfile(1 hunks)go.mod(1 hunks)internal/exec/spinner_utils.go(2 hunks)internal/exec/terraform_outputs.go(12 hunks)internal/exec/terraform_utils.go(4 hunks)internal/exec/validate_stacks.go(1 hunks)tests/fixtures/scenarios/complete/stacks/catalog/terraform/template-functions-test/defaults.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- go.mod
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (10)
internal/exec/spinner_utils.go (2)
44-63: LGTM! Well-structured spinner initialization.The function handles both TTY and non-TTY environments gracefully, with proper error handling and logging.
77-81: LGTM! Clean spinner termination.The function properly handles spinner cleanup and waits for completion.
internal/exec/terraform_utils.go (3)
29-42: LGTM! Robust environment variable handling.The code properly handles TF_DATA_DIR environment variable with appropriate fallback to .terraform.
43-44: LGTM! Safe file path construction.Using filepath.Join and filepath.Clean ensures safe and platform-independent path handling.
46-59: LGTM! Comprehensive error handling and logging.The code properly checks file existence and handles all possible error cases with appropriate logging.
internal/exec/terraform_outputs.go (2)
53-79: LGTM! Thorough environment variable management.The code properly saves, sets, and restores environment variables, ensuring clean process environment.
275-283: LGTM! Clean spinner implementation.The code properly initializes and manages spinner lifecycle with appropriate logging level checks.
internal/exec/validate_stacks.go (1)
31-37: LGTM! Clean spinner integration.The code properly uses the new spinner utilities with appropriate cleanup using defer.
examples/quick-start-advanced/Dockerfile (1)
2-2: Update Geodesic Version.
The GEODESIC_VERSION is now set to 4.0.1, which aligns with the new tool improvements. Please verify that the new version is fully compatible with your build and runtime environments, as this change could introduce differences in behavior.tests/fixtures/scenarios/complete/stacks/catalog/terraform/template-functions-test/defaults.yaml (1)
35-37: New Environment Variables Added.
The addition of the env section withTEST1: aandTEST2: bis a clear step towards enhancing configuration management for the testing environment. Confirm that downstream components correctly reference these variables during execution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/utils/map_utils.go (1)
45-55: Consider adding error handling for non-string keys.The function silently skips keys that can't be converted to strings. Consider either:
- Returning an error when non-string keys are encountered
- Adding logging to track skipped keys
func MapOfInterfaceKeysToMapOfStringKeys(input map[any]any) map[string]any { converted := make(map[string]any, len(input)) for key, value := range input { strKey, ok := key.(string) - if ok { - converted[strKey] = value - } + if !ok { + l.Debug("Skipping non-string key", "key", key) + continue + } + converted[strKey] = value } return converted }internal/exec/terraform_outputs.go (2)
189-190: Consider using consistent error logging format.The error logging format varies between lines 189 and 202. Standardize on the structured format:
- l.Error("Error converting output to YAML:", "error", err2) + l.Error("failed to convert output to YAML", "error", err2)Also applies to: 202-203
197-198: Consider using consistent debug logging format.The debug logging format varies between using
fmt.Sprintfand structured logging. Standardize on the structured format:- l.Debug(fmt.Sprintf("Converting the variable '%s' with the value\n%s\nfrom JSON to Go data type\n", k, s)) + l.Debug("Converting variable from JSON to Go data type", "key", k, "value", s)Also applies to: 205-206
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/terraform_outputs.go(10 hunks)pkg/utils/map_utils.go(2 hunks)
🧰 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 (3)
pkg/utils/map_utils.go (1)
38-43: LGTM! Clean and efficient implementation.The function efficiently uses
lo.MapEntriesfor map transformation and properly handles string conversion usingfmt.Sprintf.internal/exec/terraform_outputs.go (2)
128-140: LGTM! Well-implemented environment variables handling.The code properly:
- Checks for the existence of the env section
- Converts environment variables to the correct format
- Sets them in the Terraform executor
- Includes debug logging
241-249: LGTM! Clean spinner implementation.The spinner implementation:
- Properly initializes and cleans up resources
- Only shows spinner for appropriate log levels
- Uses defer for guaranteed cleanup
|
These changes were released in v1.160.3. |
* updates * updates * updates * updates * updates * updates * Update internal/exec/terraform_outputs.go Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]> * Update internal/exec/terraform_outputs.go Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]> * Update internal/exec/terraform_outputs.go Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]> * Update internal/exec/terraform_outputs.go Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]> * Update internal/exec/terraform_outputs.go Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]> * Update internal/exec/terraform_outputs.go Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]> * Update internal/exec/terraform_outputs.go Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]> * Update internal/exec/terraform_outputs.go Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]> * updates * updates * updates * updates * updates * updates * updates * updates * updates * updates * updates * updates * updates --------- Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
| opts = []tea.ProgramOption{tea.WithoutRenderer(), tea.WithInput(nil)} | ||
| u.LogTrace("No TTY detected. Falling back to basic output. This can happen when no terminal is attached or when commands are pipelined.") | ||
| fmt.Println(message) | ||
| if atmosConfig.Logs.Level == u.LogLevelTrace || atmosConfig.Logs.Level == u.LogLevelDebug { |
There was a problem hiding this comment.
This conditional does not belong here. It undoes the feature here: #947
| p.Quit() | ||
| <-spinnerDone | ||
| fmt.Printf("\r✓ %s\n", message) | ||
| if atmosConfig.Logs.Level == u.LogLevelTrace || atmosConfig.Logs.Level == u.LogLevelDebug { |
There was a problem hiding this comment.
This conditional does not belong here.
what
envsection) when running!terraform.outputandatmos.ComponentTF_DATA_DIRenvironment variable is set, clear the correct environment folder when reseting Terraform workspaceswhy
!terraform.outputandatmos.Componentfunctions sets environment variables (in theenvsection), set the environment variables in the process running!terraform.outputandatmos.ComponentfunctionsTF_DATA_DIRenvironment variable when reseting Terraform workspaces in the!terraform.outputandatmos.Componentfunctionsrelated
Summary by CodeRabbit
Summary by CodeRabbit
Chores
New Features
Refactor