feat(tools/cosmovisor): cosmovisor batch upgrades #21790
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
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
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range and nitpick comments (7)
tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (3)
16-16: Correct grammatical error in command's short descriptionThe
Shortdescription should be "Add APP upgrade binaries to cosmovisor" instead of "Add APP upgrades binary to cosmovisor".Apply this diff to fix the grammatical error:
- Short: "Add APP upgrades binary to cosmovisor", + Short: "Add APP upgrade binaries to cosmovisor",
23-24: Use GoDoc style comments for exported functionsThe exported function
AddBatchUpgradeshould have a comment that starts with the function name to comply with GoDoc conventions.Apply this diff to update the comment:
- // AddBatchUpgrade takes in multiple specified upgrades and creates a single - // batch upgrade file out of them + // AddBatchUpgrade takes in multiple specified upgrades and creates a single + // batch upgrade file out of them.Ensure the comment starts with the function name and ends with a period.
13-13: Add GoDoc comment for exported functionNewBatchAddUpgradeCmdThe exported function
NewBatchAddUpgradeCmdlacks a GoDoc comment. Adding a comment improves documentation and code readability.Apply this diff to add the comment:
+ // NewBatchAddUpgradeCmd creates a new Cobra command for batch adding upgrades. func NewBatchAddUpgradeCmd() *cobra.Command {tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (2)
Line range hint
68-70: Avoid using panic; return an error insteadUsing
panicfor expected errors is discouraged as it can cause the application to crash unexpectedly. Instead, return the error to allow it to be handled gracefully by the caller. Also, consider providing a more descriptive error message.Apply this diff to fix the issue:
if err := plan.ValidateBasic(); err != nil { - panic(fmt.Errorf("something is wrong with cosmovisor: %w", err)) + return fmt.Errorf("invalid upgrade plan: %w", err) }
88-123: Unexport functions that are not intended to be publicThe functions
GetConfigandAddUpgradeCmdare currently exported due to their capitalized names. If these functions are only used within this package, consider renaming them togetConfigandaddUpgradeCmdto reflect that they are unexported, following Go conventions and the Uber Go Style Guide.Apply this diff to make the changes:
// GetConfig returns a Config using passed-in flag -func GetConfig(cmd *cobra.Command) (*cosmovisor.Config, error) { +func getConfig(cmd *cobra.Command) (*cosmovisor.Config, error) { // ... // AddUpgradeCmd parses input flags and adds upgrade info to manifest -func AddUpgradeCmd(cmd *cobra.Command, args []string) error { +func addUpgradeCmd(cmd *cobra.Command, args []string) error { // ... func NewAddUpgradeCmd() *cobra.Command { addUpgrade := &cobra.Command{ // ... - RunE: AddUpgradeCmd, + RunE: addUpgradeCmd, } // ...tools/cosmovisor/process.go (1)
47-62: Use descriptive variable names for better readabilityThe variable
uInfosis not descriptive. Consider renaming it toupgradePlansorplansto improve readability and adhere to the style guide recommendations on naming.Apply this diff to rename
uInfostoupgradePlans:-var uInfos []upgradetypes.Plan +var upgradePlans []upgradetypes.PlanAnd update all occurrences of
uInfoswithin the function accordingly.tools/cosmovisor/args.go (1)
112-112: Adjust the function comment for GoDoc style compliancePer GoDoc conventions, the comment should start with the function name and be a complete sentence that describes its behavior.
Update the comment as follows:
-// UpgradeInfoBatchFilePath is the same as UpgradeInfoFilePath but with a batch suffix. +// UpgradeInfoBatchFilePath returns the same path as UpgradeInfoFilePath but with a ".batch" suffix.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 81ec7ea and 3fe1cff58bad115be9558e366f2f9f29d408c89f.
Files selected for processing (7)
- tools/cosmovisor/CHANGELOG.md (1 hunks)
- tools/cosmovisor/args.go (1 hunks)
- tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (4 hunks)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1 hunks)
- tools/cosmovisor/cmd/cosmovisor/root.go (1 hunks)
- tools/cosmovisor/go.mod (1 hunks)
- tools/cosmovisor/process.go (4 hunks)
Additional context used
Path-based instructions (6)
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/batch_upgrade.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"tools/cosmovisor/process.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/args.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Markdownlint
tools/cosmovisor/CHANGELOG.md
39-39: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
40-40: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
GitHub Check: CodeQL
tools/cosmovisor/process.go
[notice] 201-201: Spawning a Go routine
Spawning a Go routine may be a possible source of non-determinism
Additional comments not posted (3)
tools/cosmovisor/cmd/cosmovisor/root.go (1)
22-22: Addition ofNewBatchAddUpgradeCmd()follows conventions and is well-integrated.The new command
NewBatchAddUpgradeCmd()is consistent with the existing command structure. It adheres to the naming conventions outlined in the Uber Golang style guide, using theNewprefix for constructor functions that return a command.tools/cosmovisor/go.mod (1)
8-9: Verify the necessity of adding dependencies as direct requirementsPlease confirm that
github.com/cometbft/cometbft v0.38.9andgithub.zerozr99.workers.dev/fsnotify/fsnotify v1.7.0are directly imported in thecosmovisorcodebase. If these packages are only used indirectly through other dependencies, they may not need to be listed as direct requirements in thego.modfile. Keeping only necessary direct dependencies helps maintain a cleaner dependency graph and reduces potential conflicts.Run the following script to check for direct imports of these packages:
tools/cosmovisor/args.go (1)
113-115: LGTM!The new method correctly adds the batch upgrade info file path functionality and follows existing code patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
tools/cosmovisor/process.go (1)
Line range hint
1-600: Minor deviations from Uber Go Style Guide.The code follows the Uber Go Style Guide for the most part. There are a few minor deviations:
- Some lines are longer than 120 characters. Consider breaking them into multiple lines for better readability.
- Some error messages don't start with lowercase. Error messages should start with lowercase for consistency.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 3fe1cff58bad115be9558e366f2f9f29d408c89f and 7e1bf65048d5f7fb2dac8ac7a8bc849299d03b43.
Files selected for processing (4)
- tools/cosmovisor/CHANGELOG.md (1 hunks)
- tools/cosmovisor/args.go (5 hunks)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1 hunks)
- tools/cosmovisor/process.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- tools/cosmovisor/CHANGELOG.md
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go
Additional context used
Path-based instructions (2)
tools/cosmovisor/args.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/process.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (8)
tools/cosmovisor/process.go (4)
47-63: LGTM!The
loadBatchUpgradeFilefunction correctly loads and sorts the batch upgrade plans. The error handling looks good.
68-177: LGTM!The
BatchWatcherfunction correctly implements the batch upgrade process. It handles new blocks, updates theupgrade-info.jsonfile at the appropriate heights, and reloads the upgrade plans if the batch file is modified. The error handling and logging look good.
201-202: LGTM!The changes to the
Runfunction to start theBatchWatcherin a separate goroutine look good. Using a context to manage the lifecycle of theBatchWatcheris a nice touch.Also applies to: 214-215
207-207: LGTM!Calling
cancel()when a signal is received is the correct way to gracefully exit theBatchWatcher.tools/cosmovisor/args.go (4)
41-41: LGTM!The new constant
EnvCometBftRpcEndpointis defined correctly and follows the existing naming convention.
72-72: Looks good!The new
CometBftRpcEndpointfield is added to theConfigstruct correctly, with appropriate tags and a reasonable default value.
114-117: LGTM!The new
UpgradeInfoBatchFilePathfunction is implemented correctly and provides a clear way to derive the batch upgrade info file path.
217-221: Looks good!The
GetConfigFromEnvfunction andDetailStringmethod are updated appropriately to handle the newCometBftRpcEndpointfield.Also applies to: 559-559
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
tools/cosmovisor/process.go (1)
68-177: TheBatchUpgradeWatcherfunction logic is correct, but consider refactoring for better readability and maintainability.The
BatchUpgradeWatcherfunction is implemented correctly and handles the core logic of watching for upgrades and replacing the upgrade-info files. However, the function is quite long and could be refactored into smaller functions for better readability and maintainability. For example:
- Extract the logic for replacing the upgrade-info and batch files into a separate function.
- Consider propagating errors up to the caller instead of logging and returning.
- Add a way to gracefully stop the watcher.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 2fb3b68fc5beea40a0195c4bc818c37a00a03e3e and 66c7be2760d1255f846fa3efe6137c7d580aad71.
Files selected for processing (4)
- tools/cosmovisor/args_test.go (1 hunks)
- tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (4 hunks)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1 hunks)
- tools/cosmovisor/process.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go
Additional context used
Path-based instructions (3)
tools/cosmovisor/args_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/process.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (11)
tools/cosmovisor/cmd/cosmovisor/add_upgrade.go (6)
22-22: LGTM!Setting the
RunEfield toAddUpgradeCmdis the correct way to specify the handler function for theadd-upgradecommand.
31-32: Refactoring the function signature is a good improvement!Accepting parameters directly instead of relying on command-line arguments enhances the modularity and reusability of the
addUpgradefunction. This change aligns with best practices for writing clean and maintainable code.
36-36: LGTM!Converting the upgrade name to lowercase, when not disabled by configuration, is a good practice to ensure consistency in naming conventions across the codebase.
Line range hint
66-82: The code segment handles upgrades at a specific height correctly.The logic for creating an upgrade plan, validating it, and saving it to the
upgrade-info.jsonfile is implemented correctly. The code segment aligns with the best practices mentioned in the past review comment about setting appropriate file permissions for non-executable files.
88-100: Extracting the configuration retrieval logic is a good practice.Moving the configuration retrieval logic into the
getConfigFromCmdfunction improves code organization and reusability. The function handles errors appropriately and returns them to the caller, which aligns with best practices for error handling in Go.
102-122: TheAddUpgradeCmdfunction is implemented correctly.The function follows the expected logic for a command handler. It parses the input flags, retrieves the configuration, and calls the refactored
addUpgradefunction with the appropriate parameters. The error handling is done appropriately, and the code segment aligns with the changes made to theaddUpgradefunction signature.tools/cosmovisor/process.go (3)
47-63: LGTM!The
loadBatchUpgradeFilefunction is implemented correctly:
- It handles the file not found error gracefully.
- It uses the
Configfor the file path to allow configurability.- Error messages include the file path for easier debugging.
- It sorts the slice of upgrade plans by height to ensure correct order of processing.
95-101: Good use of polling to wait for the chain process to be ready.Polling the RPC endpoint to wait for the chain process to be ready is a good way to ensure the watcher starts at the correct time and doesn't miss any blocks.
214-214: Starting theBatchUpgradeWatcherin a separate goroutine is a good way to ensure it runs concurrently with the main process.The changes to the
Runfunction to start theBatchUpgradeWatcherin a separate goroutine are implemented correctly:
- It allows the watcher to run concurrently with the main process.
- It passes a context to the watcher, which allows it to be cancelled when the
Runfunction exits.- It starts the watcher before waiting for an upgrade, which ensures it doesn't miss any upgrades.
tools/cosmovisor/args_test.go (2)
480-480: Approve the addition of theCometBftRpcEndpointparameter.The addition of the
CometBftRpcEndpointparameter to theConfigstruct enhances the configurability of the application and aligns with the goal of improving integration with CometBFT.
480-480: Verify the handling of theCometBftRpcEndpointparameter.Please ensure that the
CometBftRpcEndpointparameter is properly validated and handled throughout the codebase to maintain the integrity of the application.Verification successful
CometBftRpcEndpointparameter is handled consistently, but could benefit from additional validationThe
CometBftRpcEndpointparameter is properly used throughout the codebase with a default value and environment variable support. However, consider implementing these improvements:
- Add explicit URL format validation when setting the endpoint.
- Enhance error handling when using the endpoint in HTTP requests and client creation.
- Include documentation about security considerations for this parameter.
These suggestions aim to improve robustness and security, but the current implementation is functional.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the `CometBftRpcEndpoint` parameter is properly validated and handled. # Test: Search for occurrences of `CometBftRpcEndpoint`. # Expect: Proper validation and handling of the parameter. rg --type go -A 5 $'CometBftRpcEndpoint'Length of output: 2643
|
Hi @julienrbrt I've finished the implementation of this feature, would love a code review. |
|
ping @facundomedica, this PR is ready for review |
|
I'll review today! Thank you |
|
ping @julienrbrt lmk if you have any questions about my code |
julienrbrt
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
nice work! I do think we need to make it more hacky and not use cometbft websocket.
We are trying to abstract away from the consensus.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1)
14-22: Consider simplifying the command usage message.While the command structure is correct, the
Usefield is still quite lengthy and might be confusing for users. Consider simplifying it or moving detailed usage information to theLongdescription field for better clarity.Good job on addressing the minimum arguments requirement with
cobra.MinimumNArgs(1).tools/cosmovisor/args.go (2)
72-72: LGTM: New Config field added correctly.The
CosmosGrpcEndpointfield is added with appropriate tags and a sensible default value. Consider adding a brief comment explaining the purpose of this field for better documentation.- CosmosGrpcEndpoint string `toml:"cosmos_grpc_endpoint" mapstructure:"cosmos_grpc_endpoint" default:"localhost:9090"` + CosmosGrpcEndpoint string `toml:"cosmos_grpc_endpoint" mapstructure:"cosmos_grpc_endpoint" default:"localhost:9090"` // gRPC endpoint for Cosmos SDK
114-117: LGTM: New method for batch upgrade file path added.The
UpgradeInfoBatchFilePathmethod is concise and follows the single responsibility principle. Consider adding a brief comment explaining the purpose of this method and its relation to the batch upgrade feature.-// UpgradeInfoBatchFilePath is the same as UpgradeInfoFilePath but with a batch suffix. +// UpgradeInfoBatchFilePath returns the path for the batch upgrade info file. +// It's the same as UpgradeInfoFilePath but with a ".batch" suffix, used for managing multiple upgrades. func (cfg *Config) UpgradeInfoBatchFilePath() string { return cfg.UpgradeInfoFilePath() + ".batch" }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between 66c7be2760d1255f846fa3efe6137c7d580aad71 and ba5462f61cfbedc74b19aa25c0f55629565af45f.
📒 Files selected for processing (5)
- tools/cosmovisor/args.go (5 hunks)
- tools/cosmovisor/args_test.go (1 hunks)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1 hunks)
- tools/cosmovisor/go.mod (1 hunks)
- tools/cosmovisor/process.go (4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
tools/cosmovisor/args.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/args_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tools/cosmovisor/process.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (11)
tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (2)
1-12: LGTM: Package and imports are appropriate.The package declaration and import statements are correct and relevant for the functionality implemented in this file.
24-42: LGTM: Argument processing and validation look good.The function correctly processes and validates the input arguments. Good job on sanitizing
upgradeNameusingfilepath.Base()to prevent potential path traversal vulnerabilities.tools/cosmovisor/go.mod (4)
8-8: Approval: Direct dependency on cosmos-sdk addedThe addition of
github.com/cosmos/cosmos-sdk v0.50.7as a direct dependency is appropriate. This change indicates that the Cosmovisor tool is now explicitly using features from the Cosmos SDK, which aligns with the PR objectives of enhancing upgrade management capabilities.
8-15: Summary: Dependency updates align with PR objectivesThe changes to the
go.modfile, particularly the addition of direct dependencies oncosmos-sdk,fsnotify, andgrpc, align well with the PR objectives of enhancing the Cosmovisor tool for managing upgrades. These dependencies suggest improvements in functionality related to:
- Direct interaction with the Cosmos SDK
- File system monitoring (possibly for upgrade-related files)
- Enhanced gRPC communication
These changes are likely to support the new batch upgrade feature mentioned in the PR objectives, potentially improving the efficiency and reliability of the upgrade process for node operators.
15-15: Approval: Direct dependency on gRPC addedThe addition of
google.golang.org/grpc v1.66.2as a direct dependency is appropriate. This change suggests that the Cosmovisor tool may be implementing or enhancing gRPC-based communication, possibly for interacting with the Cosmos SDK or other components during the upgrade process.Please verify the usage of gRPC in the codebase:
#!/bin/bash # Search for gRPC usage in the codebase rg --type go 'grpc\.' tools/cosmovisor
9-9: Approval: Direct dependency on fsnotify addedThe addition of
github.com/fsnotify/fsnotify v1.7.0as a direct dependency is appropriate. This change suggests that the Cosmovisor tool may be implementing file watching functionality, possibly for monitoring upgrade-related files.Please verify the usage of this package in the codebase:
✅ Verification successful
Action Required: Re-verify fsnotify Usage with Corrected Script
Please run the following shell script to accurately search for
fsnotify.usage in thetools/cosmovisordirectory:
Verification Successful: fsnotify Usage Confirmed
The
github.com/fsnotify/fsnotify v1.7.0dependency is actively used intools/cosmovisor/process.go:
watcher, err := fsnotify.NewWatcher()if event.Op&(fsnotify.Write|fsnotify.Create) != 0 {This confirms that adding
fsnotifyas a direct dependency is appropriate.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for fsnotify usage in the codebase rg --type go 'fsnotify\.' tools/cosmovisorLength of output: 324
Script:
# #!/bin/bash # Search for fsnotify usage in the codebase rg --type go 'fsnotify\.' tools/cosmovisorLength of output: 194
tools/cosmovisor/args.go (3)
41-41: LGTM: New environment variable constant added correctly.The new
EnvCosmosGrpcEndpointconstant is added consistently with the existing naming convention and placed appropriately at the end of the constant block.
563-563: LGTM: CosmosGrpcEndpoint added to DetailString method.The addition of
CosmosGrpcEndpointto theconfigEntriesslice ensures that this new configuration is included in the detailed string representation of the Config. This change is consistent with how other fields are handled in this method.
Line range hint
1-638: Overall assessment: New CosmosGrpcEndpoint feature well-implemented.The changes introduce a new
CosmosGrpcEndpointconfiguration option consistently throughout the file. The implementation follows existing patterns and conventions in the codebase. Minor suggestions have been made for improved documentation and removal of redundant code. These changes appear to support the batch upgrade feature mentioned in the PR objectives without introducing any significant issues.tools/cosmovisor/args_test.go (2)
Line range hint
1-1000: Verify impact on other filesThe addition of the
CosmosGrpcEndpointfield in theConfigstruct might have implications for other parts of the codebase. It's important to ensure that any code that uses theConfigstruct is updated to handle this new field appropriately.To check for potential impacts, you can run the following script:
#!/bin/bash # Description: Search for uses of the Config struct in other files # Test: Look for files that import and use the Config struct rg --type go -A 5 'import \(.*"cosmossdk.io/tools/cosmovisor".*\)' | rg 'cosmovisor\.Config' # If any results are found, these files might need to be updated to handle the new CosmosGrpcEndpoint field
Line range hint
483-1000: Update test cases to include CosmosGrpcEndpointWith the addition of the
CosmosGrpcEndpointfield in theConfigstruct, the test cases inTestGetConfigFromEnvshould be updated to include this new field. This ensures that the new configuration option is properly tested.To verify if the test cases need updating, you can run the following script:
julienrbrt
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
amazing work! thank you, just missing some handling when passing a file I think
| if err := json.Unmarshal(b, &batch); err != nil { | ||
| return fmt.Errorf("error unmarshalling upgrade file %s: %w", upgradeFile, err) | ||
| } | ||
| return copyFile(upgradeFile, cfg.UpgradeInfoBatchFilePath()) |
There was a problem hiding this comment.
this looks incomplete. Where is batch used? And why aren't we calling add upgrade there as well?
There was a problem hiding this comment.
This is no different than processUpgradeList where we end with writing out the batch file to the home directory (
cosmovisor run.
There was a problem hiding this comment.
processUpgradeList calls addUpgrade, which place the binaries at the right place.
This doesn't.
There was a problem hiding this comment.
You're right, my bad. upgradetypes.Plan doesn't really have field we can use for the binary path, what do you think of changing the file format for --upgrade-file to be a CSV with rows name,path/to/binary,height? I'd imagine it'd be easier for users to make than a JSON file anyway.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (2)
71-121: Use a constant for file permissionsThe function correctly uses
0o600permissions when writing the batch upgrade file, which is good for security. However, it's a good practice to define such values as constants for better maintainability and readability.Consider adding a constant at the package level:
const batchUpgradeFilePerms = 0o600Then use this constant when writing the file:
err = os.WriteFile(cfg.UpgradeInfoBatchFilePath(), batchData, batchUpgradeFilePerms)This makes the code more self-documenting and easier to maintain if the permissions need to be changed in the future.
123-142: Add a comment explaining the CSV formatThe function correctly processes a CSV batch upgrade file, but it would be helpful to add a comment explaining the expected format of the CSV file for better documentation.
Consider adding a comment at the beginning of the function:
// processUpgradeFile takes in a CSV batch upgrade file, parses it and calls processUpgradeList // The CSV file should be headerless with each row containing three fields: // 1. upgrade-name // 2. path-to-exec // 3. upgrade-height func processUpgradeFile(cfg *cosmovisor.Config, upgradeFile string) error { // ... existing code ... }This comment will help users understand the expected format of the CSV file without having to look at the command description.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (4)
17-48: LGTM! Consider adding validation for required flags.The
NewBatchAddUpgradeCmdfunction is well-implemented, following Cobra best practices. The command description, usage, and example are clear and informative. The mutual exclusivity of flags is correctly enforced.Consider adding a check to ensure that at least one of the flags (--upgrade-file or --upgrade-list) is provided. You can use
cmd.MarkFlagRequired("upgrade-file")and handle the case in theRunEfunction.
72-122: LGTM! Consider optimizing file operations and improving error messages.The
processUpgradeListfunction effectively handles the core logic of batch upgrades. The implementation is solid, with good error handling and secure practices like usingfilepath.BaseforupgradeName.Consider these minor improvements:
- Use
os.Createwithos.O_WRONLY|os.O_CREATE|os.O_TRUNCflags instead ofos.WriteFilefor better control over file permissions.- Enhance error messages to include more context, especially for file operations.
Example for point 1:
file, err := os.OpenFile(cfg.UpgradeInfoBatchFilePath(), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) if err != nil { return fmt.Errorf("error creating batch file: %w", err) } defer file.Close() _, err = file.Write(batchData) if err != nil { return fmt.Errorf("error writing to batch file: %w", err) }These changes will provide more control over file operations and improve error reporting.
124-143: LGTM! Consider enhancing error handling for CSV parsing.The
processUpgradeFilefunction effectively handles CSV file parsing and delegates the processing toprocessUpgradeList. The implementation is solid, with good use of the csv package and proper error handling.Consider enhancing the error handling for CSV parsing to provide more context about invalid records:
records, err := r.ReadAll() if err != nil { - return fmt.Errorf("error parsing upgrade CSV file %s: %w", upgradeFile, err) + if perr, ok := err.(*csv.ParseError); ok { + return fmt.Errorf("error parsing upgrade CSV file %s at line %d, column %d: %w", upgradeFile, perr.Line, perr.Column, perr.Err) + } + return fmt.Errorf("error parsing upgrade CSV file %s: %w", upgradeFile, err) }This change will provide more detailed information about parsing errors, making it easier for users to identify and correct issues in their CSV files.
1-143: Add unit tests for the new batch upgrade functionalityThe implementation of batch upgrade functionality is well-structured and meets the objectives outlined in the PR summary. However, to ensure reliability and prevent regressions, it's crucial to add comprehensive unit tests.
Consider adding unit tests for the following scenarios:
- Parsing and validating upgrade lists
- Processing CSV upgrade files
- Creating and combining individual upgrade files
- Error handling for various input scenarios
Would you like assistance in generating a basic structure for these unit tests?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tools/cosmovisor/cmd/cosmovisor/batch_upgrade.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
|
Could you fix the failing unit tests: https://github.com/cosmos/cosmos-sdk/actions/runs/11145765964/job/30976132402?pr=21790 ? |
ah it was due to the new grpc config; should be fixed now |
|
hey @julienrbrt do we need an additional review before this can be merged? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- tools/cosmovisor/args_test.go (1 hunks)
- tools/cosmovisor/go.mod (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/cosmovisor/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
tools/cosmovisor/args_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (1)
tools/cosmovisor/args_test.go (1)
480-480: Update test cases and usage for new GRPCAddress fieldThe addition of the
GRPCAddressfield to theConfigstruct is a significant change that likely requires updates to existing test cases and possibly other parts of the code that use this struct.Please ensure that:
- Existing test cases are updated to include the new
GRPCAddressfield.- New test cases are added to specifically test the behavior related to this field.
- Any other parts of the codebase that create or use
Configinstances are updated accordingly.To help verify these changes, you can run the following script:
This will help identify areas of the code that might need updates due to the new field.
Yes, someone else from the team will review and then we can merge. |
hi @julienrbrt do you think we can get a second review from the team soon? |
|
Yes. No worries it will get reviewed. |
|
Could you fix the linting and the cosmovisor tests? |
should be fixed now |
3 participants
Description
Closes: #20630
This PR adds a
add-batch-upgradetocosmovisorthat allows to add multiple named upgrades at specific heights.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
New Features
add-batch-upgradecommand for efficient batch upgrades.prepare-upgradecommand for streamlined upgrade preparation.cosmovisor show-upgrade-infocommand to display upgrade details.stdinfor enhanced input handling.Improvements
initcommand to write configuration to a specified default path and accept a configuration flag.configcommand to display configuration from a provided config file or environment variables.Bug Fixes
add-upgradecommand.Dependency Updates