Add custom help message and upgrade notification #809
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces enhancements to the command processing functionality within 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: 2
🧹 Outside diff range and nitpick comments (2)
cmd/root.go (1)
72-78: Consider consolidating help function implementations.There are now multiple help function implementations in the file (see
initConfig()). Consider consolidating these to maintain a single source of truth for help functionality.Suggestions:
- Move all help-related logic to a dedicated helper
- Ensure the styled Atmos logo is consistently displayed
- Consider creating a chain of responsibility for help message components
cmd/cmd_utils.go (1)
477-484: Consider using semantic versioning for version comparisonDirect string comparison of versions might not accurately reflect actual version precedence, especially with multi-digit numbers. Using a semantic versioning library can provide more reliable comparisons between
latestReleaseandcurrentRelease.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
cmd/cmd_utils.go(4 hunks)cmd/root.go(1 hunks)
🔇 Additional comments (3)
cmd/root.go (1)
76-78: Verify the custom help message implementation.
The integration looks solid, but let's verify the custom help message implementation.
✅ Verification successful
Custom help message implementation is well-structured and complete
The implementation is solid and includes:
- Proper preservation of original help functionality
- Clean version comparison logic
- Well-formatted upgrade message with:
- Color-coded borders and text
- Centered content with proper padding
- Helpful links to releases and installation guide
- Clear version information display
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the custom help message function implementation
# Check for the function definition
ast-grep --pattern 'func customHelpMessageToUpgradeToAtmosLatestRelease($_, $_) {
$$$
}'
# Check for any related test files
fd -e go -e test test_.*root
Length of output: 895
Script:
#!/bin/bash
# Let's verify the complete implementation by checking related functions and tests
# Check for the original help function declaration
ast-grep --pattern 'originalHelpFunc := $_'
# Check the upgrade message printing function
ast-grep --pattern 'func printMessageToUpgradeToAtmosLatestRelease($_) {
$$$
}'
# Search for any test files that might cover this functionality
rg -l "customHelpMessageToUpgradeToAtmosLatestRelease" --type go
Length of output: 2202
cmd/cmd_utils.go (2)
466-471: Verify stripANSI handles all ANSI escape codes
The regular expression in stripANSI may not cover all ANSI escape sequences. Please confirm that it effectively removes all types of ANSI codes to ensure clean message output.
432-464: The bordered upgrade message is well-crafted
The implementation for displaying the bordered upgrade notification looks solid and should enhance user awareness.
Listener430
force-pushed
the
DEV-2766
branch
from
eeb83db to
be49831
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cmd/root.go (1)
Line range hint
143-160: HARK! Multiple help function configurations detected!Brave warrior, I notice that
initConfig()also sets a help function. This could potentially conflict with our new implementation inExecute(). Consider:
- Consolidating the help function setup into one location
- Ensuring the styled logo printing is preserved
Consider refactoring to combine these help function configurations:
func initConfig() { styles := boa.DefaultStyles() b := boa.New(boa.WithStyles(styles)) RootCmd.SetUsageFunc(b.UsageFunc) - 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() - }) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
cmd/cmd_utils.go(3 hunks)cmd/root.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/cmd_utils.go
🔇 Additional comments (2)
cmd/root.go (2)
21-22: VICTORY! The variable declaration is now properly implemented!
The addition of originalHelpFunc with its documentation is a worthy improvement to the codebase.
75-81: By Zeus's beard, let's verify this help function integration!
The implementation looks solid, following the proper pattern of preserving the original help functionality. However, we should verify:
- The integration with
customHelpMessageToUpgradeToAtmosLatestRelease - The proper chaining of help functions
Let's inspect the implementation:
✅ Verification successful
The previous results show multiple help function setups, and we need to verify the complete implementation of our custom help function to ensure it properly chains the original help function and adds the upgrade message without recursion. This will help us make a definitive assessment.
By the might of Ares, this help function implementation is battle-ready!
The implementation shows excellent discipline in preventing recursion and maintaining proper command chain:
- The
customHelpMessageToUpgradeToAtmosLatestReleasefunction first callsoriginalHelpFunc(cmd, args), ensuring the original help content is displayed - It then checks for newer versions and only displays the upgrade message when a newer version exists
- The function properly avoids recursion by using the stored
originalHelpFunc
The implementation follows the warrior's path - clean, efficient, and purposeful. Your code stands strong, ready for battle! 🛡️
[category: help_functions, upgrade_notifications]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the help function implementation and its usage
# Check the custom help function implementation
ast-grep --pattern 'func customHelpMessageToUpgradeToAtmosLatestRelease($cmd *cobra.Command, $args []string) {
$$$
}'
# Verify no direct recursion in help function calls
rg -A 5 'SetHelpFunc|HelpFunc'
Length of output: 2145
Script:
#!/bin/bash
# Let's check the complete implementation of customHelpMessageToUpgradeToAtmosLatestRelease
rg -A 15 "func customHelpMessageToUpgradeToAtmosLatestRelease"
# Also check for any other help function overrides in the codebase
ast-grep --pattern 'SetHelpFunc($$$)'
Length of output: 1012
|
@aknysh hi, would appreciate if you take a look. |
|
@Listener430 we should print the upgrade message on any 
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cmd/cmd_utils.go (1)
428-458: Consider extracting color codes into constants.The implementation looks good and follows the suggested lipgloss styling. However, the color codes could be made more maintainable.
Consider extracting the color codes into named constants at the package level:
+const ( + colorSubdued = "8" // For version + colorHighlight = "10" // For latest version and border + colorLink = "14" // For URLs +) func printMessageToUpgradeToAtmosLatestRelease(latestVersion string) { // Define colors - c1 := lipgloss.NewStyle().Foreground(lipgloss.Color("8")) - c2 := lipgloss.NewStyle().Foreground(lipgloss.Color("10")) - c3 := lipgloss.NewStyle().Foreground(lipgloss.Color("14")) + c1 := lipgloss.NewStyle().Foreground(lipgloss.Color(colorSubdued)) + c2 := lipgloss.NewStyle().Foreground(lipgloss.Color(colorHighlight)) + c3 := lipgloss.NewStyle().Foreground(lipgloss.Color(colorLink))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
cmd/cmd_utils.go(3 hunks)
🔇 Additional comments (3)
cmd/cmd_utils.go (3)
11-11: LGTM! Required imports are in place.
The new imports for lipgloss and version packages are correctly added and necessary for the upgrade notification feature.
Also applies to: 20-20
460-471: Verify help function override for subcommands.
Let's ensure the custom help function is properly set for all subcommands.
✅ Verification successful
Help function is properly configured for all commands
The custom help function is correctly set at the root level and will propagate to all subcommands. The implementation in cmd/root.go shows:
- The original help function is properly preserved to avoid recursion
SetHelpFunc(customHelpMessageToUpgradeToAtmosLatestRelease)is called on the root command- No subcommands override this help function
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any commands have their own help function that might override our custom one
# Search for SetHelpFunc or SetHelpCommand in the codebase
rg -l "SetHelpFunc|SetHelpCommand" --type go
# Search for help function declarations
ast-grep --pattern 'func $_(cmd *cobra.Command, args []string) {
$$$
cmd.Help()
$$$
}'
Length of output: 153
Script:
#!/bin/bash
# Let's check the content of root.go to see how the help function is set
cat cmd/root.go
# Also check for any other potential help-related functions in the codebase
rg "func.*help" -i --type go
Length of output: 6978
460-471:
Add nil check for originalHelpFunc.
The function correctly implements the version check, but there's a potential nil pointer dereference.
Add a nil check before calling originalHelpFunc:
func customHelpMessageToUpgradeToAtmosLatestRelease(cmd *cobra.Command, args []string) {
+ if originalHelpFunc == nil {
+ cmd.Help()
+ return
+ }
originalHelpFunc(cmd, args)
// Check for the latest Atmos release on GitHub
latestReleaseTag, err := u.GetLatestGitHubRepoRelease("cloudposse", "atmos")Likely invalid or redundant comment.
|
@osterman thanks, sure, here is the new design with the lipgloss |
|
We've decided that this can be done in a separate PR: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
cmd/cmd_utils.go (1)
434-464: Consider defining color constants and adding error handling.The implementation looks good, but could be enhanced for maintainability and robustness.
Consider these improvements:
+ // Color constants for consistent styling + const ( + versionColor = "8" + newVersionColor = "10" + linkColor = "14" + ) func printMessageToUpgradeToAtmosLatestRelease(latestVersion string) { // Define colors - c1 := lipgloss.NewStyle().Foreground(lipgloss.Color("8")) - c2 := lipgloss.NewStyle().Foreground(lipgloss.Color("10")) - c3 := lipgloss.NewStyle().Foreground(lipgloss.Color("14")) + c1 := lipgloss.NewStyle().Foreground(lipgloss.Color(versionColor)) + c2 := lipgloss.NewStyle().Foreground(lipgloss.Color(newVersionColor)) + c3 := lipgloss.NewStyle().Foreground(lipgloss.Color(linkColor))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
cmd/cmd_utils.go(3 hunks)
🔇 Additional comments (2)
cmd/cmd_utils.go (2)
11-11: LGTM! Clean import additions.
The new imports for lipgloss styling and version comparison are well-organized and necessary for the new functionality.
Also applies to: 20-20
468-468:
Ensure originalHelpFunc is initialized.
The function uses originalHelpFunc which could be nil. This is a critical issue that was previously identified.
Add a guard clause:
func customHelpMessageToUpgradeToAtmosLatestRelease(cmd *cobra.Command, args []string) {
+ if originalHelpFunc == nil {
+ cmd.Help()
+ return
+ }
originalHelpFunc(cmd, args)
|
These changes were released in v1.115.0. |
what
This pull request enhances the atmos CLI by adding a bordered update notification message at the bottom of the help output for all commands, including atmos --help and subcommands like atmos terraform --help. The update notification informs users if a newer version of atmos is available
Befroe
After
final version
why
references
Summary by CodeRabbit
Summary by CodeRabbit