feat(tools/cosmovisor): Add ShowUpgradeInfoCmd #21932
Conversation
Teyz
requested review from
julienrbrt,
facundomedica and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new command, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
|
Teyz
changed the title
Teyz
changed the title
![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)
tools/cosmovisor/cmd/cosmovisor/root.go (1)
22-22: LGTM! Consider grouping related commands.The addition of
NewShowUpgradeInfoCmd()is well-integrated and follows the existing naming conventions. It aligns with the PR's objective of introducing new features to the Cosmos SDK.For better code organization, consider grouping related commands together. For instance, you might want to place
NewShowUpgradeInfoCmd()next toNewAddUpgradeCmd()since they both deal with upgrades. This could improve readability and maintainability of the code.rootCmd.AddCommand( NewIntCmd(), runCmd, configCmd, NewVersionCmd(), NewAddUpgradeCmd(), + NewShowUpgradeInfoCmd(), )tools/cosmovisor/cmd/cosmovisor/show_upgrade.go (1)
10-20: LGTM: Well-structured Cobra command setup.The
NewShowUpgradeInfoCmdfunction is well-implemented and follows Golang and Cobra best practices. The command configuration is correct, including the use ofSilenceUsageandArgs.Consider enhancing the
Shortdescription to provide more context:- Short: "Show upgrade-info.json into stdout.", + Short: "Display the contents of upgrade-info.json to stdout.",
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- tools/cosmovisor/cmd/cosmovisor/root.go (1 hunks)
- tools/cosmovisor/cmd/cosmovisor/show_upgrade.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tools/cosmovisor/cmd/cosmovisor/root.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/cmd/cosmovisor/show_upgrade.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
tools/cosmovisor/cmd/cosmovisor/show_upgrade.go (1)
3-8: LGTM: Import statements are correct and well-organized.The import statements are properly structured with standard library imports followed by third-party imports. All imported packages are used in the code.
| func showUpgradeInfoCmd(cmd *cobra.Command, args []string) error { | ||
| if len(args) != 1 { | ||
| return fmt.Errorf("expected exactly one argument") | ||
| } | ||
|
|
||
| data, err := os.ReadFile(args[0]) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read upgrade-info.json: %w", err) | ||
| } | ||
|
|
||
| fmt.Println(string(data)) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suggestions for improving error handling and output method.
The showUpgradeInfoCmd function is generally well-implemented, but there are a few areas for improvement:
- The argument check is redundant due to
cobra.ExactArgs(1)in the command setup. - Error handling could be more specific, especially for the "file not found" case.
- Directly printing to stdout might limit testability and reusability.
Consider applying these changes:
func showUpgradeInfoCmd(cmd *cobra.Command, args []string) error {
- if len(args) != 1 {
- return fmt.Errorf("expected exactly one argument")
- }
-
data, err := os.ReadFile(args[0])
if err != nil {
+ if os.IsNotExist(err) {
+ return fmt.Errorf("upgrade-info.json not found at %s: %w", args[0], err)
+ }
return fmt.Errorf("failed to read upgrade-info.json: %w", err)
}
- fmt.Println(string(data))
+ _, err = fmt.Fprintln(cmd.OutOrStdout(), string(data))
+ if err != nil {
+ return fmt.Errorf("failed to write output: %w", err)
+ }
return nil
}These changes improve error specificity, remove redundant checks, and use cmd.OutOrStdout() for better testability.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func showUpgradeInfoCmd(cmd *cobra.Command, args []string) error { | |
| if len(args) != 1 { | |
| return fmt.Errorf("expected exactly one argument") | |
| } | |
| data, err := os.ReadFile(args[0]) | |
| if err != nil { | |
| return fmt.Errorf("failed to read upgrade-info.json: %w", err) | |
| } | |
| fmt.Println(string(data)) | |
| return nil | |
| } | |
| func showUpgradeInfoCmd(cmd *cobra.Command, args []string) error { | |
| data, err := os.ReadFile(args[0]) | |
| if err != nil { | |
| if os.IsNotExist(err) { | |
| return fmt.Errorf("upgrade-info.json not found at %s: %w", args[0], err) | |
| } | |
| return fmt.Errorf("failed to read upgrade-info.json: %w", err) | |
| } | |
| _, err = fmt.Fprintln(cmd.OutOrStdout(), string(data)) | |
| if err != nil { | |
| return fmt.Errorf("failed to write output: %w", err) | |
| } | |
| return nil | |
| } |
There was a problem hiding this comment.
The bot has great suggestion, could you implement them?
There was a problem hiding this comment.
@julienrbrt, certainly! I've implemented the bot's suggestions.
Here's the updated show_upgrade.go file with the improvements applied:
package main
import (
"fmt"
"os"
"github.com/spf13/cobra"
)
func NewShowUpgradeInfoCmd() *cobra.Command {
showUpgradeInfo := &cobra.Command{
Use: "show-upgrade-info <path to executable>",
Short: "Show upgrade-info.json into stdout.",
SilenceUsage: true,
Args: cobra.ExactArgs(1),
RunE: showUpgradeInfoCmd,
}
return showUpgradeInfo
}
func showUpgradeInfoCmd(cmd *cobra.Command, args []string) error {
data, err := os.ReadFile(args[0])
if err != nil {
if os.IsNotExist(err) {
return fmt.Errorf("upgrade-info.json not found at %s: %w", args[0], err)
}
return fmt.Errorf("failed to read upgrade-info.json: %w", err)
}
_, err = fmt.Fprintln(cmd.OutOrStdout(), string(data))
if err != nil {
return fmt.Errorf("failed to write output: %w", err)
}
return nil
}These changes improve error specificity, remove the redundant argument check, and use cmd.OutOrStdout() for better testability, as suggested.
🐰😊
julienrbrt
changed the title
| func showUpgradeInfoCmd(cmd *cobra.Command, args []string) error { | ||
| if len(args) != 1 { | ||
| return fmt.Errorf("expected exactly one argument") | ||
| } | ||
|
|
||
| data, err := os.ReadFile(args[0]) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read upgrade-info.json: %w", err) | ||
| } | ||
|
|
||
| fmt.Println(string(data)) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The bot has great suggestion, could you implement them?
| return fmt.Errorf("failed to read upgrade-info.json: %w", err) | ||
| } | ||
|
|
||
| fmt.Println(string(data)) |
There was a problem hiding this comment.
Let's use cmd.Println(string(data)) instead
|
|
||
| func NewShowUpgradeInfoCmd() *cobra.Command { | ||
| showUpgradeInfo := &cobra.Command{ | ||
| Use: "show-upgrade-info <path to executable>", |
There was a problem hiding this comment.
The goal of the issue wasn't to pass an upgrade-info location, but to get it from the cosmovisor node config. We have the root of the node, so we can know the path of the executable. Taking an argument doesn't close the issue.
| showUpgradeInfo := &cobra.Command{ | ||
| Use: "show-upgrade-info", | ||
| Short: "Show upgrade-info.json into stdout.", | ||
| SilenceUsage: true, |
There was a problem hiding this comment.
Let's not silence usage.
And let's add cobra.NoArgs
|
The linter complains, you need to run |
|
could you add a changelog under tools/cosmovisor/changelog.md |
tools/cosmovisor/CHANGELOG.md
Outdated
| @@ -67,6 +67,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
| * [#16550](https://github.com/cosmos/cosmos-sdk/pull/16550) Add COSMOVISOR_CUSTOM_PREUPGRADE to cosmovisor to execute custom pre-upgrade scripts (separate from daemon `pre-upgrade` command). | |||
| * [#16963](https://github.com/cosmos/cosmos-sdk/pull/16963) Add DAEMON_SHUTDOWN_GRACE to send interrupt and wait before sending kill. | |||
| * [#15361](https://github.com/cosmos/cosmos-sdk/pull/15361) Add `cosmovisor config` command to display the configuration used by cosmovisor. | |||
| * [#21932](https://github.com/cosmos/cosmos-sdk/pull/21932) Add `cosmovisor show-upgrade-info` command to display the upgrade-info.json into stdout. | |||
There was a problem hiding this comment.
the changelog should be under unreleased features.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tools/cosmovisor/CHANGELOG.md (1)
70-71: LGTM with a minor suggestion.The new changelog entry is correctly placed and formatted. It accurately describes the new feature added in PR #21932. However, to improve clarity, consider rewording slightly:
Consider this minor rewording for improved clarity:
-* [#21932](https://github.com/cosmos/cosmos-sdk/pull/21932) Add `cosmovisor show-upgrade-info` command to display the upgrade-info.json into stdout. +* [#21932](https://github.com/cosmos/cosmos-sdk/pull/21932) Add `cosmovisor show-upgrade-info` command to display the contents of upgrade-info.json to stdout.This change more explicitly states that the command displays the contents of the file, which aligns better with the PR objectives.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- tools/cosmovisor/CHANGELOG.md (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tools/cosmovisor/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
|
there are still build issues here, could you fix? |
|
@julienrbrt I would like to fix but I still have errors when I want to run : go mod tidy and make lint ... |
Maybe you can try to use GitHub codespaces? It should work from there. |
|
Should be good @julienrbrt |
Teyz
changed the title
Description
Closes: #21726
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Summary by CodeRabbit
New Features
show-upgrade-infoto display upgrade information from a specified JSON file.Bug Fixes