feat: add Event.list() for listing existing events#2686
Open
threcc wants to merge 1 commit intoRedHatQE:mainfrom
Open
feat: add Event.list() for listing existing events#2686threcc wants to merge 1 commit intoRedHatQE:mainfrom
threcc wants to merge 1 commit intoRedHatQE:mainfrom
Conversation
Signed-off-by: threcc <trecchiu@redhat.com>
WalkthroughAdded timestamp parsing and filtering to Event class. Introduced Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
![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 the current code and only fix it if needed.
Inline comments:
In `@ocp_resources/event.py`:
- Line 113: Add a guard to validate the since_seconds parameter (the argument
declared as "since_seconds: int = 300") to ensure it is non‑negative: if
since_seconds < 0, raise a clear ValueError (or similar) with a descriptive
message so the call fails fast; apply this same validation to the other
occurrence of since_seconds in the file (the second declaration referenced in
the review).
- Around line 100-103: The parsed timestamp can be offset-naive and later
compared to the timezone-aware cutoff in list(), causing a TypeError; after
calling datetime.fromisoformat(...) (and after replacing "Z" with "+00:00"),
ensure the resulting datetime is timezone-aware by setting tzinfo to UTC when
dt.tzinfo is None (use datetime.timezone.utc or equivalent) before returning it
from the parsing helper; this guarantees safe comparisons against the
timezone-aware cutoff used in list().
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1d2563f8-02a3-4e54-a961-89d475c4509c
📒 Files selected for processing (1)
ocp_resources/event.py
Comment on lines
+100
to
+103
| try: | ||
| return datetime.fromisoformat(timestamp.replace("Z", "+00:00")) | ||
| except (ValueError, TypeError): | ||
| LOGGER.debug(f"Failed to parse event timestamp: {timestamp}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifies Python datetime behavior that can trigger the runtime error path.
python - <<'PY'
from datetime import datetime, timezone
samples = ["2026-04-03T12:00:00", "2026-04-03T12:00:00Z"]
cutoff = datetime.now(timezone.utc)
for ts in samples:
parsed = datetime.fromisoformat(ts.replace("Z", "+00:00"))
print(f"{ts} -> tzinfo={parsed.tzinfo}")
try:
print("comparison_ok:", parsed >= cutoff)
except Exception as exc:
print("comparison_error:", type(exc).__name__, str(exc))
PYRepository: RedHatQE/openshift-python-wrapper
Length of output: 249
🏁 Script executed:
# Locate and examine the event.py file
git ls-files | grep -E "event\.py$"Repository: RedHatQE/openshift-python-wrapper
Length of output: 99
🏁 Script executed:
# Read the file to understand the full context of _parse_timestamp and list methods
cat -n ocp_resources/event.py | head -220Repository: RedHatQE/openshift-python-wrapper
Length of output: 10161
🏁 Script executed:
# Check for generated-code markers per coding guidelines
grep -n "Generated using\|End of generated code" ocp_resources/event.pyRepository: RedHatQE/openshift-python-wrapper
Length of output: 59
Normalize parsed timestamps to timezone-aware UTC to prevent comparison error at runtime.
When a timestamp lacks a timezone indicator (e.g., "2026-04-03T12:00:00"), datetime.fromisoformat() returns a naive datetime. Comparing this naive datetime to the timezone-aware cutoff at line 163 raises TypeError: can't compare offset-naive and offset-aware datetimes, silently bypassing the try-except block at line 102 and crashing the list() method.
Proposed fix
try:
- return datetime.fromisoformat(timestamp.replace("Z", "+00:00"))
+ parsed = datetime.fromisoformat(timestamp.replace("Z", "+00:00"))
+ if parsed.tzinfo is None:
+ parsed = parsed.replace(tzinfo=timezone.utc)
+ return parsed.astimezone(timezone.utc)
except (ValueError, TypeError):
LOGGER.debug(f"Failed to parse event timestamp: {timestamp}")
return NoneAlso applies to: line 163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ocp_resources/event.py` around lines 100 - 103, The parsed timestamp can be
offset-naive and later compared to the timezone-aware cutoff in list(), causing
a TypeError; after calling datetime.fromisoformat(...) (and after replacing "Z"
with "+00:00"), ensure the resulting datetime is timezone-aware by setting
tzinfo to UTC when dt.tzinfo is None (use datetime.timezone.utc or equivalent)
before returning it from the parsing helper; this guarantees safe comparisons
against the timezone-aware cutoff used in list().
| namespace: str | None = None, | ||
| field_selector: str | None = None, | ||
| label_selector: str | None = None, | ||
| since_seconds: int = 300, |
There was a problem hiding this comment.
Validate since_seconds lower bound.
Negative values currently produce a future cutoff and unintuitive filtering. Add a guard to fail fast.
💡 Proposed fix
def list(
cls,
client: DynamicClient,
namespace: str | None = None,
field_selector: str | None = None,
label_selector: str | None = None,
since_seconds: int = 300,
) -> list[Any]:
+ if since_seconds < 0:
+ raise ValueError("since_seconds must be >= 0")Also applies to: 159-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ocp_resources/event.py` at line 113, Add a guard to validate the
since_seconds parameter (the argument declared as "since_seconds: int = 300") to
ensure it is non‑negative: if since_seconds < 0, raise a clear ValueError (or
similar) with a descriptive message so the call fails fast; apply this same
validation to the other occurrence of since_seconds in the file (the second
declaration referenced in the review).
2 tasks
Collaborator
|
@threcc Please address coderabiitai comments. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Short description:
Add
Event.list()classmethod that performs a standard K8s API list call instead of usingwatch()likeEvent.get().This returns existing events immediately without blocking.
More details:
_parse_timestamp()helper handleslastTimestamp→creationTimestampfallback with error handling for malformed timestampssince_secondsparameter (default: 300 = 5 minutes) filters events by time windowWhat this PR does / why we need it:
Event.get()useswatch()internally that is a streaming API that only yields events occurring after the watch starts, and completes when timeout expires.Past events are not returned.
Event.list()performs a standard K8s list API call that returns immediately with events from the specified time window (default: last 5 minutes).Suitable for diagnostic scenarios like collecting warning events after a resource timeout.
n/a
Special notes for reviewer:
n/a
Bug:
n/a
Summary by CodeRabbit