feat: add spinner during atmos validate stacks operations
#1005
Conversation
📝 WalkthroughWalkthroughThe pull request integrates a spinner UI element into the Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Cmd as ExecuteValidateStacksCmd
participant Spinner as Spinner Goroutine
participant Validate as ValidateStacks
User->>Cmd: Invoke "atmos validate stacks"
Cmd->>Spinner: Initialize and start spinner concurrently
Cmd->>Validate: Process arguments and validate stacks
Validate-->>Cmd: Return result or error
Cmd->>Spinner: Stop spinner based on execution outcome
Cmd-->>User: Return output with validation status
Assessment against linked issues
Suggested labels
✨ 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: 0
🧹 Nitpick comments (2)
internal/exec/validate_stacks.go (2)
33-44: Consider enhancing the fallback message for non-TTY environments.The non-TTY fallback is well-implemented, but the trace message could be more informative.
- u.LogTrace("No TTY detected. Falling back to basic output. This can happen when no terminal is attached or when commands are pipelined.") + u.LogTrace("No TTY detected. Using basic output mode. Common scenarios: CI/CD pipelines, redirected output, or non-interactive terminals.")
91-97: Consider adding a success message after validation.The validation completion could be more user-friendly by providing explicit feedback.
err = ValidateStacks(atmosConfig) // Ensure spinner is stopped before returning p.Quit() <-spinnerDone + if err == nil { + fmt.Println("Stack validation completed successfully!") + } + return err
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/validate_stacks.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (3)
internal/exec/validate_stacks.go (3)
14-16: LGTM! Clean import organization.The new imports for the spinner UI components are well-organized and properly scoped.
Also applies to: 22-22
64-98: LGTM! Robust error handling with proper cleanup.The error handling is thorough, ensuring the spinner is properly stopped in all error cases and the error channel is drained.
46-62: Ensure spinner errors don't block command execution.The spinner error handling is good, but we should ensure the error channel is always drained to prevent goroutine leaks.
✅ Verification successful
I'll dive a bit deeper into this by examining the context around how the
spinnerDonechannel is used within the file. Let's pull the lines around the spinner error handling code ininternal/exec/validate_stacks.goto get a better picture.
Spinner error handling is properly managed.
The code correctly employs a buffered error channel (with capacity 1), ensuring the spinner error is sent without risking a block. Additionally, in the case of an error from processing CLI arguments, the code calls
p.Quit()and then drainsspinnerDone(<-spinnerDone), which avoids any goroutine leak. This pattern ensures that spinner errors do not block command execution.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other goroutine usages to verify consistent error handling patterns ast-grep --pattern 'go func() { $$$ spinnerDone <- err $$$ }'Length of output: 69
Script:
#!/bin/bash # Extract lines 40-70 from the file to inspect spinnerDone usage in context. sed -n '40,70p' internal/exec/validate_stacks.goLength of output: 826
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exec/validate_stacks.go (1)
33-44: Consider enhancing the spinner message.The spinner implementation looks good, but we could make the message more informative.
- message := "Validating Atmos Stacks..." + message := "Validating Atmos Stacks (this may take a few moments)..."
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/validate_stacks.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (3)
internal/exec/validate_stacks.go (3)
14-16: LGTM! Good choice of dependencies.The selection of
charmbracelet/bubblesandbubbleteafor the spinner implementation is solid. These are well-maintained libraries with good community support.Also applies to: 22-22
46-62: LGTM! Robust spinner implementation.The spinner implementation is well-structured with:
- Proper error handling
- Non-blocking execution using goroutine
- Clean error propagation through channel
64-98: LGTM! Thorough error handling and cleanup.The error handling is comprehensive with proper cleanup of spinner resources in all code paths:
- Command line args processing errors
- Config initialization errors
- Stack validation errors
osterman
added
the
no-release
|
We'll release this alongside some other things going out this week. |
|
Awesome, thank you! |
why
√ . [infra] (HOST) workspace ⨠ atmos validate stacksThen:
√ . [infra] (HOST) workspace ⨠ atmos validate stacks all stacks validated successfullywhat
atmos validate stackscommandatmos validate stacksoperations #1003references
Summary by CodeRabbit