refactor: remove unused check_os_params (#14)#125
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request removes the unused check_os_params functionality from the codebase. The OS parameter checking logic was intended to verify system configuration requirements but has proven too strict in practice and is no longer used. The PR maintains backward compatibility by keeping the strict_check_os_params configuration parameter but marking it as deprecated.
Changes:
- Removed
check_os_paramsimplementation files (header and source) - Removed related unit test file and build configurations
- Removed the function call from server initialization
- Deprecated the
strict_check_os_paramsconfiguration parameter for backward compatibility
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/observer/ob_check_params.h | Deleted header file containing CheckAllParams class and check_os_params function declarations |
| src/observer/ob_check_params.cpp | Deleted implementation file with OS parameter validation logic (376 lines) |
| unittest/observer/test_check_os_params.cpp | Deleted unit test file for the removed functionality |
| src/observer/ob_server.cpp | Removed include directive and check_os_params function call from server initialization |
| src/observer/CMakeLists.txt | Removed ob_check_params.cpp from build configuration |
| unittest/observer/CMakeLists.txt | Removed test_check_os_params from test build configuration |
| src/share/parameter/ob_parameter_seed.ipp | Updated strict_check_os_params parameter with deprecation notice and clarified it has no effect |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
plz fix your conflict |
|
Do not worry about the coding activity, I marked your merged. |
a8e1fb7 to
1c45883
Compare
|
@LINxiansheng @hnwyllmm Hi, I‘ve fixed the conflict. Please check. When I opened this PR, I ran unit tests on a virtual machine(Ubuntu24.04) as well, and observed similar failures:
So the failing cases are reproducible on both my virtual machine and physical machine, and do not appear to be environment-specific. Could you please help check whether these failures can be reproduced on your side? Let me know if you’d like me to open a separate issue and/or submit an additional PR to address these test failures. |
1c45883 to
d576c11
Compare
|
@LINxiansheng @hnwyllmm I've addressed the feedback and updated the PR. |
Task Description
close #14
Solution Description
Delete
check_os_paramsimplementationPassed Regressions
ctest: 96% tests passed(303/317).The failed test cases are not directly related to the changes in this PR.
Upgrade Compatibility
Keep the configuration option strict_check_os_params for backward compatibility, but mark it as deprecated so that existing configuration files or scripts continue to work without causing any functional behavior changes.
Other Information
Release Note