increment ATMOS_SHLVL each time atmos terraform shell is called #803
Conversation
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
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
🧹 Outside diff range and nitpick comments (2)
website/docs/cli/commands/terraform/terraform-shell.mdx (1)
42-44: Consider enhancing the documentation with additional details.The documentation could be even more helpful with these additions:
- Add an example showing the
ATMOS_SHLVLbehavior:- If ATMOS_SHLVL is already set, it increments the value by 1 for each new shell. + +For example: +```shell +$ echo $ATMOS_SHLVL +1 +$ atmos terraform shell component-1 -s stack-1 +$ echo $ATMOS_SHLVL +2 +```
- Document what happens to
ATMOS_SHLVLwhen exiting the shell- Clarify if there's a maximum nesting level
internal/exec/shell_utils.go (1)
154-165: Consider adding bounds checking for ATMOS_SHLVL.While the implementation correctly handles the basic case, consider adding validation for edge cases:
- Negative values should be normalized to 0 before incrementing
- Consider adding an upper bound (e.g., 1000) to prevent potential integer overflow
Here's a suggested improvement:
atmosShellLvl := os.Getenv("ATMOS_SHLVL") atmosShellVal := 1 if atmosShellLvl != "" { val, err := strconv.Atoi(atmosShellLvl) if err != nil { return err } + // Normalize negative values + if val < 0 { + val = 0 + } + // Prevent integer overflow + if val >= 1000 { + return fmt.Errorf("ATMOS_SHLVL value too high: %d", val) + } atmosShellVal = val + 1 }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
internal/exec/shell_utils.go(2 hunks)website/docs/cli/commands/terraform/terraform-shell.mdx(1 hunks)
🔇 Additional comments (3)
website/docs/cli/commands/terraform/terraform-shell.mdx (1)
42-44: LGTM! Clear and concise documentation of the ATMOS_SHLVL behavior.
The documentation effectively explains how the ATMOS_SHLVL environment variable tracks shell nesting levels, making it clear for users to understand the behavior.
internal/exec/shell_utils.go (2)
12-12: LGTM! Clean import addition.
The strconv import is correctly placed and necessary for the new ATMOS_SHLVL parsing functionality.
154-171: Verify test coverage and usage.
Let's ensure proper test coverage exists and check for other uses of ATMOS_SHLVL:
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
internal/exec/shell_utils.go (1)
154-186: Consider extracting ATMOS_SHLVL management into helper functions.The ATMOS_SHLVL management logic could be more modular and reusable. Consider extracting it into helper functions:
+// incrementAtmosShellLevel increments ATMOS_SHLVL and returns any error +func incrementAtmosShellLevel() error { + atmosShellLvl := os.Getenv("ATMOS_SHLVL") + atmosShellVal := 1 + if atmosShellLvl != "" { + val, err := strconv.Atoi(atmosShellLvl) + if err != nil { + return err + } + atmosShellVal = val + 1 + } + return os.Setenv("ATMOS_SHLVL", fmt.Sprintf("%d", atmosShellVal)) +} + +// decrementAtmosShellLevel decrements ATMOS_SHLVL and logs any errors +func decrementAtmosShellLevel(cliConfig schema.CliConfiguration) { + atmosShellLvl := os.Getenv("ATMOS_SHLVL") + if atmosShellLvl == "" { + return + } + val, err := strconv.Atoi(atmosShellLvl) + if err != nil { + u.LogWarning(cliConfig, fmt.Sprintf("Failed to parse ATMOS_SHLVL: %v", err)) + return + } + newVal := val - 1 + if newVal < 0 { + newVal = 0 + } + if err := os.Setenv("ATMOS_SHLVL", fmt.Sprintf("%d", newVal)); err != nil { + u.LogWarning(cliConfig, fmt.Sprintf("Failed to update ATMOS_SHLVL: %v", err)) + } +}Then in
execTerraformShellCommand:- atmosShellLvl := os.Getenv("ATMOS_SHLVL") - atmosShellVal := 1 - if atmosShellLvl != "" { - val, err := strconv.Atoi(atmosShellLvl) - if err != nil { - return err - } - atmosShellVal = val + 1 - } - if err := os.Setenv("ATMOS_SHLVL", fmt.Sprintf("%d", atmosShellVal)); err != nil { - return err - } + if err := incrementAtmosShellLevel(); err != nil { + return err + } // decrement the value after exiting the shell defer func() { - atmosShellLvl := os.Getenv("ATMOS_SHLVL") - if atmosShellLvl == "" { - return - } - val, err := strconv.Atoi(atmosShellLvl) - if err != nil { - u.LogWarning(cliConfig, fmt.Sprintf("Failed to parse ATMOS_SHLVL: %v", err)) - return - } - newVal := val - 1 - if newVal < 0 { - newVal = 0 - } - if err := os.Setenv("ATMOS_SHLVL", fmt.Sprintf("%d", newVal)); err != nil { - u.LogWarning(cliConfig, fmt.Sprintf("Failed to update ATMOS_SHLVL: %v", err)) - } + decrementAtmosShellLevel(cliConfig) }()This refactoring would:
- Make the code more maintainable and reusable
- Simplify testing of the ATMOS_SHLVL logic
- Improve readability of the main function
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
internal/exec/shell_utils.go(2 hunks)
🔇 Additional comments (2)
internal/exec/shell_utils.go (2)
167-186: Excellent implementation of error handling in defer block!
The code successfully addresses all the concerns raised in the previous review:
- Proper error handling for parsing ATMOS_SHLVL
- Prevention of negative values
- Warning logs for errors
- Early returns on invalid states
154-165: LGTM! Implementation matches Bash's SHLVL behavior.
The code correctly handles the ATMOS_SHLVL initialization and increment logic with proper error handling.
Let's verify that the behavior matches Bash's SHLVL:
✅ Verification successful
ATMOS_SHLVL implementation is correct and well-documented
The implementation perfectly aligns with the documented behavior in the docs:
- Initializes to 1 when not set
- Increments by 1 for each new shell
- Includes proper error handling and cleanup
The code matches the documented behavior in website/docs/cli/commands/terraform/terraform-shell.mdx, and the implementation in internal/exec/shell_utils.go follows the same pattern as Bash's SHLVL, making it familiar to users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare ATMOS_SHLVL behavior with Bash's SHLVL
# Test: Search for SHLVL usage in shell scripts to understand standard patterns
rg -g '*.sh' 'SHLVL' -A 3 -B 3
Length of output: 30
Script:
#!/bin/bash
# Let's verify the ATMOS_SHLVL implementation by checking:
# 1. All references to ATMOS_SHLVL in the codebase
# 2. The shell execution context where it's used
# 3. Any tests that verify this behavior
# Search for all ATMOS_SHLVL references
echo "=== ATMOS_SHLVL References ==="
rg "ATMOS_SHLVL"
# Check the shell execution context
echo -e "\n=== Shell Execution Context ==="
ast-grep --pattern 'func execTerraformShellCommand($$$) {
$$$
}'
# Look for related test files
echo -e "\n=== Related Test Files ==="
fd "shell.*test\.go$"
Length of output: 1297
|
@pkbhowmick please add screenshots to demonstrate functionality |
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]>
|
Here is the working demo @osterman |
what
why
references
Summary by CodeRabbit
New Features
ATMOS_SHLVLenvironment variable for improved shell command execution.ATMOS_SHLVLwith each new shell instance to track nesting levels.Documentation
atmos terraform shellcommand to clarify the newATMOS_SHLVLfunctionality and its impact on shell nesting levels.