Conversation
…-also-show-terraform-help
dev-2821-atmos-terraform-help-should-also-show-terraform-help
…-also-show-terraform-help
…terraform-help' of https://github.com/cloudposse/atmos into feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help
…-also-show-terraform-help
…terraform-help' of https://github.com/cloudposse/atmos into feature/dev-2821-atmos-terraform-help-should-also-show-terraform-help
…-also-show-terraform-help
…-also-show-terraform-help
…-also-show-terraform-help
|
💥 This pull request now has conflicts. Could you fix it @samtholiya? 🙏 |
|
@samtholiya please resolve the conflicts |
dev-2896-incorrect-output-for-atmos-validate
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
cmd/root.go (2)
133-145: Consider adding input validation.The function handles the happy path well, but could benefit from input validation.
Consider this enhancement:
func getInvalidCommandName(input string) string { + if input == "" { + return "" + } // Regular expression to match the command name inside quotes re := regexp.MustCompile(`unknown command "([^"]+)"`)
Line range hint
179-207: Consider simplifying help function logic.The help function handles errors well but the logic flow could be more straightforward.
Consider extracting the logo printing logic into a separate function to reduce duplication:
+func printAtmosLogo() error { + fmt.Println() + return tuiUtils.PrintStyledText("ATMOS") +} + RootCmd.SetHelpFunc(func(command *cobra.Command, args []string) { // ... existing conditions ... - fmt.Println() - err := tuiUtils.PrintStyledText("ATMOS") + err := printAtmosLogo() if err != nil { u.LogErrorAndExit(atmosConfig, err) } // ... rest of the function ...cmd/terraform.go (1)
39-41: Consider using handleHelpRequest for consistent help handling.Since this PR aims to standardize help and usage content, consider using the new
handleHelpRequestutility function mentioned in the PR summary.if info.NeedHelp { - actualCmd.Usage() - return + return handleHelpRequest(actualCmd) }🧰 Tools
🪛 golangci-lint (1.62.2)
40-40: Error return value of
actualCmd.Usageis not checked(errcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
cmd/root.go(6 hunks)cmd/terraform.go(3 hunks)cmd/version.go(1 hunks)go.mod(0 hunks)internal/tui/templates/templater.go(5 hunks)pkg/utils/log_utils.go(1 hunks)tests/snapshots/TestCLICommands_check_atmos_--help_in_empty-dir.stdout.golden(3 hunks)tests/test-cases/core.yaml(1 hunks)tests/test-cases/empty-dir.yaml(2 hunks)
💤 Files with no reviewable changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/version.go
- tests/test-cases/core.yaml
- pkg/utils/log_utils.go
- internal/tui/templates/templater.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/terraform.go
40-40: Error return value of actualCmd.Usage is not checked
(errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (7)
tests/snapshots/TestCLICommands_check_atmos_--help_in_empty-dir.stdout.golden (2)
28-31: Well done on adding command aliases!The addition of aliases
hfandtffor commonly used commands will improve developer productivity. This is a great quality-of-life enhancement.
6-6: Strong work on standardizing command terminology!The consistent use of
[sub-command]notation and updated example usage improves clarity. This aligns well with the PR's objective of standardizing help content.Also applies to: 59-59, 62-62
tests/test-cases/empty-dir.yaml (2)
31-31: Good catch on the exit code!Changing the exit code to 1 when config is missing is more appropriate. This follows the Unix convention where non-zero exit codes indicate errors.
45-45: Consistent terminology in test expectations.The updated help text matches the new standardized terminology using "subcommand". This maintains consistency across the codebase.
cmd/root.go (1)
7-8: LGTM! Good additions for enhanced functionality.The new imports and FParseErrWhitelist field are well-chosen for improving command parsing flexibility and error handling.
Also applies to: 13-13, 30-30
cmd/terraform.go (2)
7-7: LGTM! Clean import addition.The cobra import is properly formatted and essential for the CLI functionality.
22-24: Verify hook execution with the new config info retrieval!The refactoring looks good! The use of
getConfigAndStacksInfosimplifies the implementation while maintaining hook functionality.Let's verify the hook execution still works as expected:
✅ Verification successful
Hook execution verified - Implementation is battle-ready! 💪
The refactoring maintains a solid execution flow:
getConfigAndStacksInfoproperly processes command arguments and generates the required configurationhooks.RunEcorrectly receives and handles the configuration info- Error handling is robust throughout the execution chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if hooks are properly registered and called rg -A 5 "RunE.*info" pkg/hooks/Length of output: 357
Script:
#!/bin/bash # Check getConfigAndStacksInfo implementation ast-grep --pattern 'func getConfigAndStacksInfo($$$) $$$' # Look for hook-related code and tests rg -g '!vendor' -A 5 'hooks\.' cmd/ rg -g '!vendor' -l 'hooks.*test' .Length of output: 1409
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/test-cases/log-level-validation.yaml (1)
Warrior, your instincts serve you well. These test changes indeed stray from our mission.
The log level validation tests appear to be unrelated to the PR's objective of standardizing help and usage content. The code search reveals that while log level validation is an existing feature in the codebase (in pkg/config/), these test additions are not connected to any help/usage content standardization efforts.
🔗 Analysis chain
Line range hint
1-80: Verify alignment with PR objectives.Spartan, these test changes seem unrelated to the PR's objective of standardizing help and usage content. The modifications to log level validation behavior appear to be outside the scope of the current changes.
Let me check if there are related changes in other files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related changes in help/usage standardization rg -l "cobra.NoArgs|handleHelpRequest|showUsageAndExit" --type go # Check if log level validation is modified elsewhere git diff | grep -A 5 "logs-level"Length of output: 904
Script:
#!/bin/bash # Search for log level validation code ast-grep --pattern 'Invalid log level.*Valid options' # Search for log level related code rg "ATMOS_LOGS_LEVEL|--logs-level" --type go -A 3 # Check for other test changes in the same PR git diff -- 'tests/**/*.yaml'Length of output: 1298
Script:
#!/bin/bash # Search for help/usage message changes git diff -U0 | rg "Usage:|Error:" -B2 -A2 # Check for cobra command changes related to error messages rg "cobra.*Error" --type go -A 3Length of output: 78
🧹 Nitpick comments (1)
tests/snapshots/TestCLICommands_atmos_--help.stdout.golden (1)
Line range hint
1-62: Consider adding examples for the new aliases.While the aliases are well-documented, consider enhancing the example section to demonstrate their usage. This would help users quickly understand how to leverage these shortcuts.
Add examples like:
atmos atmos [subcommand] <component> -s <stack> -- <native-flags> + + Examples using aliases: + atmos tf plan <component> -s <stack> # Same as: atmos terraform plan + atmos hf diff <component> -s <stack> # Same as: atmos helmfile diff
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(0 hunks)tests/snapshots/TestCLICommands_atmos_--help.stdout.golden(3 hunks)tests/test-cases/log-level-validation.yaml(3 hunks)
💤 Files with no reviewable changes (1)
- go.mod
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (5)
tests/snapshots/TestCLICommands_atmos_--help.stdout.golden (4)
6-6: LGTM! Clear and consistent usage syntax.The updated syntax
atmos [sub-command] [flags]is more precise and aligns with the standardization goals. The explicit mention of[flags]helps users understand all possible command elements.
28-32: Excellent addition of command aliases!The new SubCommand Aliases section introduces helpful shortcuts (
hffor helmfile,tffor terraform) that will improve the CLI experience. These aliases target frequently used commands, making the CLI more efficient to use.
59-59: LGTM! Example follows the new terminology.The example usage maintains consistency with the new
[subcommand]terminology while preserving the clear structure for component and stack parameters.
62-62: LGTM! Help reference matches new terminology.The help command reference consistently uses
[subcommand], maintaining the standardized terminology throughout the documentation.tests/test-cases/log-level-validation.yaml (1)
48-48:⚠️ Potential issueReview test expectations for valid log levels.
Spartan, these changes appear to be introducing incorrect behavior. The test cases for valid log levels (in config file, environment variable, and command line flag) now expect failure (exit code 1) where they previously expected success (exit code 0). Valid inputs should succeed, not fail.
Consider the following points:
- Valid log levels like "Debug" and "Info" are legitimate values
- Failing on valid input would break existing functionality
- This change contradicts standard CLI behavior where valid inputs succeed
Let's verify the intended behavior:
Also applies to: 64-64, 80-80
|
These changes were released in v1.152.0. |
what
Example usage scenarios:
Example help scenarios:
Example atmos config error code
Example alias subCommand section
why
references
Summary by CodeRabbit
Release Notes
Command Line Interface Improvements
New Features
helmfile applyhelmfile destroyhelmfile diffhelmfile syncCommand Behavior Changes
Dependency Updates
coloredcobralibrary.golang.org/x/oauth2dependency.Usability Enhancements