http_client: fixed support of runtime tests#11724
http_client: fixed support of runtime tests#11724mabrarov wants to merge 4 commits intofluent:masterfrom
Conversation
📝 WalkthroughWalkthroughDefault test-skip list in the analysis script was reduced (two outputs removed), the HTTP client init now guards access to a possibly-NULL union connection, and CI workflow checkouts were redirected to a different reusable scripts repository/ref. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
c86abe0 to
31424a8
Compare
76edf7c to
f006d83
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/flb_http_client.c (1)
1020-1032: LGTM — null-guard correctly protects u_conn access in test paths.The added
if (u_conn != NULL)wrapper correctly prevents the NPE onu_conn->net/u_conn->upstreamwhencreate_http_client()is invoked in runtime-test mode with a NULL connection, while preserving the original behavior for the non-NULL path. Whenu_conn == NULL,c->original_net_setupstays NULL (fromflb_calloc), which is consistent with the NULL checks inhttp_client_clamp_connection_io_timeout(Line 1499),http_client_update_connection_io_timeout(Line 1524), andhttp_client_unbind_connection(Line 1592).Optional: this block now duplicates the net-setup selection logic already centralized in
http_client_bind_connection()(Lines 1567–1588). Consider replacing the body with a call tohttp_client_bind_connection(c, u_conn)after thec->u_conn = u_conn;assignment is removed from Line 1019 — the helper already setsc->u_conn, selectsoriginal_net_setup, copiesrequest_net_setup, and repointsu_conn->net. The only behavioral difference is that the helper additionally invokeshttp_client_update_connection_io_timeout(), which is a no-op here sinceresponse_timeout/read_idle_timeoutare still 0 at this point, so it should be safe. Feel free to defer if you prefer to keep this PR minimally scoped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_http_client.c` around lines 1020 - 1032, The duplicated net-setup logic in create_http_client (the if (u_conn != NULL) block that sets c->original_net_setup, copies into c->request_net_setup and rewires u_conn->net) should be replaced by a call to the existing helper http_client_bind_connection(c, u_conn); remove the manual c->u_conn = u_conn assignment that precedes it so the helper can set c->u_conn itself; keep in mind http_client_bind_connection also calls http_client_update_connection_io_timeout (which is harmless here because response_timeout/read_idle_timeout are 0), so simply call http_client_bind_connection(c, u_conn) instead of duplicating the selection and rewire code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/flb_http_client.c`:
- Around line 1020-1032: The duplicated net-setup logic in create_http_client
(the if (u_conn != NULL) block that sets c->original_net_setup, copies into
c->request_net_setup and rewires u_conn->net) should be replaced by a call to
the existing helper http_client_bind_connection(c, u_conn); remove the manual
c->u_conn = u_conn assignment that precedes it so the helper can set c->u_conn
itself; keep in mind http_client_bind_connection also calls
http_client_update_connection_io_timeout (which is harmless here because
response_timeout/read_idle_timeout are 0), so simply call
http_client_bind_connection(c, u_conn) instead of duplicating the selection and
rewire code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f71e2bd1-e11f-444f-bf3d-d8ce25282e9e
📒 Files selected for processing (2)
run_code_analysis.shsrc/flb_http_client.c
…analysis script Signed-off-by: Marat Abrarov <[email protected]>
…sts for Forward and Elasticsearch output plugins, for Disk I/O metrics and Process metrics input plugins. Signed-off-by: Marat Abrarov <[email protected]>
2ebb0e7 to
cd068ee
Compare
Signed-off-by: Marat Abrarov <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/unit-tests.yaml:
- Around line 116-117: In the unit-test workflow jobs that currently reference
the personal fork (the repository: mabrarov/fluent-bit-ci and ref:
feat/enable_tests entries in the unit-test job), revert those two fields to the
official CI repository and branch used elsewhere (e.g., repository:
fluent/fluent-bit-ci and the stable ref previously used) so both Linux and macOS
unit-test jobs match the other workflows (pr-perf-test.yaml and
call-run-integration-test.yaml); update the repository and ref values in the
unit-test job definitions to remove the personal fork and feature branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 434bfef2-66e6-4d3f-a46e-8e3cb76877ee
📒 Files selected for processing (2)
.github/workflows/unit-tests.yamlrun_code_analysis.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- run_code_analysis.sh
Summary
Fixed support of runtime tests in HTTP client. Refer to #11686 (comment).
The enabled tests were successfully executed on CI within cd068ee temporary commit using fluent/fluent-bit-ci#153 - refer to https://github.com/fluent/fluent-bit/actions/runs/24608193781?pr=11724.
Testing
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
Bug Fixes
Tests
Chores