refactor: remove viper as a direct dependency #21635
Conversation
WalkthroughWalkthroughThe pull request introduces a new Changes
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: .coderabbit.yml Files selected for processing (2)
Additional context usedPath-based instructions (2)
Additional comments not posted (7)
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:server/v2
github-actions
bot
removed
C:server/v2
kocubinski
requested review from
a team,
facundomedica,
cool-develope,
akhilkumarpilli,
lucaslopezf,
hieuvubk,
julienrbrt,
testinginprod and
aaronc
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
core/server/config.go (1)
5-7: LGTM!The
DynamicConfiginterface provides a clean abstraction for dynamic configuration management and adheres to the Uber Go Style Guide.Consider using a more specific return type for the
Getmethod instead ofanyto improve type safety. For example:type DynamicConfig interface { Get(string) (any, bool) GetString(string) string }This would allow the caller to check if the key exists and handle the case accordingly.
runtime/v2/go.mod (1)
Dependencies Still in Use: Reconsider Removal
The following dependencies are still being imported in the codebase, which suggests that their removal from
go.modcould lead to issues:
github.com/mitchellh/mapstructureis imported inserver/v2/config.go.github.com/magiconair/propertiesis imported incrypto/keys/secp256k1/secp256k1_cgo_test.go.Please verify the necessity of these dependencies and consider refactoring the code to remove their usage if they are no longer needed. Alternatively, ensure they remain in
go.modif they are still required.Analysis chain
Line range hint
16-27: Removal of Viper dependency is approved, but verify the impact of removing other dependencies.The removal of the
github.com/spf13/viperdependency aligns with the PR objective and is approved.However, several other dependencies have also been removed. Please verify that removing these dependencies does not introduce any unintended consequences or break any functionality.
Run the following script to verify the impact of removing the dependencies:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: # - Verify that the removed dependencies are not imported in the codebase. # - Ensure that the tests pass after removing the dependencies. # Test: Search for the removed dependencies in the codebase. Expect: No results. rg --type go $'github.com/hashicorp/hcl|github.com/magiconair/properties|github.com/mitchellh/mapstructure|github.com/pelletier/go-toml/v2|github.com/sagikazarmark/locafero|github.com/sagikazarmark/slog-shim|github.com/sourcegraph/conc|github.com/spf13/pflag|github.com/subosito/gotenv|go.uber.org/multierr' # Test: Run the tests. Expect: All tests pass. go test ./...Length of output: 2756
Script:
#!/bin/bash # Description: Verify if the other removed dependencies are still being imported in the codebase. # Test: Search for the remaining removed dependencies in the codebase. Expect: No results. rg --type go $'github.com/hashicorp/hcl|github.com/magiconair/properties|github.com/mitchellh/mapstructure|github.com/sagikazarmark/locafero|github.com/sagikazarmark/slog-shim|github.com/sourcegraph/conc|github.com/subosito/gotenv|go.uber.org/multierr'Length of output: 397
tests/integration/genutil/init_test.go (1)
Line range hint
44-97: Consider adding more test cases.The test function covers the happy path scenario well. However, consider adding more test cases to cover error scenarios and improve the test coverage.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumruntime/v2/go.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumx/upgrade/go.sumis excluded by!**/*.sum
Files selected for processing (24)
- core/server/config.go (1 hunks)
- go.mod (1 hunks)
- runtime/v2/builder.go (6 hunks)
- runtime/v2/go.mod (6 hunks)
- runtime/v2/module.go (4 hunks)
- simapp/go.mod (1 hunks)
- simapp/simd/cmd/testnet_test.go (2 hunks)
- simapp/v2/app_di.go (4 hunks)
- simapp/v2/go.mod (1 hunks)
- simapp/v2/simdv2/cmd/commands.go (4 hunks)
- simapp/v2/simdv2/cmd/root_di.go (1 hunks)
- tests/e2e/genutil/export_test.go (1 hunks)
- tests/go.mod (2 hunks)
- tests/integration/genutil/export_test.go (1 hunks)
- tests/integration/genutil/genaccount_test.go (6 hunks)
- tests/integration/genutil/init_test.go (8 hunks)
- testutil/network/util.go (2 hunks)
- testutil/x/genutil/helper.go (1 hunks)
- x/auth/tx/config/depinject.go (4 hunks)
- x/genutil/v2/cli/commands.go (1 hunks)
- x/genutil/v2/cli/export.go (3 hunks)
- x/genutil/v2/types.go (1 hunks)
- x/upgrade/depinject.go (3 hunks)
- x/upgrade/go.mod (3 hunks)
Files skipped from review due to trivial changes (8)
- simapp/go.mod
- simapp/simd/cmd/testnet_test.go
- simapp/v2/go.mod
- tests/e2e/genutil/export_test.go
- tests/integration/genutil/export_test.go
- tests/integration/genutil/genaccount_test.go
- testutil/x/genutil/helper.go
- x/genutil/v2/cli/commands.go
Additional context used
Path-based instructions (13)
core/server/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/genutil/v2/types.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.x/upgrade/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/root_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/builder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.testutil/network/util.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/app_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/tx/config/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/integration/genutil/init_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/go.mod (1)
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (39)
x/genutil/v2/types.go (1)
4-4: Approve the change fromloggertoctx, but request more information about the removal ofviper.
Replacing
logger log.Loggerwithctx context.Context:
- This change is approved as it can enhance the function's ability to handle cancellation signals and deadlines, improving its integration with other components that utilize context.
Removing
viper *viper.Viper:
- Please provide more information about the reason for removing the
viperparameter and its impact on the codebase.- Is this change related to a broader refactoring or a shift in how configuration is managed within the application?
Also applies to: 11-11
x/genutil/v2/cli/export.go (3)
Line range hint
24-24: LGTM!The changes to the
ExportCmdfunction signature, which remove theloggerandviperparameters, align with the PR objective of eliminating the direct dependency on the Viper library. This simplifies the command's execution flow by reducing dependencies on these variables.
63-63: LGTM!The modification to the
appExporterfunction call, which now passes the command's context directly instead of theloggerandviper, aligns with the PR objective of introducing a newDynamicConfiginterface to enhance dynamic configuration management and replace the existing configuration management using theviperlibrary. This change streamlines the approach to exporting application state by relying solely on the command context and the other parameters.
100-103: LGTM!The formatting changes to the flag definitions enhance the code's readability and maintainability without affecting functionality. The changes adhere to the Uber Golang style guide.
x/upgrade/depinject.go (4)
10-10: LGTM!The import statement is correct and aligns with the PR objective of using the new
DynamicConfiginterface from theserverv2package.
46-47: LGTM!The changes to the
ModuleInputsstruct are correct and align with the PR objective of transitioning from the oldAppOptsto the newDynamicConfigfor configuration management. Marking both fields as optional ensures backward compatibility during the transition.
63-69: LGTM!The changes to the
ProvideModulefunction are correct and align with the PR objective of transitioning fromAppOptstoDynamicConfigfor configuration management. The code correctly prioritizesDynamicConfigoverAppOptsand retrieves the necessary configuration values.
Line range hint
1-100: Code style and conformity with the Uber Golang style guide: LGTM!The code follows the Uber Golang style guide, is well-formatted, and readable. No changes are required.
runtime/v2/go.mod (1)
8-8: LGTM!The change to replace the
cosmossdk.io/coremodule with a local path is approved. This change aligns with the PR objective of using a local development version of the module.simapp/v2/simdv2/cmd/root_di.go (1)
85-85: Verify the removal of the generic type parameterTfrom theinitRootCmdfunction.The generic type parameter
Thas been removed from theinitRootCmdfunction signature and the function call at line 85. This change suggests that theinitRootCmdfunction is now constrained to work with a specific transaction type or a default transaction type, potentially limiting its flexibility in handling different transaction types.Please verify if this change aligns with the intended design and functionality of the
initRootCmdfunction. Consider the following:
- Is the
initRootCmdfunction expected to handle only a specific transaction type or a default transaction type?- Will the removal of the generic type parameter
Timpact the flexibility and extensibility of theinitRootCmdfunction in the future?- Are there any downstream dependencies or code that rely on the generic nature of the
initRootCmdfunction?If the change is intentional and aligns with the desired behavior, please ensure that the documentation and comments are updated accordingly to reflect the new functionality of the
initRootCmdfunction.simapp/v2/simdv2/cmd/commands.go (4)
89-98: Improved modularity in genesisCommand function.The removal of the
appExportparameter and retrieval of the function from themoduleManagerwithin thegenesisCommandfunction body is a positive change. It promotes a more encapsulated design where the command construction relies on the module manager's state rather than external parameters.This change improves modularity, reduces coupling, and enhances the overall design of the command structure.
151-167: Enhanced context management and error handling in appExport function.The update to the
appExportfunction signature to include actx context.Contextparameter and the extraction ofviperandloggerfrom the context is a positive change. It signifies a transition to a context-aware approach, allowing the function to access necessary values from the context.Furthermore, the type checking of the extracted values enhances error handling and type safety within the function. It ensures that the function is more robust against incorrect usage and provides informative error messages when the expected types are not met.
Overall, these changes improve context management, error handling, and the overall reliability of the
appExportfunction.
4-4: Appropriate import of "context" package.The import of the
"context"package aligns with the usage ofcontext.Contextin the updatedappExportfunction. It is a necessary addition to support the context-aware approach introduced in the function.
12-12: Appropriate import of corectx package.The import of the
corectxpackage with an alias suggests its usage in the updatedappExportfunction. It likely provides additional context-related functionality or constants used in the function, supporting the context-aware approach.runtime/v2/module.go (3)
20-20: LGTM!The new import statement is necessary to use the
server.DynamicConfigtype.
148-148: Looks good!The changes to the
AppInputsstruct align with the PR objective to remove the direct dependency on Viper and introduce a new abstraction for dynamic configuration management.
159-160: LGTM!The changes to the
SetupAppBuilderfunction correctly handle the transition from using Viper to the newDynamicConfigfor configuration management. The conditional assignment ensures backward compatibility whenDynamicConfigis not provided.runtime/v2/builder.go (5)
12-12: LGTM!The import is necessary to use the
server.DynamicConfigtype in theAppBuilderstruct.
27-28: LGTM!The changes to the
AppBuilderstruct are part of the refactoring to remove the direct dependency on the Viper library and simplify the management of store options.
123-129: LGTM!The changes to retrieve the home directory path and create a new database using
server.DynamicConfigare consistent with the refactoring to remove the direct dependency on the Viper library.
134-142: LGTM!The change to create
rootstore.FactoryOptionsusinga.storeOptionssimplifies the configuration handling within theAppBuilder.
Line range hint
205-233: LGTM!The formatting change improves the readability of the
AppBuilderWithTxValidatorfunction, and the newAppBuilderWithStoreOptionsfunction enhances the configurability of theAppBuilderby allowing store options to be set through a functional option pattern.testutil/network/util.go (1)
183-187: LGTM!The changes to the configuration setup process in the
collectGenFilesfunction look good. Explicitly defining the configuration parameters improves clarity and control over the configuration process.The code segment conforms to the Uber Go Style Guide.
simapp/v2/app_di.go (1)
Line range hint
101-200: LGTM!The changes to the
NewSimAppfunction look good:
The introduction of the
storeOptionsvariable and the associated logic to populate it from the Viper configuration enhances the flexibility of the application setup by allowing dynamic configuration of store options.The code adheres to the Uber Go Style Guide and follows idiomatic Go practices.
Error handling is properly implemented when unmarshaling the sub-configuration.
The modifications to the
appBuilder.Build()call correctly integrate thestoreOptionsinto the application builder process.Overall, these changes improve the configurability of the
NewSimAppfunction and provide a more flexible application setup.go.mod (1)
188-188: LGTM! The change enhances local development.The new
replacedirective that maps the module pathcosmossdk.io/coreto the local directory./coreis a good addition. It allows the Go module system to reference the local implementation of thecorepackage instead of fetching it from the remote repository.This change indicates a shift towards local development or testing of the
corepackage without relying on external sources, which can be beneficial for faster iteration and debugging.x/auth/tx/config/depinject.go (3)
18-18: LGTM!The import statement is correct and necessary for using the
DynamicConfigtype in theModuleInputsstruct.
69-69: LGTM!The addition of the optional
DynamicConfigfield is a key change that enables the transition from using Viper to the new dynamic configuration approach. Making it optional allows for backward compatibility.
129-130: LGTM!The changes reflect the transition from using Viper to the new
DynamicConfigfor retrieving configuration values. The conditional check ensures that the new approach is used only when all the required dependencies (AccountKeeper,BankKeeper, andDynamicConfig) are available, maintaining backward compatibility.x/upgrade/go.mod (2)
162-162: LGTM!The change to make
github.com/spf13/viperan indirect dependency aligns with the PR objective of removing the direct dependency on theviperlibrary.
206-208: LGTM!The addition of replace directives for
cosmossdk.io/core,cosmossdk.io/core/testing, andcosmossdk.io/storemodules to use local paths aligns with the PR objective of improving modularity in the codebase. Using local paths for these modules can facilitate testing and development.tests/integration/genutil/init_test.go (8)
Line range hint
99-132: LGTM!The code changes are approved.
Line range hint
134-161: LGTM!The code changes are approved.
Line range hint
163-208: LGTM!The code changes are approved.
Line range hint
210-242: LGTM!The code changes are approved.
Line range hint
244-254: LGTM!The code changes are approved.
Line range hint
256-304: LGTM!The code changes are approved.
Line range hint
306-340: LGTM!The code changes are approved.
Line range hint
342-374: LGTM!The code changes are approved.
tests/go.mod (1)
53-53: Approved: Direct dependency onjackfan.us.kg/rs/zerologis acceptable.The change from an indirect to a direct dependency on the
zerologlogging library aligns with the project's logging requirements and is acceptable.
* main: docs(client/debug): correct `debug raw-bytes` command example (#21671) build: don't reinstall golangci-lint if already installed (#21662) refactor(server/v2): kill viper from server components (#21663) chore: sync changelog with latest releases (#21658) refactor: remove viper as a direct dependency (#21635) ci: centralized job for rocksdb libaries cache (#21657) fix: remove stray fmt.Println (#21661)
Co-authored-by: Matt Kocubinski <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
This fell out as part of the v2 integration test work. It is desirable to interact with config through an abstraction rather than viper specifically.
Specifically, this PR introduces
core/server.DynamicConfigto interface with runtime config instead of viper, analogous to server v1AppOptions. This completely removes viper fromruntime/v2/go.mod, and moves viper to an indirect (from direct) dependency inx/genutilandx/upgrade.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
Bug Fixes
Refactor
Chores