config_format: yaml: Handle list format of parsers_file on service section#11926
config_format: yaml: Handle list format of parsers_file on service section#11926cosmo0920 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds YAML sequence support for the ChangesParsers file sequence list support
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/config_format/flb_cf_yaml.csrc/config_format/flb_cf_yaml.c:20:10: fatal error: 'fluent-bit/flb_info.h' file not found ... [truncated 744 characters] ... nux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include" 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6700422c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/internal/config_format_yaml.c (1)
326-360: ⚡ Quick winAdd a negative test for malformed
service.parsers_filelists.This new test covers the success path only. Please add one failure-path case (e.g., non-scalar list item) and assert parse/load fails, so regressions in
STATE_SECTION_VAL_LISTvalidation are caught.Based on learnings: tests for behavior changes should include focused coverage and validate both success and failure paths for parser-related handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/internal/config_format_yaml.c` around lines 326 - 360, Add a negative test alongside test_parser_conf_list that creates a YAML CF via flb_cf_yaml_create(…, FLB_PARSERS_LIST, …) but with a malformed service.parsers_file list (e.g., include a non-scalar item such as a map or sequence inside the list), then call flb_config_load_config_format(config, cf) and assert it fails (ret != 0) and that parsers count does not increase; use the same setup/teardown (flb_config_init, config->conf_path, flb_cf_destroy, flb_config_exit) and the same test helpers (TEST_CHECK, TEST_MSG) to validate the failure-path for STATE_SECTION_VAL_LIST validation.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config_format/flb_cf_yaml.c`:
- Around line 2001-2004: The call to flb_cf_section_property_add (used with
conf, state->cf_section->properties, state->key, value) is pointer-returning
here but the code tests for "< 0"; change the check to test for NULL instead,
e.g. verify the returned pointer is non-NULL and on NULL call flb_error("unable
to add property") and handle the failure path the same way the negative-check
previously did (stop parsing/return error). Update the conditional around
flb_cf_section_property_add accordingly so dropped properties are detected.
---
Nitpick comments:
In `@tests/internal/config_format_yaml.c`:
- Around line 326-360: Add a negative test alongside test_parser_conf_list that
creates a YAML CF via flb_cf_yaml_create(…, FLB_PARSERS_LIST, …) but with a
malformed service.parsers_file list (e.g., include a non-scalar item such as a
map or sequence inside the list), then call
flb_config_load_config_format(config, cf) and assert it fails (ret != 0) and
that parsers count does not increase; use the same setup/teardown
(flb_config_init, config->conf_path, flb_cf_destroy, flb_config_exit) and the
same test helpers (TEST_CHECK, TEST_MSG) to validate the failure-path for
STATE_SECTION_VAL_LIST validation.
🪄 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: 9d4e8741-a21b-4093-90a2-6c34a6940f61
📒 Files selected for processing (4)
src/config_format/flb_cf_yaml.ctests/internal/config_format_yaml.ctests/internal/data/config_format/yaml/parsers/parsers-extra.conftests/internal/data/config_format/yaml/parsers/parsers-list.yaml
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-handle-multi-parsers_file-on-service-section
branch
from
d670042 to
18995b7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/internal/config_format_yaml.c (1)
327-327: ⚡ Quick winUse
const char *for immutable fixture paths.
pathis only read and all call sites pass string literals. Making itconst char *tightens the helper contract and avoids accidental mutation later.Suggested diff
-static void test_parser_conf_list_file(char *path) +static void test_parser_conf_list_file(const char *path)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/internal/config_format_yaml.c` at line 327, The helper function signature test_parser_conf_list_file currently takes a mutable char *path even though it only reads the value and all callers pass string literals; change the parameter type to const char *path to express immutability and prevent accidental modification. Update the function declaration/definition for test_parser_conf_list_file and any prototypes/usages to accept const char * so callers remain compatible with string literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/internal/config_format_yaml.c`:
- Line 327: The helper function signature test_parser_conf_list_file currently
takes a mutable char *path even though it only reads the value and all callers
pass string literals; change the parameter type to const char *path to express
immutability and prevent accidental modification. Update the function
declaration/definition for test_parser_conf_list_file and any prototypes/usages
to accept const char * so callers remain compatible with string literals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8169978-7ae8-4361-9d59-e04edb0f3b8f
📒 Files selected for processing (5)
src/config_format/flb_cf_yaml.ctests/internal/config_format_yaml.ctests/internal/data/config_format/yaml/parsers/parsers-extra.conftests/internal/data/config_format/yaml/parsers/parsers-list-camel-case.yamltests/internal/data/config_format/yaml/parsers/parsers-list.yaml
✅ Files skipped from review due to trivial changes (3)
- tests/internal/data/config_format/yaml/parsers/parsers-list.yaml
- tests/internal/data/config_format/yaml/parsers/parsers-list-camel-case.yaml
- tests/internal/data/config_format/yaml/parsers/parsers-extra.conf
🚧 Files skipped from review as they are similar to previous changes (1)
- src/config_format/flb_cf_yaml.c
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
1 participant
In this PR, we support a list format of parsers_file.
Closes #11910.
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
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
New Features
Tests