Add omclickhouse CI workflow backed by ClickHouse container#127
Add omclickhouse CI workflow backed by ClickHouse container#127
Conversation
- add an omclickhouse CI workflow backed by a ClickHouse service container - add ClickHouse helpers to diag.sh and reuse them across omclickhouse tests - adjust the ClickHouse tests and tooling for external clients and HTTP defaults AI-Agent: ChatGPT
Summary of ChangesHello @rgerhards, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the testing infrastructure for the Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a CI workflow for omclickhouse using a containerized ClickHouse instance, which is a great improvement for testing. The changes involve refactoring the test scripts to use new helper functions in diag.sh, making them more flexible and configurable for different environments. The refactoring of test configurations using heredocs is a nice cleanup.
My review focuses on shell scripting best practices and potential security improvements. I've pointed out the use of eval, which can be risky, and a bug related to its unquoted use. I've also suggested a minor improvement for robustness in one of the new helper functions. Overall, the changes are well-structured and achieve the goal of improving the test setup.
| #wait_startup_pid /var/run/clickhouse-server/clickhouse-server.pid | ||
| if [ -n "$CLICKHOUSE_START_CMD" ]; then | ||
| printf 'starting clickhouse...\n' | ||
| eval "$CLICKHOUSE_START_CMD" & |
There was a problem hiding this comment.
Using eval can be a security risk as it executes arbitrary code from the CLICKHOUSE_START_CMD variable. While this might be acceptable in a controlled CI environment, it's a good practice to avoid it if possible. If CLICKHOUSE_START_CMD is expected to be a simple command with arguments, consider splitting it into an array to execute it more safely, similar to how CLICKHOUSE_CLIENT is handled in _clickhouse_exec.
| if [ "$1" == "" ]; then | ||
| printf 'TESTBENCH_ERROR: clickhouse_query requires a SQL statement\n' | ||
| error_exit 100 |
There was a problem hiding this comment.
For checking if a string is empty, it's more robust and conventional in shell scripting to use [ -z "$1" ] instead of [ "$1" == "" ]. This is a minor improvement for robustness and adherence to common shell scripting practices.
| if [ "$1" == "" ]; then | |
| printf 'TESTBENCH_ERROR: clickhouse_query requires a SQL statement\n' | |
| error_exit 100 | |
| if [ -z "$1" ]; then | |
| printf 'TESTBENCH_ERROR: clickhouse_query requires a SQL statement\n' | |
| error_exit 100 |
- revert the direct config writes and use add_conf so the testbench instrumentation runs while keeping the new ClickHouse helper parameters in place\n\nAI-Agent: Codex
## Summary - explain why libuuid stays disabled in the omclickhouse workflow so CI runs without the dependency. ## Testing - not run (not requested) AI-Agent: Codex
Switch the ClickHouse GitHub Actions job to run autoreconf directly so the job can pass custom configure options without pulling libuuid via autogen.sh. AI-Agent: ChatGPT
Adopt the run_codecov_base.yml configure matrix so the ClickHouse CI job builds with the full option set while explicitly enabling the module and disabling uuid to avoid the dependency. AI-Agent: ChatGPT
## Summary - poll ClickHouse readiness before preparing the test database and skip the suite when no endpoint is reachable - run the preparation helper via env so docker-exec based clients are honoured and guard the database drop step - make the prepare script idempotent by ignoring existing databases and failing fast when the client command errors ## Testing - not run (missing ClickHouse service in container) AI-Agent: Codex
The ClickHouse action helper defaulted to HTTPS, which broke the shell\ntests because the ClickHouse service provided in CI only exposes HTTP on\nport 8123. Restore the previous behaviour by defaulting the helper back\nto HTTP so existing scenarios connect successfully.\n\nAI-Agent: ChatGPT
* convert the TabSeparated EXPECTED strings to use real tab characters so they match ClickHouse 25.9.2.1 output\n* note in the scripts that the tabular and CSV expectations were validated with clickhouse local 25.9.2.1\n\nAI-Agent: Codex
- cache clickhouse readiness in the testbench and gate data-driven scenarios\n- let availability-sensitive tests skip individually while mock checks keep running\n- update the omclickhouse workflow yaml header so Actions picks it up\n\nAI-Agent: ChatGPT
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68d91a4937008332ae338a83c9f0ec97