feat: add events viewer for cluster events#3239
feat: add events viewer for cluster events#3239eggfoobar wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: eggfoobar The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughAdds backend logic and an HTTP endpoint to fetch and normalize Kubernetes events.json for a job run from GCS, plus a new frontend EventsChart component, route, and link to view those events. Changes
Sequence DiagramsequenceDiagram
actor User
participant Frontend as Frontend (React)
participant Server as Backend (HTTP)
participant DB as Database
participant GCS as GCS Storage
User->>Frontend: Navigate to Events page
Frontend->>Server: GET /api/jobs/runs/events (jobRunID, jobName, repoInfo, pullNumber)
Server->>DB: Fetch job run record (artifact path / URL override)
DB-->>Server: Job run entry (may include GCS path / URL override)
Server->>GCS: Locate artifact via regex & fetch events.json
GCS-->>Server: Return events.json
Server->>Server: Unmarshal JSON, flatten to KubeEvent[], build EventListResponse
Server-->>Frontend: 200 OK + EventListResponse
Frontend->>Frontend: Apply filters/sorting and render table
Frontend-->>User: Display interactive events table
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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
🤖 Fix all issues with AI agents
In `@sippy-ng/src/prow_job_runs/EventsChart.js`:
- Around line 297-301: The current render uses <LaunderedLink
address={jobRunUrl}> which throws when jobRunUrl is empty; update EventsChart.js
to guard that usage by checking jobRunUrl before rendering LaunderedLink (e.g.,
if jobRunUrl is falsy render plain text "GCS job run" or a non-link fallback
inside the same Typography), so the code path that references LaunderedLink only
runs when jobRunUrl is a non-empty, valid URL; modify the JSX around the
Typography block that currently uses jobRunUrl to conditionally render the link
component.
In `@sippy-ng/src/prow_job_runs/IntervalsChart.js`:
- Around line 572-577: Move the useParams() call out of the render-after-returns
zone and to the top of the IntervalsChart component alongside the other hooks
(i.e., where other useState/useEffect calls live), so hooks are invoked
unconditionally; then either compute eventsPath after the early-return checks
inside the render body or compute it at the top using a useMemo that depends on
the params (keep references to useParams and eventsPath intact), ensuring hook
order doesn’t change between renders.
🧹 Nitpick comments (3)
pkg/api/jobrunevents/job_run_events.go (1)
57-76: Consider accepting a context parameter for cancellation support.The function uses
context.TODO()on line 92 when callinggcsJobRun.GetContent(). For consistency with other API handlers and to support request cancellation/timeouts, consider accepting acontext.Contextparameter.♻️ Proposed signature change
-func JobRunEvents(gcsClient *storage.Client, dbc *db.DB, jobRunID int64, gcsBucket, gcsPath string, logger *log.Entry) (*EventListResponse, error) { +func JobRunEvents(ctx context.Context, gcsClient *storage.Client, dbc *db.DB, jobRunID int64, gcsBucket, gcsPath string, logger *log.Entry) (*EventListResponse, error) {Then update line 92:
- content, err := gcsJobRun.GetContent(context.TODO(), eventsPath) + content, err := gcsJobRun.GetContent(ctx, eventsPath)pkg/sippyserver/server.go (1)
1428-1475: Consider extracting duplicated GCS path construction logic.The GCS path construction logic (lines 1454-1465) is duplicated from
jsonJobRunIntervals(lines 1399-1417). Consider extracting this into a shared helper function to reduce duplication and ensure consistent behavior.♻️ Example helper extraction
// buildJobRunGCSPath constructs a GCS path from job run parameters func buildJobRunGCSPath(jobRunIDStr, jobName, repoInfo, pullNumber string) string { if len(jobName) == 0 { return "" } if len(repoInfo) > 0 { if repoInfo == "openshift_origin" { return fmt.Sprintf("pr-logs/pull/%s/%s/%s", pullNumber, jobName, jobRunIDStr) } return fmt.Sprintf("pr-logs/pull/%s/%s/%s/%s", repoInfo, pullNumber, jobName, jobRunIDStr) } return fmt.Sprintf("logs/%s/%s", jobName, jobRunIDStr) }sippy-ng/src/prow_job_runs/EventsChart.js (1)
84-88: Redundant parameter extraction from bothuseParams()and props.The component extracts route parameters via both
useParams()(line 86) andprops(line 87). The props come from theEventsChartWrapperinApp.jswhich itself usesuseParams(). This creates confusion about which source should be used.Consider using only props (which are passed by the wrapper) and removing the
useParams()call, or vice versa. Currently,useParamsvalues are used for the intervals link (line 292) while props are used for API calls (lines 111-114).♻️ Proposed simplification - use only props
export default function EventsChart(props) { const classes = useStyles() - const { jobrunid, jobname, repoinfo, pullnumber } = useParams() const { jobRunID, jobName, repoInfo, pullNumber } = propsThen update line 292:
- to={buildIntervalsPath(jobrunid, jobname, repoinfo, pullnumber)} + to={buildIntervalsPath(jobRunID, jobName, repoInfo, pullNumber)}And update
buildIntervalsPathto handle the prop names (they use different casing).
eggfoobar
force-pushed
the
add-cluster-events-html
branch
from
f562700 to
fb423ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@sippy-ng/src/prow_job_runs/EventsChart.js`:
- Around line 86-115: The component inconsistently uses route params
(useParams() / jobrunid, jobname, repoinfo, pullnumber) for links while using
props (jobRunID, jobName, repoInfo, pullNumber) for fetching, causing undefined
links when props are provided; fix by computing single “effective” values (e.g.,
const effectiveJobRunID = jobRunID || jobrunid, effectiveJobName = jobName ||
jobname, etc.) once at top of EventsChart and then use those effective*
variables everywhere—including inside fetchData URL construction and when
building the Intervals link—so both fetch and link paths come from the same
source (reference symbols: useParams, jobrunid, jobname, repoinfo, pullnumber,
jobRunID, jobName, repoInfo, pullNumber, fetchData).
|
Scheduling required tests: |
added ability to consume the events.json collected from gather-extra of a job run Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: ehila <ehila@redhat.com>
eggfoobar
force-pushed
the
add-cluster-events-html
branch
from
fb423ef to
e991678
Compare
|
Scheduling required tests: |
|
/cc |
|
Whoah this looks cool. Are the filters linkable in bugs and such? |
|
Oh, that's such a great idea, let me add that in, it's super useful! |
added support to link specific event filters Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: ehila <ehila@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@sippy-ng/src/prow_job_runs/EventsChart.js`:
- Around line 166-173: The error/loading messages currently interpolate jobRunID
and jobName which can be undefined when route params are used; update message
construction to use effectiveJobRunID and effectiveJobName instead. Locate calls
to setFetchError and any loading/error messages (e.g., the setFetchError call
shown and the messages around lines 304-306) and replace jobRunID/jobName with
effectiveJobRunID/effectiveJobName so the displayed text matches the actual
fetch parameters; ensure any setIsLoading or other status messages use the same
effective* variables for consistency.
- Around line 274-291: clearFilters currently only resets timeFrom/timeTo when
allEvents has timestamps, leaving stale values when allEvents is empty; update
clearFilters (function) to explicitly clear the time range in the else case by
calling setTimeFrom and setTimeTo with the “cleared” value your component
expects (e.g. empty string or null) so time filters are reset even when
allEvents.length === 0; ensure you still compute min/max and call
formatDateTimeLocalUTC(min/max) when timestamps exist.
|
Scheduling required tests: |
|
@eggfoobar: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
added ability to consume the events.json collected from gather-extra of a job run
Summary by CodeRabbit