fix: preserve subject and source on GenericChange findings - #2129
fix: preserve subject and source on GenericChange findings#2129Sanjays2402 wants to merge 1 commit 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.
|
|
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 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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
🤖 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
| ) | ||
| from robusta.integrations.kubernetes.base_triggers import ( | ||
| DEFAULT_CHANGE_FILTERS, | ||
| DEFAULT_CHANGE_IGNORE, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unused DEFAULT_CHANGE_INCLUDE import.
Flake8 reports this import as unused (F401), so it should be removed before merge.
Proposed fix
from robusta.integrations.kubernetes.base_triggers import (
DEFAULT_CHANGE_FILTERS,
DEFAULT_CHANGE_IGNORE,
- DEFAULT_CHANGE_INCLUDE,
K8sBaseTrigger,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DEFAULT_CHANGE_IGNORE, | |
| from robusta.integrations.kubernetes.base_triggers import ( | |
| DEFAULT_CHANGE_FILTERS, | |
| DEFAULT_CHANGE_IGNORE, | |
| K8sBaseTrigger, |
🤖 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/test_change_filters.py` at 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.
Source: Linters/SAST tools
| def test_default_finding_keeps_subject_and_source(self, event): | ||
| finding = event.create_default_finding() | ||
|
|
||
| assert finding.aggregation_key == "GenericChange" | ||
| assert finding.subject.namespace == "default" | ||
| assert finding.subject.name == "xxx-deployment" | ||
| assert finding.source == FindingSource.KUBERNETES_API_SERVER | ||
| # sink routing/grouping reads these, so they must not fall back to "None" | ||
| assert finding.attribute_map["namespace"] == "default" | ||
| assert finding.attribute_map["name"] == "xxx-deployment" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Cover labels and annotations in the regression test.
The stated contract includes preserving subject.labels and subject.annotations, but this test only verifies namespace and name. Add assertions for both fields against the fixture’s metadata so regressions in the complete subject are detected.
🤖 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/test_change_filters.py` around lines 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.
2 participants
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.