refactor(server/v2): kill viper from server components #21663
Conversation
julienrbrt
requested review from
facundomedica,
sontrinh16,
a team and
hieuvubk
as code owners
github-actions
bot
added
C:server/v2
WalkthroughWalkthroughThis pull request introduces a significant change in configuration management across several components of the server. The primary modification involves replacing the Changes
Possibly related PRs
Suggested labels
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
|
julienrbrt
added
the
backport/v0.52.x
![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 (1)
server/v2/server_mock_test.go (1)
34-36: LGTM! Update the method documentation.The method signature change aligns with the PR objective of removing Viper from the server components.
If the empty method body is a placeholder, consider adding a TODO comment to track the pending implementation.
Update the method documentation to reflect the change in the configuration parameter type from
*viper.Vipertomap[string]any.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (12)
- server/v2/api/grpc/server.go (3 hunks)
- server/v2/api/grpcgateway/server.go (3 hunks)
- server/v2/cometbft/config.go (2 hunks)
- server/v2/cometbft/go.mod (2 hunks)
- server/v2/cometbft/server.go (2 hunks)
- server/v2/commands.go (1 hunks)
- server/v2/config.go (1 hunks)
- server/v2/config_test.go (1 hunks)
- server/v2/server.go (3 hunks)
- server/v2/server_mock_test.go (2 hunks)
- server/v2/server_test.go (1 hunks)
- server/v2/store/server.go (2 hunks)
Files skipped from review due to trivial changes (1)
- server/v2/cometbft/go.mod
Additional context used
Path-based instructions (11)
server/v2/server_mock_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"server/v2/config_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"server/v2/store/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/server_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"server/v2/cometbft/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/grpcgateway/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/grpc/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (15)
server/v2/config_test.go (2)
Line range hint
14-25:
This test function has no changes in the provided diff. Skipping the review.
34-34: LGTM!The changes simplify the code by using the
cfgvariable that represents all settings, instead of directly using thevvariable. This does not alter the behavior of the test.The code changes are approved.
Also applies to: 37-37, 44-44
server/v2/store/server.go (1)
26-33: LGTM!The changes to the
Initmethod are approved. The shift from using the Viper library to a more generic map-based configuration approach simplifies the configuration handling mechanism and reduces dependencies on external libraries. The changes are correctly implemented and there are no apparent issues.server/v2/config.go (1)
42-54: LGTM!The changes to the
UnmarshalSubConfigfunction look good:
- The function signature update improves flexibility by allowing it to work with a generic map instead of being coupled to the Viper library.
- The updated logic correctly handles the case when the sub-configuration name is empty by unmarshaling the entire map.
- The function uses the
mapstructurelibrary to decode the configuration into the target struct, which provides additional decoding options.- Error handling is done correctly by returning descriptive errors.
The code changes are approved.
server/v2/server_test.go (3)
59-59: LGTM!The change to derive a configuration map from the Viper instance using
AllSettings()is approved. This seems to be part of a larger effort to move away from direct usage of Viper.
63-63: LGTM!The change to initialize the
grpcServerusing the derived configuration mapcfginstead of the Viper instance is approved. This change is consistent with the previous change to use a configuration map.
67-67: LGTM!The change to initialize the
storeServerusing the derived configuration mapcfginstead of the Viper instance is approved. This change is consistent with the previous changes to use a configuration map.server/v2/cometbft/config.go (1)
Line range hint
1-50: Removal of Viper dependency for configuration managementThe removal of the
getConfigTomlFromViperfunction and theviperimport statement indicates a shift away from using Viper for configuration management within this context. This change aligns with the PR objective of killing Viper from server components and may simplify the configuration process or indicate a move towards a different configuration handling strategy.However, the overall control flow of the configuration management has been altered by this removal. Any previous reliance on the
getConfigTomlFromViperfunction for setting up application configuration will now need to be addressed elsewhere in the codebase.Please ensure that the configuration management is properly handled after this change. Verify that any parts of the application that previously depended on the
getConfigTomlFromViperfunction for configuration retrieval have been updated accordingly.You can use the following script to search for potential usage of the removed function across the codebase:
server/v2/api/grpcgateway/server.go (2)
Line range hint
85-97: LGTM! The changes improve configuration flexibility and maintainability.The modifications to the
Initmethod signature and internal logic enhance the flexibility of the configuration handling process by decoupling it from the Viper library. The method now accepts a more genericmap[string]anystructure for configuration management, making it easier to integrate with different configuration sources.The added check for the presence of entries in the
cfgmap ensures that the configuration is only unmarshaled when necessary, preventing potential errors.The renaming of the
cfgvariable toserverCfgimproves code clarity, making it more apparent that the variable represents the unmarshaled server configuration.Overall, these changes contribute to better maintainability and adaptability of the codebase.
Line range hint
85-97: Code conforms to the Uber Go Style Guide.The reviewed code segment adheres to the conventions outlined in the Uber Go Style Guide. The naming, formatting, and structure of the code are consistent with the guidelines, promoting readability and maintainability.
The utilization of generics with the
GRPCGatewayServer[T]struct aligns with the recommendations for leveraging Go's type system effectively, enabling type-safe and reusable code.No deviations from the style guide were observed in this code segment.
server/v2/api/grpc/server.go (2)
49-61: LGTM!The changes to the
Initmethod look good. The shift from using the Viper library to a more generic map structure for configuration management enhances flexibility and simplifies the initialization process. The updated logic correctly handles the new configuration type and ensures that the server's configuration is set up based on the provided input.The changes also include proper checks to handle cases where the
cfgmap is empty and updates to how the maximum send and receive message sizes are accessed using theserverCfg.Overall, the modifications improve the configuration handling within the gRPC server initialization process.
49-61: Code style looks good!The
Initmethod follows the Uber Golang style guide:
- The method signature and parameters adhere to the recommended naming conventions.
- The error handling is in line with the recommended practices.
- The method length is within the suggested limit.
No deviations from the style guide were found.
server/v2/commands.go (1)
115-115: LGTM!The change aligns with the PR objective of removing the Viper library from the server components. The
server.Initfunction now receives a comprehensive set of settings viav.AllSettings(), instead of the rawviper.Viperinstance.The change is localized and does not introduce any new errors or edge cases.
server/v2/server.go (1)
191-211: LGTM!The changes to the
Initmethod signature and implementation look good. The move from the Viper library to a more generic map structure for configuration enhances the modularity and adaptability of the server's initialization process. The updated logic to unmarshal configuration and the new variable names accurately reflect the new configuration type. The overall flow of the method remains intact, and the code changes conform to the Uber Go Style Guide.Great work on this refactor! The changes are approved.
server/v2/cometbft/server.go (1)
60-71: Approve: The changes effectively remove the dependency on the Viper library and introduce a more flexible configuration management approach.The modifications to the
Initmethod signature and the corresponding updates to the configuration retrieval logic align well with the objective of eliminating the reliance on the Viper library. By accepting a genericmap[string]anyinstead of aviper.Viperinstance, the code becomes more adaptable to different configuration sources and management strategies.The updated logic for retrieving the home directory, unmarshaling configuration, and obtaining the
chainIDdemonstrates a consistent and coherent approach to working with the new configuration map. The fallback mechanism for reading the genesis file remains intact, ensuring backward compatibility.Overall, the changes are well-structured, maintain the functionality of the
Initmethod, and provide a solid foundation for more flexible configuration management in the future.Also applies to: 81-94
(cherry picked from commit 88cfebe) # Conflicts: # server/v2/api/grpc/server.go # server/v2/api/grpcgateway/server.go # server/v2/commands.go # server/v2/config.go # server/v2/config_test.go # server/v2/server.go # server/v2/server_mock_test.go # server/v2/server_test.go # server/v2/store/server.go
* 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)
…) (#21673) Co-authored-by: Julien Robert <[email protected]>
Description
@kocubinski rightfully pointed viper may not be needed as well in server components.
Follow-up of #21635
This gives us:
Main server, reads the app.toml and config.toml using viper, and pass down the whole config to server components as a map[string]any. The server components are able to unmarshal their own config from that map.
The app still needs viper, but this is unrelated to this PR.
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
Chores