add(RHIDP-15096): add flag skip_metadata_assert to skip values assertion logic from ES histroy data - #175
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds an optional Changesskip_metadata_assert flag propagation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant PassOrFail as pass_or_fail.doit
participant Loader as elasticsearch_loader.load
participant StatusData as StatusData.__init__
PassOrFail->>Loader: Pass skip_metadata_assert from args
Loader->>Loader: Read skip_metadata_assert from kwargs
loop each Elasticsearch hit
Loader->>StatusData: Construct with skip_metadata_assert
alt skip_metadata_assert is false
StatusData->>StatusData: Assert required metadata keys
else skip_metadata_assert is true
StatusData->>StatusData: Skip metadata assertions
end
end
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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 |
5033064 to
fdb2216
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/opl/status_data.py (1)
28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the new
skip_metadata_assertbypass path.Logic is correct, but there's no test verifying that assertions are actually skipped when
skip_metadata_assert=Trueand still enforced by default. This guards a validation path that other code (elasticsearch_loader) now relies on being correctly bypassable.As per path instructions, focusing on maintainability impact of untested new behavior.
Also applies to: 46-50
🤖 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 `@core/opl/status_data.py` at line 28, Add tests for the new skip_metadata_assert bypass in StatusData.__init__ to cover both behaviors: when skip_metadata_assert=True, metadata validation assertions should be skipped, and when omitted or false, the existing assertions should still run. Update or add a focused test around StatusData and any caller path used by elasticsearch_loader so the bypass is exercised through the new parameter and the default enforcement remains covered.Source: Path instructions
🤖 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 `@core/opl/status_data.py`:
- Line 28: Add tests for the new skip_metadata_assert bypass in
StatusData.__init__ to cover both behaviors: when skip_metadata_assert=True,
metadata validation assertions should be skipped, and when omitted or false, the
existing assertions should still run. Update or add a focused test around
StatusData and any caller path used by elasticsearch_loader so the bypass is
exercised through the new parameter and the default enforcement remains covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 361af50e-a4ba-4bbd-9fc0-3642888d4281
📒 Files selected for processing (6)
core/opl/investigator/README.mdcore/opl/investigator/config.pycore/opl/investigator/elasticsearch_loader.pycore/opl/investigator/sample_config.yamlcore/opl/pass_or_fail.pycore/opl/status_data.py
fdb2216 to
b43a5f4
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/opl/investigator/elasticsearch_loader.py (1)
42-45: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winPR objective not met:
KeyErroron missingparametersstill present.The PR summary states this PR fixes the
KeyErrorwhen an Elasticsearch document lacks aparametersfield, but line 44 still accessesitem['_source']['parameters']directly in theif 'run' in item['_source']['parameters']condition. Ifparametersis absent fromitem['_source'], this raisesKeyErrorbefore the ternary can evaluate toNone.The
idfield on the same line uses the safe pattern (if 'id' in item['_source']), butparametersdoes not follow the same approach.🐛 Proposed fix using `.get()` for `parameters`
logging.debug( - f"Loading data from document ID {item['_id']} with field id={item['_source']['id'] if 'id' in item['_source'] else None} or parameters.run={item['_source']['parameters']['run'] if 'run' in item['_source']['parameters'] else None}" + f"Loading data from document ID {item['_id']} with field id={item['_source'].get('id')} or parameters.run={item['_source'].get('parameters', {}).get('run')}" )🤖 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 `@core/opl/investigator/elasticsearch_loader.py` around lines 42 - 45, The Elasticsearch loader still raises KeyError when `parameters` is missing because `elasticsearch_loader.py` in the hit-processing loop accesses `item['_source']['parameters']` directly in the debug log. Update the logging expression in the `for item in response["hits"]["hits"]` block to use the same safe access pattern as the `id` field, such as retrieving `parameters` via `.get()` before checking for `run`, so the message can fall back to None when `parameters` is absent.
🤖 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.
Outside diff comments:
In `@core/opl/investigator/elasticsearch_loader.py`:
- Around line 42-45: The Elasticsearch loader still raises KeyError when
`parameters` is missing because `elasticsearch_loader.py` in the hit-processing
loop accesses `item['_source']['parameters']` directly in the debug log. Update
the logging expression in the `for item in response["hits"]["hits"]` block to
use the same safe access pattern as the `id` field, such as retrieving
`parameters` via `.get()` before checking for `run`, so the message can fall
back to None when `parameters` is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: d4cdf129-7112-4b86-9819-7c4daaffd086
📒 Files selected for processing (6)
core/opl/investigator/README.mdcore/opl/investigator/config.pycore/opl/investigator/elasticsearch_loader.pycore/opl/investigator/sample_config.yamlcore/opl/pass_or_fail.pycore/opl/status_data.py
✅ Files skipped from review due to trivial changes (2)
- core/opl/investigator/README.md
- core/opl/investigator/sample_config.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- core/opl/pass_or_fail.py
- core/opl/investigator/config.py
- core/opl/status_data.py
shashankkestwal
changed the title
shashankkestwal
changed the title
|
add option flag 'skip_metadata_assert' to skip historical field validation Introduce a configurable flag to bypass validation of The existing behaviour remains unchanged unless the new flag is explicitly enabled. |
…n logic from ES histroy data Signed-off-by: skestwal <skestwal@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
b43a5f4 to
d09214d
Compare
jhutar
left a comment
jhutar
left a comment
There was a problem hiding this comment.
Looks perfectly safe, thank you!
2 participants
No description provided.