add support for disabling custom terraform shell prompt #786
Conversation
pkbhowmick
force-pushed
the
pulak/DEV-2743
branch
from
19dffc8 to
205ffb9
Compare
Signed-off-by: Pulak Kanti Bhowmick <pkbhowmick007@gmail.com>
Signed-off-by: Pulak Kanti Bhowmick <pkbhowmick007@gmail.com>
pkbhowmick
force-pushed
the
pulak/DEV-2743
branch
from
e6a71b0 to
8d06b14
Compare
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the shell prompt configuration in the Atmos CLI. Specifically, the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
pkg/schema/schema.go (1)
79-81: Consider adding field documentation.The
ShellConfigstruct is well-designed with a focused responsibility. To maintain consistency with Go best practices, consider adding a documentation comment to describe the purpose of thePromptfield.type ShellConfig struct { + // Prompt defines the custom shell prompt string. If empty, the default prompt is used. Prompt string `yaml:"prompt" json:"prompt" mapstructure:"prompt"` }
🛑 Comments failed to post (1)
internal/exec/shell_utils.go (1)
158-160: 💡 Codebase verification
⚠️ Potential issueShell prompt environment variable needs PS1 prefix
Greetings, fellow code warrior! Our reconnaissance has revealed that the shell prompt is indeed being set without the crucial
PS1=prefix. The configuration struct shows it's a raw string value, and there are no existingPS1environment variable settings in the codebase.Let's fortify this code:
if cliConfig.Components.Terraform.Shell.Prompt != "" { - componentEnvList = append(componentEnvList, cliConfig.Components.Terraform.Shell.Prompt) + componentEnvList = append(componentEnvList, fmt.Sprintf("PS1=%s", cliConfig.Components.Terraform.Shell.Prompt)) }This change:
- Properly formats the shell prompt as an environment variable
- Maintains compatibility with standard shell behavior
- Ensures the prompt will be correctly interpreted by the shell
🔗 Analysis chain
Ensure proper shell prompt environment variable format
Greetings, warrior! While the implementation aligns well with making the prompt optional, we should ensure the prompt is properly formatted when added to the environment variables.
Consider this battle-tested approach:
if cliConfig.Components.Terraform.Shell.Prompt != "" { - componentEnvList = append(componentEnvList, cliConfig.Components.Terraform.Shell.Prompt) + componentEnvList = append(componentEnvList, fmt.Sprintf("PS1=%s", cliConfig.Components.Terraform.Shell.Prompt)) }This ensures:
- The prompt is properly prefixed with
PS1=- Consistent behavior across different shells
Let's verify the shell prompt handling in other parts of the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other shell prompt configurations rg -l 'PS1=' # Search for any shell environment variable settings ast-grep --pattern 'componentEnvList = append($_, $_)'Length of output: 1003
Script:
#!/bin/bash # Let's check the configuration struct to understand how the prompt is defined rg -A 5 "type.*Shell.*struct" # Also check for any documentation about the shell prompt format rg -i "shell.*prompt|prompt.*shell"Length of output: 722
|
@pkbhowmick please update docs on atmos shell |
|
@pkbhowmick please post screenshots with it enabled and disabled |
|
@coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
internal/exec/shell_utils.go (1)
159-162: Document the default prompt behavior.Since this change introduces configurable prompts, we should ensure the behavior is well-documented. Consider adding code comments explaining:
- The default behavior when no prompt is specified
- The format expectations for custom prompts
- Any limitations or security considerations
+// hasCustomShellPrompt determines if a user-defined shell prompt should be used. +// When no prompt is specified, the default Terraform shell prompt is maintained. +// Custom prompts should avoid shell metacharacters for security. hasCustomShellPrompt := cliConfig.Components.Terraform.Shell.Prompt != ""pkg/schema/schema.go (1)
79-81: LGTM! Well-structured ShellConfig type.The new
ShellConfigtype is cleanly defined with appropriate struct tags. ThePromptfield provides a clear configuration point for customizing the shell prompt.Consider adding validation for the prompt string in the future if there are any format requirements or restrictions that should be enforced.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
internal/exec/shell_utils.go(1 hunks)pkg/schema/schema.go(1 hunks)
🔇 Additional comments (1)
pkg/schema/schema.go (1)
69-77: LGTM! Clean addition of the Shell configuration field.
The reformatting and addition of the Shell field to the Terraform struct follows consistent patterns with other fields. The struct tags are properly defined for YAML, JSON, and mapstructure serialization.
Hi @osterman, could you please give me a pointer in the doc where to update? Should I add a new section for it? |
|
Without the prompt, we should use the login shell like before. Also show screenshots using atmos templates. |
Signed-off-by: Pulak Kanti Bhowmick <pkbhowmick007@gmail.com>
Hi @osterman templating is bit tricky here as user can put anything on config. But for now code can handle any combination of stack and component name With template:
|
Great, I wanted to confirm this worked. Please add as an example to the docs. |
|
Share an updated screenshot of the login shell working. |
Signed-off-by: Pulak Kanti Bhowmick <pkbhowmick007@gmail.com>
Here is the login shell example:
|
|
These changes were released in v1.107.0. |
what
why
Working demo
With custom prompt:
Without custom prompt:
Closes #779 (disable new prompt by default until we make it work better with geodesic)
references
atmos terraform shellmode #761atmos terraform shell#495Summary by CodeRabbit
Summary by CodeRabbit
New Features
ShellConfigtype to support the new shell prompt configuration.Bug Fixes