fix: preserve subject and source on GenericChange findings - #2129
fix: preserve subject and source on GenericChange findings#2129Sanjays2402 wants to merge 2 commits into
Conversation
K8sBaseChangeEvent.create_default_finding() built a Finding with only a title and aggregation_key, so finding.subject stayed at the ExecutionBaseEvent default (name "Unresolved", namespace None) and source stayed NONE. Sink routing and Slack grouping read finding.attribute_map["namespace"], which derives from finding.subject.namespace, so Kubernetes change notifications were routed as namespace "None" instead of the resource's actual namespace - sending production changes to a fallback sink. The event already exposes get_subject() and get_source(); pass them through, matching how PrometheusKubernetesAlert.create_default_finding() already does. Adds a regression test asserting the default finding keeps the resource's namespace, name and KUBERNETES_API_SERVER source.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. WalkthroughKubernetes default findings now include the event subject and source. Tests cover aggregation key, namespace/name preservation, Kubernetes API server source, and corresponding attribute map values. ChangesKubernetes finding metadata
Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Kubernetes GenericChange findings now retain resource subject metadata and Kubernetes API-server source, preserving namespace-aware routing and grouping. Regression coverage accompanies the change, with no remaining merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/test_change_filters.py`:
- Around line 99-108: Extend test_default_finding_keeps_subject_and_source to
assert that finding.subject.labels and finding.subject.annotations match the
corresponding labels and annotations in the event fixture metadata, alongside
the existing namespace and name checks.
- Line 17: Remove the unused DEFAULT_CHANGE_INCLUDE import from the imports in
tests/test_change_filters.py, leaving the required DEFAULT_CHANGE_IGNORE import
and other dependencies unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ee27cd95-e523-495c-918d-219feaa8db02
📒 Files selected for processing (2)
src/robusta/integrations/kubernetes/base_event.pytests/test_change_filters.py
|
Thanks for the review — both points addressed in 312869f:
|
Closes #2044
K8sBaseChangeEvent.create_default_finding()built aFindingwith only a title andaggregation_key, sofinding.subjectstayed at theExecutionBaseEventdefault (namespaceNone) andsourcestayedNONE. Sink routing readsfinding.attribute_map["namespace"], which derives fromsubject.namespace, so Kubernetes change notifications were routed as namespace"None"rather than the resource's real namespace.The event already exposes
get_subject()/get_source()— this passes them through, matching whatPrometheusKubernetesAlert.create_default_finding()already does. The new test intests/test_change_filters.pyfails onmaster(assert None == 'default') and passes with the fix.