Fix Terraform help printing log twice and causing error #798
Conversation
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on modifying the command execution flow within the Atmos CLI application. Key alterations include the removal of 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
|
![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 (5)
cmd/terraform.go (2)
20-27: LGTM! This effectively addresses the double logo issue.The new help detection block provides an early exit path that prevents the logo from being printed twice. This aligns perfectly with the PR objectives.
Consider making the help detection more robust with this enhancement:
-if cmd.Flags().Changed("help") || (len(args) > 0 && args[0] == "help") { +if cmd.Flags().Changed("help") || cmd.Flags().Changed("h") || (len(args) > 0 && (args[0] == "help" || args[0] == "--help" || args[0] == "-h")) {This would catch additional common help invocation patterns.
22-25: Consider enhancing error handling for better user experience.The current error handling is functional but could be more specific about what went wrong during help display.
Consider wrapping the error for more context:
err := e.ExecuteTerraformCmd(cmd, args, nil) if err != nil { - u.LogErrorAndExit(schema.CliConfiguration{}, err) + u.LogErrorAndExit(schema.CliConfiguration{}, fmt.Errorf("failed to display help: %w", err)) }cmd/root.go (3)
51-51: Early return after error is correct but redundant.The early return after
LogErrorAndExitis unnecessary sinceLogErrorAndExitalready terminates the program. While not harmful, it's unreachable code.Consider removing the redundant return:
- return
Line range hint
147-160: Help command may still print logo twice.The
SetHelpFuncimplementation still prints the logo, which could lead to double printing in combination with other logo printing points. Based on the PR objectives to fix the double logo printing issue, we should consolidate the logo printing logic.Consider this refactor to ensure single logo printing:
RootCmd.SetHelpFunc(func(command *cobra.Command, strings []string) { - // Print a styled Atmos logo to the terminal - fmt.Println() - err := tuiUtils.PrintStyledText("ATMOS") - if err != nil { - u.LogErrorAndExit(schema.CliConfiguration{}, err) - } - b.HelpFunc(command, strings) command.Usage() })
Multiple logo printing locations identified across the codebase
The logo is printed in several locations:
cmd/root.go: Two instances - in the main execution and help functioncmd/version.go: In the version commandcmd/cmd_utils.go: In utility functionsinternal/exec/terraform.go: Two instances - help and default commandinternal/exec/helmfile.go: In default commandThese multiple print locations could lead to duplicate logos appearing. A centralized approach for logo printing should be considered to maintain consistency and prevent duplicates.
🔗 Analysis chain
Line range hint
147-160: Verify logo printing across the codebase.Let's ensure there are no other locations that might cause double logo printing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all potential logo printing locations echo "Searching for logo printing calls..." rg -l "PrintStyledText.*ATMOS" --type go # Search for help-related functions that might trigger logo printing echo "Searching for help-related functions..." rg "SetHelpFunc|help.*func|RunE.*help" --type goLength of output: 444
Script:
#!/bin/bash # Let's examine the logo printing implementations in each file echo "=== Logo printing in cmd/root.go ===" rg "PrintStyledText.*ATMOS" -A 3 -B 3 cmd/root.go echo -e "\n=== Logo printing in cmd/version.go ===" rg "PrintStyledText.*ATMOS" -A 3 -B 3 cmd/version.go echo -e "\n=== Logo printing in cmd/cmd_utils.go ===" rg "PrintStyledText.*ATMOS" -A 3 -B 3 cmd/cmd_utils.go echo -e "\n=== Logo printing in internal/exec/terraform.go ===" rg "PrintStyledText.*ATMOS" -A 3 -B 3 internal/exec/terraform.go echo -e "\n=== Logo printing in internal/exec/helmfile.go ===" rg "PrintStyledText.*ATMOS" -A 3 -B 3 internal/exec/helmfile.goLength of output: 2218
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
cmd/root.go(1 hunks)cmd/terraform.go(1 hunks)internal/exec/terraform.go(1 hunks)
🔇 Additional comments (4)
cmd/terraform.go (1)
20-27: Verify the fix resolves the double logo issue.
The implementation looks correct, but let's verify it resolves the issue completely.
✅ Verification successful
Fix successfully prevents double logo printing
The implementation is verified to be correct. The changes in cmd/terraform.go ensure that help requests are handled early, before any logo printing occurs. The flow now works as follows:
- Help flag check happens first in terraform command
- If help is requested, it directly calls
ExecuteTerraformCmdand returns - Logo printing only happens once through the normal command flow
The code structure prevents any possibility of double logo printing while maintaining the help functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining double logo printing logic
# Search for logo printing patterns
echo "Checking for logo printing patterns..."
rg -l "PrintLogo|logo" --type go
# Search for help text display patterns
echo "Checking for help text display patterns..."
rg -l "help.*print|print.*help" --type go
Length of output: 444
Script:
#!/bin/bash
# Let's check the actual logo printing implementation and help handling
# Check logo printing in cmd/root.go
echo "=== Logo printing in cmd/root.go ==="
rg "PrintLogo" cmd/root.go -C 3
# Check help handling in internal/exec/terraform.go
echo -e "\n=== Help handling in internal/exec/terraform.go ==="
rg "help" internal/exec/terraform.go -C 3
# Check the ExecuteTerraformCmd implementation
echo -e "\n=== ExecuteTerraformCmd implementation ==="
ast-grep --pattern 'func (e *Executor) ExecuteTerraformCmd($$$) {
$$$
}'
Length of output: 643
Script:
#!/bin/bash
# Let's examine the complete flow in cmd/terraform.go and root.go
# Check the complete terraform.go implementation
echo "=== Complete terraform.go implementation ==="
cat cmd/terraform.go
# Check the complete root.go implementation focusing on logo and help
echo -e "\n=== Root command implementation ==="
rg -B 5 -A 10 "func Execute|PrintLogo|RunE:" cmd/root.go
Length of output: 2350
cmd/root.go (1)
Line range hint 26-39: Excellent handling of help flags!
The PersistentPreRun function properly handles both help command and help flag cases, ensuring correct usage and error output behavior. This is a robust implementation that follows CLI best practices.
internal/exec/terraform.go (2)
41-49: Strong error handling implementation!
The error handling for both logo printing and help processing is thorough and follows best practices.
38-39: Verify all help command flows in the codebase
Let's ensure this change doesn't affect other help command flows.
✅ Verification successful
Help command flows are consistent and properly handled
The verification shows a consistent pattern across the codebase:
NeedHelpflag is properly handled in both terraform and helmfile components- Logo printing with
PrintStyledText("ATMOS")is consistently implemented across help flows - Help processing is handled through the centralized
processHelpfunction
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check all help command flows and logo printing logic
# Check for NeedHelp flag usage
echo "Checking NeedHelp flag usage:"
rg "NeedHelp" -A 3
# Check for logo printing
echo -e "\nChecking logo printing:"
rg "PrintStyledText.*ATMOS" -A 3
# Check for help processing
echo -e "\nChecking help processing:"
rg "processHelp" -A 3
Length of output: 4565
|
@Cerebrovinny can you please post screenshots before/after. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
pkg/config/config.go (1)
Line range hint
137-157: Consider extracting repeated error handling pattern.The error handling pattern is repeated multiple times throughout the config file processing. Consider creating a helper function to reduce duplication.
Here's a suggested helper function:
func handleConfigFileProcessing(cliConfig schema.CliConfiguration, configFile string, v *viper.Viper) (bool, error) { found, err := processConfigFile(cliConfig, configFile, v) if err != nil { return false, err } return found, nil }This would simplify the repeated pattern to:
found, err := handleConfigFileProcessing(cliConfig, configFile1, v) if err != nil { return cliConfig, err } if found { configFound = true }cmd/cmd_utils.go (1)
Line range hint
466-468: Consider making version command detection more robust.While the current implementation works for direct version commands, it might be worth considering subcommands that contain 'version' as well.
Consider this more robust implementation:
-func isVersionCommand() bool { - return len(os.Args) > 1 && os.Args[1] == "version" +func isVersionCommand() bool { + if len(os.Args) <= 1 { + return false + } + // Check both direct version command and subcommands + return os.Args[1] == "version" || (len(os.Args) > 2 && os.Args[2] == "version") }internal/exec/terraform.go (2)
Line range hint
42-53: Consider consolidating error handlingWhile the error handling is good, we can make it more concise:
- err := tuiUtils.PrintStyledText("ATMOS") - if err != nil { - return err - } - - err = processHelp(schema.CliConfiguration{}, "terraform", "") - if err != nil { - return err - } + if err := tuiUtils.PrintStyledText("ATMOS"); err != nil { + return err + } + + if err := processHelp(schema.CliConfiguration{}, "terraform", ""); err != nil { + return err + }
62-72: Add clarifying comment for validation skip logicThe conditional validation is correct, but let's add a comment to explain why help commands skip validation:
- // Skip stack validation for help commands + // Skip stack validation for help commands since they don't require a valid stack context + // This allows users to access help documentation even without a valid configurationinternal/exec/utils.go (2)
636-636: Enhance the comment to be more specific about command handling.The current comment could be more descriptive about which specific commands trigger help display.
-// For commands like `atmos terraform help`, `atmos terraform clean` and `atmos terraform plan`, show the command help +// Handle single-argument commands that should display help: +// - Explicit help command: `atmos terraform help` +// - Built-in commands: `atmos terraform clean`, `atmos terraform plan` +// This prevents the logo from being displayed twice when showing help
639-645: LGTM! The help command handling logic looks solid.The changes effectively address the double logo issue by:
- Explicitly handling the "help" command case
- Setting appropriate flags to ensure consistent help display
- Preventing duplicate processing that could cause the logo to appear twice
Consider fixing the indentation to be consistent with the rest of the file:
if info.SubCommand == "help" { info.NeedHelp = true info.SubCommand = "" info.AdditionalArgsAndFlags = []string{"--help"} -} else { - info.NeedHelp = true -} + } else { + info.NeedHelp = true + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
cmd/cmd_utils.go(2 hunks)cmd/root.go(2 hunks)cmd/terraform.go(0 hunks)internal/exec/terraform.go(2 hunks)internal/exec/utils.go(1 hunks)pkg/config/config.go(3 hunks)
💤 Files with no reviewable changes (1)
- cmd/terraform.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/root.go
🧰 Additional context used
📓 Learnings (1)
cmd/cmd_utils.go (1)
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
🔇 Additional comments (7)
pkg/config/config.go (2)
100-107: Well done! Clean implementation of help flag detection.
The early return of defaultCliConfig when help is requested effectively prevents unnecessary config processing and resolves the double logo printing issue.
133-134: Good move on improving variable scoping.
Moving the configFound declaration closer to its usage follows Go best practices and improves code maintainability.
cmd/cmd_utils.go (2)
386-390: LGTM! Clean handling of version command.
The code appropriately suppresses the missing config message for version commands, improving the user experience.
369-376: LGTM! Early exit for help commands prevents unnecessary config validation.
The implementation correctly handles all help command variants and aligns with the PR objective of fixing the double logo printing issue.
Let's verify that we catch all help command scenarios:
✅ Verification successful
Help command handling is correctly implemented and consistent across the codebase
The implementation in cmd/cmd_utils.go aligns perfectly with the project's help command patterns. The search results confirm:
- Help flags are consistently defined as constants (
HelpFlag1="-h",HelpFlag2="--help") - Help detection is uniform across multiple components (cmd, internal/exec, pkg/config)
- The early exit strategy for help commands is used consistently to prevent unnecessary operations
The implementation follows the established patterns in the codebase and correctly handles all three variants: --help, -h, and help.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify help command handling across the codebase
# Check for any other help command patterns we might have missed
rg -g '!vendor' -g '*.go' '(flag\.Bool|flag\.String|cobra.*Command).*help'
Length of output: 76
Script:
#!/bin/bash
# Let's try a broader search for help-related patterns
rg -g '!vendor' -g '*.go' -i 'help.*flag|--help|-h' -A 2 -B 2
# Also search for help command handling in main functions
ast-grep --pattern 'func main() {
$$$
help
$$$
}'
# Check for any cobra command help patterns
rg -g '!vendor' -g '*.go' 'cobra.*Command.*help|help.*cobra'
Length of output: 8797
internal/exec/terraform.go (3)
38-40: Strong improvement in help detection logic!
The consolidated help check using isHelp is more comprehensive, covering both the internal flag and CLI arguments. This is a cleaner approach that prevents help-related logic from being scattered throughout the code.
56-61: Excellent optimization of config initialization!
Moving the config initialization after the help check ensures that users can access help documentation without requiring a valid configuration. This directly addresses the issue mentioned in PR objectives.
Line range hint 38-72: Verify logo printing locations
Let's ensure there are no other instances of logo printing that could cause duplication.
✅ Verification successful
Logo printing appears to be intentional in different command contexts
The logo printing in internal/exec/terraform.go is intentional and serves a specific purpose for the help command. Each instance of logo printing occurs in distinct command contexts:
- Root command execution (
cmd/root.go) - Help function (
cmd/root.go) - Version command (
cmd/version.go) - Missing config message (
cmd/cmd_utils.go) - Terraform help (
internal/exec/terraform.go) - Helmfile help (
internal/exec/helmfile.go)
These are separate execution paths that don't overlap, ensuring the logo is printed exactly once per command execution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential logo printing calls
rg "PrintStyledText.*ATMOS" --type go
Length of output: 404
Script:
#!/bin/bash
# Let's check the context around these logo prints to understand the flow
rg "PrintStyledText.*ATMOS" -B 5 -A 5 --type go
Length of output: 3224
Cerebrovinny
force-pushed
the
fix/DEV-2739
branch
from
4541db1
to
8060b5f
Compare
| @@ -285,6 +281,16 @@ func ProcessStacks( | |||
|
|
|||
| configAndStacksInfo.StackFromArg = configAndStacksInfo.Stack | |||
|
|
|||
| // Initialize component section maps | |||
There was a problem hiding this comment.
@Cerebrovinny why this is needed? Did it cause issues w/o this?
If needed, please add more info in the comment
There was a problem hiding this comment.
processHelp func is not used anymore?
There was a problem hiding this comment.
ProcessComponentConfig is causing it to display nil pointer dereference errors in tests when we process the stack
so we need to initialize it first before accessing.
what
This fix Terraform help command causing logo to display twice and causes error
Evidences Test
Before:
After 2 flags testing (--help) and (help):
references
Summary by CodeRabbit
New Features
-help) for improved command-line assistance.Bug Fixes
Documentation