config_format: yaml: handle configuration metadata on yaml conf#11690
config_format: yaml: handle configuration metadata on yaml conf#11690cosmo0920 wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested reviewers
Poem
🚥 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/internal/config_format_yaml.c (1)
840-887: Please assert the config still loads withmetadatapresent.This verifies parse-time retention, but the feature contract is “accept and ignore operationally.” Adding a
flb_config_load_config_format()success check here would catch any later regression wherecf->othersstarts affecting runtime config loading.As per coding guidelines,
tests/**: "Add or update tests for behavior changes, especially protocol parsing and encoder/decoder paths" and "Validate both success and failure paths (invalid payloads, boundary sizes, null/missing fields) in tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/internal/config_format_yaml.c` around lines 840 - 887, The test parses a YAML config into a flb_cf (test_metadata_section via flb_cf_yaml_create) but doesn’t assert that runtime config loading still succeeds when metadata is present; call flb_config_load_config_format(cf) after the existing parse/metadata assertions and add a TEST_CHECK that it returns success (e.g., 0) to ensure cf->others/metadata is ignored at load time; finally proceed to flb_cf_destroy(cf) as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config_format/flb_cf_yaml.c`:
- Line 1839: In the STATE_METADATA handling in flb_cf_yaml.c ensure top-level
metadata scalar nodes are routed through state_variant_parse_scalar() instead of
falling through to flb_cf_section_property_add(); update the STATE_METADATA case
(and the similar block around lines 1921-1937) to detect scalar tokens at the
top metadata level and call state_variant_parse_scalar() to create CFL_VARIANT_*
values, then add the resulting variant to the metadata section rather than
storing the raw string via flb_cf_section_property_add(), preserving type parity
with nested metadata parsing.
- Around line 2420-2423: When cfl_kvlist_insert(state->cf_section->properties,
state->key, variant) fails the newly created nested value stored in variant is
leaked; fix by destroying/freeing variant before returning (call the appropriate
cleanup function, e.g., cfl_variant_destroy(variant)) and then return
YAML_FAILURE. Update the error path in the block that checks the return of
cfl_kvlist_insert so it releases variant (and nulls it if your codebase
convention requires) to avoid the memory leak; refer to symbols
cfl_kvlist_insert, state->cf_section->properties, state->key, and variant to
locate the change.
---
Nitpick comments:
In `@tests/internal/config_format_yaml.c`:
- Around line 840-887: The test parses a YAML config into a flb_cf
(test_metadata_section via flb_cf_yaml_create) but doesn’t assert that runtime
config loading still succeeds when metadata is present; call
flb_config_load_config_format(cf) after the existing parse/metadata assertions
and add a TEST_CHECK that it returns success (e.g., 0) to ensure
cf->others/metadata is ignored at load time; finally proceed to
flb_cf_destroy(cf) as before.
🪄 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: b477964d-a275-4772-af84-5bc1479daa1c
📒 Files selected for processing (4)
src/config_format/flb_cf_yaml.ctests/internal/config_format_yaml.ctests/internal/data/config_format/yaml/metadata.yamltests/internal/data/config_format/yaml/other_with_nested_map.yaml
![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: 89d9f20510
ℹ️ 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".
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 `@tests/internal/config_format_yaml.c`:
- Around line 472-480: The test currently dereferences second_processor and
sampling_type immediately after TEST_CHECK, which can crash on regression;
update the assertions in tests/internal/config_format_yaml.c to first verify
second_processor != NULL and second_processor->type == CFL_VARIANT_KVLIST (from
cfl_array_fetch_by_index) before accessing second_processor->data, and similarly
verify sampling_type != NULL and sampling_type->type == CFL_VARIANT_STRING (from
cfl_kvlist_fetch) before reading sampling_type->data.as_string; implement these
guards using explicit if checks around the existing checks (or convert
TEST_CHECK to conditional TEST_SKIP/return on failure) so failures produce clean
test assertions instead of dereference crashes.
🪄 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: 2e234d49-63da-4ee4-90dc-8c49795acb02
📒 Files selected for processing (2)
tests/internal/config_format_yaml.ctests/internal/data/config_format/yaml/processors.yaml
✅ Files skipped from review due to trivial changes (1)
- tests/internal/data/config_format/yaml/processors.yaml
cosmo0920
force-pushed
the
cosmo0920-handle-configuration-metadata-on-yaml-conf
branch
from
aaef92c to
e56d7b5
Compare
|
I think for this implementation we need a section name that is generic enough for fluent bit use cases, note that |
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-handle-configuration-metadata-on-yaml-conf
branch
from
e56d7b5 to
f97cead
Compare
For now, I updated as "ignorable" to show that this section can be ignored and meaning less for other configuration and only for adding metadata with structured attributes. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/config_format/flb_cf_yaml.c (1)
1921-1937: Nit: redundant ENV guard in container-value rejection.The
SECTION_ENVbranch and the!= SECTION_IGNORABLEbranch both fall through to the sameyaml_error_event+return YAML_FAILURE. SinceSECTION_ENV != SECTION_IGNORABLE, the second condition already covers ENV; the two checks can be collapsed into one and would read more clearly.♻️ Proposed refactor
case YAML_MAPPING_START_EVENT: case YAML_SEQUENCE_START_EVENT: - if (state->section == SECTION_ENV) { - yaml_error_event(ctx, state, event); - return YAML_FAILURE; - } if (state->section != SECTION_IGNORABLE) { yaml_error_event(ctx, state, event); return YAML_FAILURE; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config_format/flb_cf_yaml.c` around lines 1921 - 1937, Remove the redundant SECTION_ENV guard in the YAML_MAPPING_START_EVENT/YAML_SEQUENCE_START_EVENT branch: replace the two checks that call yaml_error_event(ctx, state, event) and return YAML_FAILURE with a single check that tests if state->section != SECTION_IGNORABLE, then calls yaml_error_event(ctx, state, event) and returns YAML_FAILURE; keep the subsequent call to state_push_variant(...) and the existing error handling (flb_error and return YAML_FAILURE) untouched; look for the conditional block handling YAML_MAPPING_START_EVENT and YAML_SEQUENCE_START_EVENT and update it accordingly (symbols: YAML_MAPPING_START_EVENT, YAML_SEQUENCE_START_EVENT, state->section, SECTION_ENV, SECTION_IGNORABLE, yaml_error_event, state_push_variant).
🤖 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/config_format/flb_cf_yaml.c`:
- Around line 1921-1937: Remove the redundant SECTION_ENV guard in the
YAML_MAPPING_START_EVENT/YAML_SEQUENCE_START_EVENT branch: replace the two
checks that call yaml_error_event(ctx, state, event) and return YAML_FAILURE
with a single check that tests if state->section != SECTION_IGNORABLE, then
calls yaml_error_event(ctx, state, event) and returns YAML_FAILURE; keep the
subsequent call to state_push_variant(...) and the existing error handling
(flb_error and return YAML_FAILURE) untouched; look for the conditional block
handling YAML_MAPPING_START_EVENT and YAML_SEQUENCE_START_EVENT and update it
accordingly (symbols: YAML_MAPPING_START_EVENT, YAML_SEQUENCE_START_EVENT,
state->section, SECTION_ENV, SECTION_IGNORABLE, yaml_error_event,
state_push_variant).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fbb3451-b73f-4bf4-b126-a839d9871bce
📒 Files selected for processing (5)
src/config_format/flb_cf_yaml.ctests/internal/config_format_yaml.ctests/internal/data/config_format/yaml/ignorable.yamltests/internal/data/config_format/yaml/other_with_nested_map.yamltests/internal/data/config_format/yaml/processors.yaml
✅ Files skipped from review due to trivial changes (2)
- tests/internal/data/config_format/yaml/other_with_nested_map.yaml
- tests/internal/data/config_format/yaml/ignorable.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/internal/data/config_format/yaml/processors.yaml
- tests/internal/config_format_yaml.c
2 participants
Closes #11683.
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
Release Notes
ignorablesection in YAML configuration files