-
-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
change PS1 to show that atmos is in the atmos terraform shell
mode
#761
Conversation
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
internal/exec/shell_utils.go (2)
157-157
: Consider enhancing the PS1 prompt with more context.The PS1 change is a solid improvement, warrior! However, we can make it even more informative by including the stack and component names. This would further prevent confusion when working with multiple stacks.
- componentEnvList = append(componentEnvList, "PS1=atmos>") + componentEnvList = append(componentEnvList, fmt.Sprintf("PS1=[atmos:%s/%s]> ", stack, component))
Line range hint
192-201
: Add error handling for shell command resolution.The shell command resolution is robust, but we should handle the case where neither SHELL env var is set nor bash is found.
if len(shellCommand) == 0 { bashPath, err := exec.LookPath("bash") if err != nil { + // Try fallback to sh if bash is not available + shPath, shErr := exec.LookPath("sh") + if shErr != nil { + return fmt.Errorf("no suitable shell found: %v", err) + } + shellCommand = shPath + } else { return err + } } shellCommand = bashPath }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
internal/exec/shell_utils.go
(1 hunks)
🔇 Additional comments (1)
internal/exec/shell_utils.go (1)
Line range hint 159-165
: LGTM! Clear and informative debug logging.
The debug logging provides excellent context about the shell environment. This helps users understand exactly where they are and what variables are set.
Also see the @coderabbitai suggestions to enhance the prompt and improve error handling if shell is not set |
Tip For best results, initiate chat on the files or code changes. @osterman: To enhance the prompt, we can adjust the componentEnvList = append(componentEnvList, fmt.Sprintf("PS1=atmos-%s> ", stackName)) Additionally, improving error handling by checking if the if shellCommand == "" {
return fmt.Errorf("no shell found to launch the interactive session")
} These changes will enhance the user experience by providing clearer context and preventing errors when the shell is not configured properly. |
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
For |
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
with |
What's the full command that you tried? |
I just added |
@pkbhowmick what are the next steps here? |
@pkbhowmick this works for me: PS1="Foobar> " zsh -d -f -i
Foobar>
Foobar> |
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
Hi @osterman, thanks a lot. It is working now |
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @pkbhowmick
These changes were released in v1.104.0. |
what
why
references
New Features
Bug Fixes
-l
flag for non-Windows systems and implementing a fallback tosh
ifbash
is unavailable./bin/zsh
shell with additional flags.Closes #495