config_format: yaml: handle configuration metadata on yaml conf#11690
config_format: yaml: handle configuration metadata on yaml conf#11690
Conversation
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a top-level 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".
src/config_format/flb_cf_yaml.c
Outdated
| return YAML_FAILURE; | ||
| } | ||
|
|
||
| if (cfl_kvlist_insert(state->cf_section->properties, state->key, variant) < 0) { |
There was a problem hiding this comment.
When a metadata value is a map/array, this branch inserts it with raw state->key via cfl_kvlist_insert, while scalar metadata values still go through flb_cf_section_property_add (which applies the usual key normalization/translation). That makes key handling depend on value type: e.g., a camelCase key with a scalar is normalized, but the same key with a nested object is not, so lookups and downstream handling become inconsistent for mixed metadata shapes.
Useful? React with 👍 / 👎.
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
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
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
aaef92c to
e56d7b5
Compare
1 participant
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
New Features
Bug Fixes / Validation
Tests