(DAL) Remove global variable #1813
Conversation
WalkthroughWalkthroughThe changes focus on refactoring and improving the modularity of the DAL API server initialization and its components. Key modifications include the restructuring of controller setup and usage, introducing a Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- node/pkg/dal/api/controller.go (9 hunks)
- node/pkg/dal/api/route.go (1 hunks)
- node/pkg/dal/app.go (1 hunks)
- node/pkg/dal/collector/collector.go (1 hunks)
- node/pkg/dal/tests/api_test.go (8 hunks)
- node/pkg/dal/tests/collector_test.go (4 hunks)
- node/pkg/dal/tests/main_test.go (4 hunks)
- node/pkg/dal/utils/initializer/initializer.go (6 hunks)
Files skipped from review due to trivial changes (1)
- node/pkg/dal/collector/collector.go
Additional context used
Learnings (1)
node/pkg/dal/api/route.go (1)
Learnt from: nick-bisonai PR: Bisonai/orakl#1791 File: node/pkg/dal/api/controller.go:0-0 Timestamp: 2024-07-12T02:42:57.258Z Learning: In the `node/pkg/dal/api/controller.go`, the `handleWebsocket` function no longer requires explicit validation for the `X-Api-Key` header, as this is handled by the `gofiber keyauth` middleware in `initializer.go`.
Additional comments not posted (7)
node/pkg/dal/api/route.go (1)
14-14: Approved: Updated websocket handler usage.The change to use
HandleWebsocketin the websocket route is consistent with the PR's goal to refactor and improve the code structure.node/pkg/dal/app.go (1)
39-41: Ensure proper shutdown sequence.The deferred shutdown of the application is a good practice. However, ensure that other resources are also cleanly shutdown to prevent any resource leakage.
node/pkg/dal/utils/initializer/initializer.go (1)
Line range hint
28-75: RefactoredSetupfunction with enhanced error handling and middleware setup.The changes to the
Setupfunction introduceapiControllerandkeyCacheas parameters, enhancing the function's modularity and reducing reliance on global state. The added null checks forapiControllerandkeyCacheimprove robustness by preventing potential runtime errors due to null references.The middleware setup now includes context and key cache information, which aligns with the PR's objective to enhance the application's configurability and maintainability.
node/pkg/dal/api/controller.go (2)
Line range hint
18-36: EnhancedSetupfunction with improved error handling and structure.The
Setupfunction now properly handles errors from configuration requests and collector creation, providing detailed log messages for failures. This improves the function's reliability and maintainability by ensuring that errors are not silently ignored and are adequately reported.The separation of concerns is evident as the function now solely focuses on setting up the controller with the necessary configurations and collector, rather than handling multiple unrelated tasks.
Line range hint
105-157: Improved WebSocket handling inHandleWebsocketfunction.The
HandleWebsocketfunction has been refactored to use local storage (conn.Locals) for passing theapiControllerand context, aligning with best practices for handling state in WebSocket servers. This change reduces dependencies on global variables and enhances the function's testability and reusability.The detailed error handling and logging within the function provide clear diagnostics in case of failures, which is crucial for maintaining live WebSocket connections.
node/pkg/dal/tests/api_test.go (2)
Line range hint
44-68: Updated test cases to reflect new controller structure and functionality.The test cases have been updated to use the new
Controllerstructure, ensuring that they remain effective and relevant after the refactoring. The tests now properly handle new configurations and collector setups, aligning with the changes in the main codebase.The use of
Controller.Collectorin the tests ensures that the collector's functionality is thoroughly tested, which is crucial for maintaining the reliability of data processing and retrieval features.
Line range hint
143-203: Enhanced WebSocket testing inTestApiWebsocket.The test case for WebSocket functionality (
TestApiWebsocket) has been significantly improved to test the new WebSocket handling code. The test ensures that the WebSocket connections are properly managed and that data is correctly processed and sent over the WebSocket.This test is crucial for verifying the robustness and reliability of the WebSocket implementation, especially after the refactoring changes made in the main codebase.
| return err | ||
| } | ||
| defer func() { | ||
| _ = app.Shutdown() |
There was a problem hiding this comment.
just out of curiosity, I've seen this pattern a lot in the code where the return value is assigned to a blank identifier, which is not used. Afaik the blank identifier is useful when a function returns multiple values and you are not interested in some of them. Is it still a good practice to use the blank identifier in this way?
There was a problem hiding this comment.
I don't think so,
it didn't pass the linter when I didn't assign blank identifier, so it kind of became habit I think
Description
Refactor package to remove global variables
Type of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment