test(systemtests): fix export v2 #22799
Conversation
julienrbrt
added
the
backport/v0.52.x
julienrbrt
requested review from
kocubinski,
hieuvubk,
a team and
cool-develope
as code owners
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the configuration handling within the Cosmos SDK. The Changes
Possibly related 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
|
github-actions
bot
added
C:Store
C:x/genutil
![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
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/systemtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
server/v2/store/server.go(1 hunks)store/v2/root/builder.go(1 hunks)tests/systemtests/go.mod(1 hunks)x/genutil/v2/cli/export.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/systemtests/go.mod
🧰 Additional context used
📓 Path-based instructions (3)
store/v2/root/builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/store/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/genutil/v2/cli/export.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
x/genutil/v2/cli/export.go
114-114: Error return value of (*github.com/spf13/pflag.FlagSet).MarkDeprecated is not checked
(errcheck)
🪛 GitHub Check: golangci-lint
x/genutil/v2/cli/export.go
[failure] 114-114:
Error return value of (*github.com/spf13/pflag.FlagSet).MarkDeprecated is not checked (errcheck)
🔇 Additional comments (3)
server/v2/store/server.go (1)
81-81: LGTM: Simplified configuration initialization
The change to use root.DefaultConfig() directly improves code clarity while maintaining the same functionality.
store/v2/root/builder.go (1)
58-62: LGTM: Added essential validation for AppDBBackend
Good addition of validation to ensure the application database backend is configured before proceeding with store creation.
x/genutil/v2/cli/export.go (1)
20-20: LGTM: Clean implementation of zero-height export flag
The implementation provides a clear way to handle zero-height exports while maintaining backward compatibility.
Also applies to: 60-63
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/genutil/v2/cli/export.go (1)
60-63: Add a comment explaining the flag's purposeWhile the flag is marked as deprecated in favor of
--height=0, it would be helpful to add a comment explaining why this alternative flag exists (e.g., if it's for backward compatibility with existing scripts).+// forZeroHeight provides an alternative way to export state from height 0 +// This flag is deprecated in favor of --height=0 but maintained for backward compatibility forZeroHeight, _ := cmd.Flags().GetBool(flagForZeroHeight)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
store/db/prefixdb_test.go(1 hunks)tests/integration/v2/distribution/fixture_test.go(0 hunks)x/genutil/v2/cli/export.go(3 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/v2/distribution/fixture_test.go
✅ Files skipped from review due to trivial changes (1)
- store/db/prefixdb_test.go
🧰 Additional context used
📓 Path-based instructions (1)
x/genutil/v2/cli/export.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
x/genutil/v2/cli/export.go (2)
20-20: LGTM!
The constant declaration follows Go naming conventions and is appropriately grouped with related flag constants.
113-114:
Add error handling for MarkDeprecated
The error returned by MarkDeprecated should be checked to ensure the deprecation notice is properly set.
Apply this fix:
-_ = cmd.Flags().MarkDeprecated(flagForZeroHeight, "use --height=0 instead")
+if err := cmd.Flags().MarkDeprecated(flagForZeroHeight, "use --height=0 instead"); err != nil {
+ panic(fmt.Sprintf("failed to mark flag as deprecated: %v", err))
+}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
x/genutil/v2/cli/export.go (2)
60-68: Add error handling for flag valuesThe flag values are retrieved without error checking. While these operations typically don't fail, it's good practice to handle potential errors.
- forZeroHeight, _ := cmd.Flags().GetBool(flagForZeroHeight) - jailAllowedAddrs, _ := cmd.Flags().GetStringSlice(flagJailAllowedAddrs) + forZeroHeight, err := cmd.Flags().GetBool(flagForZeroHeight) + if err != nil { + return fmt.Errorf("failed to get for-zero-height flag: %w", err) + } + jailAllowedAddrs, err := cmd.Flags().GetStringSlice(flagJailAllowedAddrs) + if err != nil { + return fmt.Errorf("failed to get jail-allowed-addrs flag: %w", err) + }
110-110: Fix typo in flag descriptionThere's a typo in the flag description: "preproccessing" should be "preprocessing".
- cmd.Flags().Bool(flagForZeroHeight, false, "Export state to start at height zero (perform preproccessing)") + cmd.Flags().Bool(flagForZeroHeight, false, "Export state to start at height zero (perform preprocessing)")
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
simapp/v2/app_test.go(1 hunks)simapp/v2/export.go(2 hunks)x/genutil/v2/cli/commands.go(1 hunks)x/genutil/v2/cli/export.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
simapp/v2/app_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"
simapp/v2/export.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/genutil/v2/cli/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/genutil/v2/cli/export.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (5)
simapp/v2/export.go (2)
16-16: LGTM! Method signature update is well documented
The addition of the forZeroHeight parameter is well documented and the method signature change is clear and consistent.
Also applies to: 17-20
49-52: LGTM! Height reset logic is correct
The conditional height reset for zero-height exports is implemented correctly.
x/genutil/v2/cli/commands.go (1)
23-23: LGTM! Interface update is consistent
The ExportableApp interface has been correctly updated to match the implementation changes.
x/genutil/v2/cli/export.go (2)
20-21: LGTM! Constants are well defined
The new flag constants are clearly defined and follow the existing naming convention.
110-110:
Add error handling for MarkDeprecated
The error returned by MarkDeprecated should be checked to ensure the deprecation notice is properly set.
- cmd.Flags().Bool(flagForZeroHeight, false, "Export state to start at height zero (perform preprocessing)")
+ cmd.Flags().Bool(flagForZeroHeight, false, "Export state to start at height zero (perform preprocessing)")
+ if err := cmd.Flags().MarkDeprecated(flagForZeroHeight, "use --height=0 instead"); err != nil {
+ panic(fmt.Sprintf("failed to mark flag as deprecated: %v", err))
+ }
(cherry picked from commit 34f407d) # Conflicts: # server/v2/store/server.go # store/db/prefixdb_test.go # store/v2/root/builder.go # tests/integration/v2/distribution/fixture_test.go # tests/systemtests/go.sum
Co-authored-by: Julien Robert <[email protected]>
Description
ref: #22753
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
for-zero-heightto allow exports starting from height zero.Bug Fixes
AppDBBackendfield in the configuration is not empty, enhancing error handling.Chores
cosmossdk.io/systemtestsdependency in the module.Refactor
ExportAppStateAndValidatorsmethod to include an additional parameter for zero height.