Patch empty on_pod_conditions, to avoid Hikaru schema validation failure #1909
Conversation
WalkthroughAdds EmptyFileBlock handling across multiple chat/integration senders to skip empty attachments without warnings. Introduces Kubernetes patching to ensure Job/RobustaJob PodFailurePolicy rules have non-null onPodConditions lists. Adds a runtime setter patch for V1PodFailurePolicyRule to coerce None to [] and fixes a log message typo. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor KubeEvent as K8s Event
participant Trigger as K8sBaseTrigger
participant Patch as __patch_k8s_object
participant Hikaru as hikaru.from_dict
KubeEvent->>Trigger: __load_hikaru_obj(obj)
Note over Trigger: metadata.managedFields = None
Trigger->>Patch: apply resource-specific patch
alt model is Job/RobustaJob
Patch->>Patch: Ensure spec.podFailurePolicy.rules[*].onPodConditions = []
else other models
Patch-->>Trigger: No changes
end
Trigger->>Hikaru: from_dict(patched obj)
HIkaru-->>Trigger: Hikaru model
Trigger-->>KubeEvent: Processed object
sequenceDiagram
autonumber
actor Report as Report Builder
participant Sender as Integration Sender
participant Platform as Chat Platform
Report->>Sender: convert(block)
alt block is EmptyFileBlock
Sender-->>Report: skip ([], "", or None)
else supported block type
Sender->>Platform: send formatted content
Platform-->>Sender: ack
else unsupported block type
Sender-->>Report: warn/fallback (unchanged)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/robusta/patch/patch.py (1)
54-54: Clarify log to reflect both setter and init patches (nit)Minor: the message mentions only the property; it now patches both setter and init.
Apply:
-logging.debug("Creating kubernetes PodFailurePolicyRule.on_pod_conditions monkey patch") +logging.debug("Patching kubernetes PodFailurePolicyRule: on_pod_conditions setter and __init__ (None → [])")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/robusta/patch/patch.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_tests
- GitHub Check: run_tests
- GitHub Check: Deploy docs
🔇 Additional comments (1)
src/robusta/patch/patch.py (1)
61-70: Confirm no positional instantiations of V1PodFailurePolicyRule exist. I ranrg -nP 'V1PodFailurePolicyRule\s*\('and found no matches; please verify that no other call sites rely on positional args so the inspect-based monkey-patch covers all uses.
remove EmptyBlocks warning messages
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/robusta/integrations/mattermost/sender.py (1)
76-77: Skip EmptyFileBlock without emitting empty strings.Returning "" can create stray blank lines in the final Mattermost message. Prefer filtering out falsy conversions when building output_blocks.
Apply outside-this-hunk change in __send_blocks_to_mattermost:
- for block in other_blocks: - output_blocks.append(self.__to_mattermost(block, self.sink_params.name)) + for block in other_blocks: + rendered = self.__to_mattermost(block, self.sink_params.name) + if rendered: # skip empty blocks + output_blocks.append(rendered)src/robusta/core/sinks/robusta/dal/model_conversion.py (2)
201-203: Duplicate handling of EmptyFileBlock; remove dead branch and align behavior.EmptyFileBlock is both appended (as an object) and later skipped, making the latter unreachable. Drop the duplicate and pick one behavior to avoid confusion.
Minimal fix (remove the unreachable later check):
- elif isinstance(block, EmptyFileBlock): - continue # skip empty blockIf the intent is to skip EmptyFileBlock entirely (to match chat integrations), instead remove the earlier append:
- elif isinstance(block, EmptyFileBlock): - structured_data.append(ModelConversion.get_empty_file_object(block)) + elif isinstance(block, EmptyFileBlock): + continue # skip empty blockAlso applies to: 278-279
82-88: Confirm consumer expectations for empty-file evidence shape.You emit:
- type: file extension
- metadata: as-is (with is_empty/remarks)
- data: ""
Ensure downstream consumers expect data == "" (not None) and use metadata.is_empty. If filenames may lack an extension, consider hardening get_file_type.
Proposed hardened helper (outside this hunk):
- def get_file_type(filename: str): - last_dot_idx = filename.rindex(".") - return filename[last_dot_idx + 1 :] + def get_file_type(filename: str): + idx = filename.rfind(".") + return filename[idx + 1 :] if idx != -1 else ""src/robusta/integrations/jira/sender.py (1)
6-6: Import EmptyFileBlock from blocks to avoid implicit re-exportsSafer and consistent with other files. Prevents breakage if
robusta.core.reporting.__init__stops re-exporting.-from robusta.core.reporting import EmptyFileBlock +from robusta.core.reporting.blocks import EmptyFileBlocksrc/robusta/integrations/kubernetes/base_triggers.py (1)
192-208: Also coerce onPodConditions=None to [] for completenessSome payloads may include
"onPodConditions": null. Handling both missing and null here makes the patch robust even if the Hikaru setter patch isn’t applied early.- for rule in rules: - if isinstance(rule, dict) and "onPodConditions" not in rule: - rule["onPodConditions"] = [] + for rule in rules: + if isinstance(rule, dict) and ( + "onPodConditions" not in rule or rule.get("onPodConditions") is None + ): + rule["onPodConditions"] = []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
src/robusta/core/sinks/robusta/dal/model_conversion.py(1 hunks)src/robusta/integrations/discord/sender.py(2 hunks)src/robusta/integrations/google_chat/sender.py(2 hunks)src/robusta/integrations/jira/sender.py(2 hunks)src/robusta/integrations/kubernetes/base_triggers.py(1 hunks)src/robusta/integrations/mattermost/sender.py(2 hunks)src/robusta/integrations/msteams/sender.py(2 hunks)src/robusta/integrations/rocketchat/sender.py(2 hunks)src/robusta/integrations/slack/sender.py(2 hunks)src/robusta/integrations/zulip/sender.py(2 hunks)src/robusta/patch/patch.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/robusta/patch/patch.py
🧰 Additional context used
🧬 Code graph analysis (9)
src/robusta/integrations/msteams/sender.py (1)
src/robusta/core/reporting/blocks.py (1)
EmptyFileBlock(136-162)
src/robusta/integrations/google_chat/sender.py (1)
src/robusta/core/reporting/blocks.py (2)
FileBlock(72-133)EmptyFileBlock(136-162)
src/robusta/integrations/jira/sender.py (1)
src/robusta/core/reporting/blocks.py (1)
EmptyFileBlock(136-162)
src/robusta/integrations/mattermost/sender.py (1)
src/robusta/core/reporting/blocks.py (1)
EmptyFileBlock(136-162)
src/robusta/integrations/discord/sender.py (1)
src/robusta/core/reporting/blocks.py (1)
EmptyFileBlock(136-162)
src/robusta/integrations/slack/sender.py (1)
src/robusta/core/reporting/blocks.py (1)
EmptyFileBlock(136-162)
src/robusta/integrations/rocketchat/sender.py (1)
src/robusta/core/reporting/blocks.py (2)
TableBlock(321-448)EmptyFileBlock(136-162)
src/robusta/core/sinks/robusta/dal/model_conversion.py (1)
src/robusta/core/reporting/blocks.py (1)
EmptyFileBlock(136-162)
src/robusta/integrations/zulip/sender.py (1)
src/robusta/core/reporting/blocks.py (2)
FileBlock(72-133)EmptyFileBlock(136-162)
🪛 Ruff (0.12.2)
src/robusta/integrations/zulip/sender.py
16-16: Redefinition of unused FileBlock from line 8
(F811)
🪛 Flake8 (7.2.0)
src/robusta/integrations/zulip/sender.py
[error] 16-16: redefinition of unused 'FileBlock' from line 6
(F811)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (13)
src/robusta/integrations/mattermost/sender.py (1)
15-16: Import addition looks good.EmptyFileBlock is correctly imported alongside TableBlock.
src/robusta/integrations/msteams/sender.py (2)
39-40: LGTM: silently ignore EmptyFileBlock.Not logging a warning for EmptyFileBlock keeps logs clean while skipping the block.
3-3: Import from robusta.core.reporting is correct
EmptyFileBlock is re-exported in robusta/core/reporting/init.py, so the existing import is valid.Likely an incorrect or invalid review comment.
src/robusta/integrations/slack/sender.py (2)
40-41: Import addition looks good.EmptyFileBlock included with other blocks; consistent with usage.
231-232: LGTM: safely skip EmptyFileBlock.Returning [] avoids warnings and matches Slack’s constraints on empty uploads.
src/robusta/integrations/discord/sender.py (1)
190-191: LGTM: skip EmptyFileBlock.Cleanly ignored in Discord output.
src/robusta/integrations/jira/sender.py (1)
156-157: Skip EmptyFileBlock — LGTMConsistent with other sinks; avoids noisy warnings.
src/robusta/integrations/zulip/sender.py (1)
125-126: Skip EmptyFileBlock without warning — LGTMKeeps logs clean while preserving warnings for unknown block types.
src/robusta/integrations/google_chat/sender.py (2)
8-9: Import addition — LGTMAdding
EmptyFileBlockto imports aligns with new handling.
100-101: Skip EmptyFileBlock — LGTMReturning
Noneto omit rendering is correct for Chat’s webhook limitations.src/robusta/integrations/kubernetes/base_triggers.py (1)
215-217: Patch injection point — LGTMRunning the patch just before
hikaru.from_dictis the right place; oldObj is handled via the same path below.src/robusta/integrations/rocketchat/sender.py (2)
21-22: Import addition — LGTMBrings
EmptyFileBlockinto scope for the new branch below.
170-171: Skip EmptyFileBlock — LGTMPrevents empty-log “attachments” and avoids warning noise.
Avi-Robusta
left a comment
Avi-Robusta
left a comment
There was a problem hiding this comment.
LGTM,
Mostly stylistic comments you can ignore if you are in a rush,
Check UI with the get logs button in the app page and the job page when there are no logs, just to verify that this PR doesn't break anything in the UI and it doesnt wait forever.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/robusta/integrations/mattermost/sender.py (1)
87-91: Fix character-wise join bug in __to_mattermost_diffchain() over strings iterates characters; results in one char per line. Collect strings and join.
Apply:
- _blocks = list( - chain(*[self.__to_mattermost(transformed_block, sink_name) for transformed_block in transformed_blocks]) - ) - - return "\n".join(_blocks) + _blocks = [ + b + for b in (self.__to_mattermost(tb, sink_name) for tb in transformed_blocks) + if isinstance(b, str) and b + ] + return "\n".join(_blocks)
🧹 Nitpick comments (4)
src/robusta/integrations/mattermost/sender.py (2)
121-125: Filter empty strings to avoid blank lines in Mattermost messageEmpty blocks currently add superfluous newlines. Filter before formatting.
- for block in other_blocks: - output_blocks.append(self.__to_mattermost(block, self.sink_params.name)) + for block in other_blocks: + rendered = self.__to_mattermost(block, self.sink_params.name) + if isinstance(rendered, str) and rendered: + output_blocks.append(rendered) - attachments = self.__format_msg_attachments(output_blocks, msg_color) + attachments = self.__format_msg_attachments(output_blocks, msg_color)
115-121: Minor: header_block default should be a string, not dictpost_message likely expects a str; use "" when no title.
- header_block = {} + header_block = "" if title: title = self.__add_mattermost_title( title=title, status=status, severity=severity, add_silence_url=add_silence_url ) header_block = self.__to_mattermost(HeaderBlock(title), self.sink_params.name)src/robusta/integrations/msteams/sender.py (1)
39-40: Prefer explicit branch for EmptyFileBlockEarly-return reads clearer and avoids nesting.
- if not isinstance(block, EmptyFileBlock): - logging.warning(f"cannot convert block of type {type(block)} to msteams format block: {block}") + if isinstance(block, EmptyFileBlock): + return + logging.warning(f"cannot convert block of type {type(block)} to msteams format block: {block}")src/robusta/integrations/zulip/sender.py (1)
108-110: Remove stray closing parenthesis in K8s diff lineMessage currently ends with an extra “)”.
- yield f"{self.__to_zulip_bold('.'.join(d.path))}: {d.other_value} ➡️ {d.value})" + yield f"{self.__to_zulip_bold('.'.join(d.path))}: {d.other_value} ➡️ {d.value}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/robusta/integrations/discord/sender.py(2 hunks)src/robusta/integrations/jira/sender.py(2 hunks)src/robusta/integrations/mattermost/sender.py(2 hunks)src/robusta/integrations/msteams/sender.py(2 hunks)src/robusta/integrations/rocketchat/sender.py(2 hunks)src/robusta/integrations/slack/sender.py(2 hunks)src/robusta/integrations/zulip/sender.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/robusta/integrations/jira/sender.py
- src/robusta/integrations/slack/sender.py
- src/robusta/integrations/discord/sender.py
- src/robusta/integrations/rocketchat/sender.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/robusta/integrations/zulip/sender.py (1)
src/robusta/core/reporting/blocks.py (1)
EmptyFileBlock(136-162)
🪛 Flake8 (7.2.0)
src/robusta/integrations/zulip/sender.py
[error] 16-16: 'robusta.core.reporting.base.LinkType' imported but unused
(F401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (5)
src/robusta/integrations/mattermost/sender.py (2)
16-17: LGTM: import EmptyFileBlockConsistent with other sinks.
77-78: Skip EmptyFileBlock without noiseGood call; avoids noisy logs.
src/robusta/integrations/msteams/sender.py (1)
15-16: LGTM: import EmptyFileBlockMatches behavior across other integrations.
src/robusta/integrations/zulip/sender.py (2)
14-15: LGTM: handle EmptyFileBlockSilent skip is consistent with other sinks.
126-127: LGTM: don’t warn on EmptyFileBlockKeeps logs clean.
No description provided.