TRT-2376: labels in sippy UI #3098
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sosiouxme: This pull request references TRT-2376 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Pipeline controller notification For optional jobs, comment |
|
Skipping CI for Draft Pull Request. |
WalkthroughThis change introduces a labels and symptoms system for job runs. It adds database models for labels and symptoms, extends seed data to populate these entities and assign them to runs, adds a backend API endpoint to retrieve labels, enhances autocomplete functionality, and extends the frontend UI to display and interact with labels within job run tables. Changes
Sequence Diagram(s)sequenceDiagram
participant FE as Frontend (JobRunsTable)
participant API as Backend API
participant DB as Database
Note over FE,DB: On Component Mount
FE->>API: GET /api/labels
API->>DB: Query all Labels
DB-->>API: Labels (id, label_title, explanation)
API-->>FE: { labels: [...] }
FE->>FE: Store in allLabels state
Note over FE,DB: On Label Button Click
FE->>FE: Open Dialog with selectedLabels<br/>(from current job run's labels array)
Note over FE: Dialog Render
FE->>FE: Map selectedLabels to allLabels<br/>Render label_title & explanation<br/>(markdown formatted)
FE-->>FE: Display in Dialog with Close button
sequenceDiagram
participant Seed as Seeding Process
participant DB as Database
Note over Seed,DB: Seed Workflow
Seed->>Seed: Create test models & ProwJobRuns
Seed->>Seed: Compute OverallResult per run<br/>(70% TestFailure, 15% Upgrade,<br/>7% Install, 8% Infrastructure)
Seed->>DB: Persist ProwJobRuns with OverallResult
Seed->>Seed: createLabelsAndSymptoms()
Seed->>DB: Upsert predefined Labels<br/>(with display contexts)
Seed->>DB: Upsert predefined Symptoms<br/>(with LabelIDs associations)
Seed->>Seed: applyLabelsToJobRuns()
Seed->>DB: Query all JobRuns & Labels
DB-->>Seed: JobRuns, Labels
Seed->>Seed: Assign 1–3 random labels<br/>to ~25% of runs
Seed->>DB: Persist JobRun.Labels updates
Seed->>Seed: Log seeding results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touchesImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sosiouxme The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@sosiouxme: This pull request references TRT-2376 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@sosiouxme: This pull request references TRT-2376 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@sosiouxme: This pull request references TRT-2376 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/sippyserver/server.go (1)
979-992: Consider pagination for scalability.The endpoint fetches all labels without pagination or limits. While the 24-hour cache mitigates performance concerns, consider adding optional pagination parameters if the number of labels is expected to grow significantly.
sippy-ng/src/jobs/JobRunsTable.js (1)
391-412: Consider surfacing label fetch errors to users.Currently, errors fetching labels are only logged to console (line 410). If label data is critical for the user experience, consider displaying an error message or notification when the fetch fails.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (12)
cmd/sippy/seed_data.go(4 hunks)pkg/api/autocomplete.go(1 hunks)pkg/apis/api/types.go(3 hunks)pkg/db/db.go(2 hunks)pkg/db/models/jobrunscan/label.go(1 hunks)pkg/db/models/jobrunscan/symptom.go(1 hunks)pkg/db/models/prow.go(1 hunks)pkg/db/views.go(1 hunks)pkg/filter/filterable.go(5 hunks)pkg/sippyserver/server.go(3 hunks)sippy-ng/src/datagrid/GridToolbarAutocomplete.js(2 hunks)sippy-ng/src/jobs/JobRunsTable.js(7 hunks)
🔇 Additional comments (11)
pkg/filter/filterable.go (2)
65-78: Helper correctly generates ILIKE SQL, but note usage context.The
ilikeFilterhelper correctly centralizes ILIKE pattern generation for both string and array fields. The negation is baked into the returned SQL string (e.g.,"NOT ILIKE"or"NOT EXISTS"), which is the correct approach for OR filters that build SQL strings.However, this design choice creates a critical issue when used with
applyIlikeFilterfor AND filters (see next comment).
105-105: Refactoring approach is sound—centralizes ILIKE logic effectively.The migration to use
ilikeFilterandapplyIlikeFiltersuccessfully:
- Eliminates code duplication across Contains, StartsWith, and EndsWith operators
- Adds array field support via PostgreSQL
unnestfor pattern matching- Maintains consistency between OR filter (lines 105, 138, 140) and AND filter paths (lines 171, 210, 212)
Once the double negation bug in
applyIlikeFilteris fixed, this refactoring will work correctly.Also applies to: 138-140, 171-171, 210-212
pkg/db/db.go (1)
166-173: LGTM!The migration order is correct, adding the new label and symptom tables before creating audit log indexes.
pkg/apis/api/types.go (1)
358-358: LGTM!The Labels field is correctly added to JobRun with proper JSON and GORM tags, and all accessor methods (GetFieldType, GetArrayValue) are updated to support the new field.
Also applies to: 377-378, 444-445
pkg/db/views.go (1)
208-208: LGTM!The materialized view is correctly extended to project the labels column from prow_job_runs.
pkg/sippyserver/server.go (1)
2001-2007: LGTM!The endpoint is properly configured with LocalDBCapability requirement and appropriate 24-hour cache duration for relatively static label data.
sippy-ng/src/datagrid/GridToolbarAutocomplete.js (1)
30-37: LGTM!The changes properly handle both string arrays (existing behavior) and object arrays (new label options), maintaining backward compatibility while supporting the new title-aware display.
Also applies to: 66-68
pkg/db/models/prow.go (1)
60-62: LGTM!The Labels field is correctly defined with an appropriate GIN index for efficient array containment queries. The comment clearly explains the field's purpose.
pkg/api/autocomplete.go (1)
11-15: LGTM!The LabelOption type correctly defines the structure for label autocomplete with both ID and title fields.
sippy-ng/src/jobs/JobRunsTable.js (2)
156-194: LGTM!The Labels column implementation is well-designed with a tooltip showing label titles and a button that opens a detailed dialog. The rendering handles missing labels gracefully.
567-608: LGTM!The labels dialog provides a clean UI for viewing label details with titles and markdown-rendered explanations. The dialog properly handles cases where labels might not be found.
|
@sosiouxme: This pull request references TRT-2376 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-merge-robot
added
the
needs-rebase
|
@sosiouxme: This pull request references TRT-2376 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
sosiouxme
force-pushed
the
20251105-TRT-2376-sippy-UI-labels
branch
from
d46a554 to
2b1edab
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@sosiouxme: This pull request references TRT-2376 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
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)
pkg/api/autocomplete.go (1)
97-99: Missingreturnafter 404 response in default case.The
defaultcase callsRespondWithJSONwith a 404 status but does not return. Execution continues to the release filter, search filter, limit, and scan operations with an uninitialized/base query, which could produce unexpected behavior or errors.default: RespondWithJSON(404, w, map[string]string{"message": "Autocomplete field not found."}) + return }
🧹 Nitpick comments (5)
pkg/sippyserver/server.go (1)
982-995:/api/labelshandler looks correct; consider adding a deterministic sort.The handler and endpoint wiring are consistent with the rest of the server (capability‑gated, cached, proper error handling). To make UI behavior deterministic and cache keys more stable when labels change infrequently, consider explicitly ordering the query (e.g., by
idorlabel_title) before returning.func (s *Server) jsonLabelsFromDB(w http.ResponseWriter, req *http.Request) { - var labels []jobrunscan.Label - if err := s.db.DB.Find(&labels).Error; err != nil { + var labels []jobrunscan.Label + if err := s.db.DB. + Order("id ASC"). + Find(&labels).Error; err != nil { log.WithError(err).Error("error fetching labels") failureResponse(w, http.StatusInternalServerError, "Error fetching labels: "+err.Error()) return }Also applies to: 2004-2010
cmd/sippy/seed_data.go (1)
298-324: OverallResult seeding logic is fine; consider wiring infra flags for more realistic data.The randomized assignment of
OverallResultbased ontestFailuresandfailureTypelooks correct and guarantees a non‑zero value for each run. For closer fidelity to production data, you might also set the corresponding boolean flags (InfrastructureFailure,KnownFailure, etc.) when assigning specific failure types so downstream dashboards that key off those flags behave consistently in seeded environments.pkg/db/models/jobrunscan/symptom.go (1)
10-64: Symptom model looks solid; only minor naming/doc nits.The Symptom schema (string ID primary key, matcher config, label ID array, filters, and explicit timestamps) is well-structured and aligns with how you seed and will likely query these records. Not embedding
gorm.Modelavoids the duplicate primary‑key problem called out in earlier reviews.One small nit:
MatcherTypeFilebeing backed by the string"none"while comments refer to a “file exists” behavior may be slightly confusing for future readers; consider either renaming the constant or clarifying in the comment that"none"is the historical wire value meaning “file existence only”.sippy-ng/src/helpers.js (1)
210-218: Pre-existing bug:filter.items === []always evaluates to false.In JavaScript, comparing an array to an empty array literal using
===always returnsfalsedue to reference comparison. This should usefilter.items.length === 0instead. Note: this line is not changed by this PR, but if you're modifying this area, consider fixing it.export function pathForJobRunsWithFilter(release, filter) { - if (!filter || filter.items === []) { + if (!filter || filter.items.length === 0) { return `/jobs/${release}/runs` }sippy-ng/src/jobs/JobRunsTable.js (1)
359-380: Consider surfacing label fetch errors to the user.The fetch silently returns an empty object on non-200 status, causing labels to display as IDs rather than titles. While this degradation is acceptable, consider showing a dismissible warning to inform users that label details couldn't be loaded.
Example addition after line 378:
.catch((error) => { console.error('Could not fetch labels:', error) + // Optionally set a state variable to show a warning banner })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (21)
.coderabbit.yaml(1 hunks)cmd/sippy/seed_data.go(4 hunks)pkg/api/autocomplete.go(1 hunks)pkg/api/tests.go(6 hunks)pkg/apis/api/types.go(3 hunks)pkg/db/db.go(2 hunks)pkg/db/models/jobrunscan/label.go(1 hunks)pkg/db/models/jobrunscan/symptom.go(1 hunks)pkg/db/models/prow.go(1 hunks)pkg/db/views.go(1 hunks)pkg/filter/filterable.go(7 hunks)pkg/sippyserver/server.go(3 hunks)sippy-ng/src/constants.js(1 hunks)sippy-ng/src/datagrid/GridToolbarAutocomplete.js(2 hunks)sippy-ng/src/datagrid/GridToolbarFilterItem.js(1 hunks)sippy-ng/src/helpers.js(6 hunks)sippy-ng/src/jobs/JobRunsTable.js(8 hunks)sippy-ng/src/jobs/VariantStatus.js(3 hunks)sippy-ng/src/tests/FeatureGates.js(6 hunks)sippy-ng/src/tests/TestAnalysis.js(1 hunks)sippy-ng/src/tests/TestTable.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/apis/api/types.go
- pkg/db/views.go
- sippy-ng/src/datagrid/GridToolbarAutocomplete.js
- pkg/db/models/jobrunscan/label.go
- pkg/db/db.go
🧰 Additional context used
🧬 Code graph analysis (6)
sippy-ng/src/tests/TestTable.js (1)
sippy-ng/src/helpers.js (1)
filterFor(280-282)
sippy-ng/src/tests/TestAnalysis.js (1)
sippy-ng/src/helpers.js (2)
not(306-309)filterFor(280-282)
sippy-ng/src/jobs/VariantStatus.js (1)
sippy-ng/src/helpers.js (1)
filterFor(280-282)
pkg/api/autocomplete.go (2)
pkg/db/db.go (1)
DB(27-33)vendor/github.com/gorilla/mux/mux.go (1)
Vars(430-435)
pkg/filter/filterable.go (1)
pkg/apis/api/types.go (2)
ColumnTypeArray(24-24)ColumnTypeTimestamp(25-25)
pkg/db/models/jobrunscan/symptom.go (1)
vendor/github.com/invopop/jsonschema/id.go (1)
ID(12-12)
🔇 Additional comments (33)
.coderabbit.yaml (1)
10-14: Good reorganization of tool configuration.Moving linting tools (
shellcheck,markdownlint) from chat to reviews is the right call—these are code quality/static analysis tools that belong in the review pipeline, not chat. The configuration is structurally correct per the CodeRabbit schema.pkg/db/models/prow.go (1)
60-62: ProwJobRun.Labels field is well-aligned with existing model patterns.Using
pq.StringArrayplus a GIN index for label IDs fits the existing schema conventions (e.g., variants) and should work well with the new filtering semantics. JSON exposure as"labels"is appropriate for the API representation.sippy-ng/src/datagrid/GridToolbarFilterItem.js (1)
37-42: Array operator set matches new semantics and integrates cleanly.Updating array operators to
['has entry', 'has entry containing', 'is empty']aligns with the new membership semantics and still respects the existing “no value” handling foris empty. Users can express “is not empty” via theNottoggle, so no further changes seem required here.sippy-ng/src/tests/TestTable.js (1)
737-745: Variant filter change tohas entryis consistent and correct.Switching the variants filter in the job‑runs links from
containstohas entrymatches the new array‑field semantics and the updated operator set in the toolbar. The rest of the filter construction logic is unchanged and remains compatible.sippy-ng/src/tests/TestAnalysis.js (1)
58-66: Updated default variants filters in TestAnalysis correctly usehas entry.Using
has entry(wrapped innot(...)) for the initialvariantsfilters keeps TestAnalysis consistent with the rest of the UI and the backend’s array semantics while preserving the intent of excluding aggregated/never‑stable variants.sippy-ng/src/jobs/VariantStatus.js (1)
34-41: VariantStatus now consistently useshas entryfor variants filters.All three variant‑based filters (chart, jobs table, job runs table) now use
has entry, which matches the new array operator behavior elsewhere in the UI and should keep variant views and job‑run links in sync.Also applies to: 63-68, 75-80
cmd/sippy/seed_data.go (1)
336-428: Fix pq.StringArray assignments for Symptom.LabelIDs and ProwJobRun.Labels (compile-time type mismatch).
jobrunscan.Symptom.LabelIDsandmodels.ProwJobRun.Labelsare declared aspq.StringArray, but they are initialized/assigned from[]stringvalues. Sincepq.StringArrayis a named type over[]string, direct assignment from[]stringis not assignable and will fail to compile without an explicit conversion.Fix by importing
github.com/lib/pqand wrapping[]stringvalues withpq.StringArray(...):
- For each
LabelIDs: []string{...}in the symptoms slice, useLabelIDs: pq.StringArray{...}- For the
jobRuns[i].Labels = selectedLabelsassignment, usejobRuns[i].Labels = pq.StringArray(selectedLabels)Applies to lines 336-428 and 430-487. Run
go build ./cmd/sippyto verify the fix resolves the type mismatch errors.sippy-ng/src/helpers.js (7)
120-124: LGTM - Operator change aligns with array membership semantics.The switch from
containstohas entryfor thevariantsfield correctly reflects array membership checking rather than substring matching.
138-151: Consistent application ofhas entryfor variant exclusion.The negated filter using
not(filterFor('variants', 'has entry', variant))correctly excludes job runs containing specific variants from the analysis.
175-179: LGTM - Both array fields now usehas entry.Correctly applies
has entrysemantics for bothfailed_test_namesandvariantsarray fields.
182-194: LGTM - Consistent operator for test failure filtering.
196-208: LGTM - Flake filtering uses same semantics.
261-265: LGTM - Job variant path useshas entry.
284-286: LGTM -withoutUnstablehelper updated consistently.sippy-ng/src/constants.js (1)
117-128: LGTM - Bookmark operators updated for array field semantics.The
NO_NEVER_STABLEandNO_AGGREGATEDbookmarks now correctly usehas entryfor thevariantsarray field, consistent with the filtering behavior changes across the codebase.pkg/api/autocomplete.go (2)
11-15: LGTM - Clean struct for label autocomplete results.The
LabelOptionstruct appropriately represents the label data with bothName(id) andTitlefor display purposes.
26-45: LGTM - Labels autocomplete returns structured objects.The special handling for labels correctly queries
job_run_labelsand returnsLabelOptionobjects with both the identifier and human-readable title, enabling richer autocomplete UX in the frontend.sippy-ng/src/tests/FeatureGates.js (2)
27-58: Verify intentional distinction betweenhas entry containingandhas entryoperators.The
defaultFilterModeluseshas entry containingwhilestaleFeatureGates(lines 60-89) useshas entryfor the sameenabledcolumn exclusions. This means:
defaultFilterModel: Excludes if any array element contains the substring "Default:Hypershift"staleFeatureGates: Excludes only if an array element exactly matches "Default:Hypershift"Please confirm this is the intended behavior, as both filters appear to serve similar purposes (excluding default-enabled feature gates).
96-157: LGTM - Bookmarks consistently usehas entryfor exact array element matching.The bookmark filters correctly use
has entryfor selecting or excluding exact values likeDefault:Hypershift,TechPreviewNoUpgrade:SelfManagedHA, etc. from theenabledarray.pkg/api/tests.go (4)
358-372: LGTM - Parameterized BigQuery queries prevent SQL injection.Good improvement: using
@releaseplaceholder and collecting parameters viaqueryParamsslice prevents SQL injection. TheparamIndexpointer correctly tracks parameter naming across filter applications.
397-402: LGTM - Processed filter parameters correctly appended.The
processedFilterparameters are properly accumulated intoqueryParamsusing the same pattern asrawFilter.
446-455: LGTM - Parameters correctly assigned to query.All accumulated parameters are properly assigned to the BigQuery query before execution.
468-469: LGTM - Parameter reuse for overall query is correct.Reusing the same
queryParamsfor the overall aggregation query ensures consistency and avoids parameter duplication.sippy-ng/src/jobs/JobRunsTable.js (3)
156-195: LGTM! Well-structured Labels column implementation.The column correctly handles missing label definitions by falling back to the label ID, and appropriately disables sorting for array fields. The tooltip and dialog interaction provides good UX.
535-576: LGTM! Clean dialog implementation.The dialog correctly handles missing label definitions and uses ReactMarkdown for rich formatting. The title dynamically shows context (job name and timestamp), improving usability.
239-257: LGTM! Consistent type annotations.Explicitly marking these columns as
type: 'array'aligns with the new Labels column and likely supports proper filtering and autocomplete behavior for array fields.pkg/filter/filterable.go (7)
69-104: LGTM! Well-structured helper functions.The helper functions correctly handle ILIKE filtering for both scalar and array fields, with proper BigQuery and Postgres-specific logic for empty array checks. The fix to use
db.Whereunconditionally inapplyIlikeFilter(avoiding double negation) is correct.
113-120: LGTM! Correct Postgres array operators.The
OperatorHasEntrycorrectly usesANYfor positive matches andALLwith NULL handling for negation. UnifyingOperatorContainsandOperatorHasEntryContainingthroughilikeFilteris a good refactoring.
170-222: LGTM! Consistent filter application.The
andFilterToSQLcorrectly mirrorsorFilterToSQLlogic and appropriately delegates to helper functions for ILIKE filtering and empty checks.
257-263: LGTM! Correct BigQuery LIKE and EXISTS logic.The Contains operators correctly generate
NOT EXISTS (...)for negated array searches andNOT LOWER(field) LIKE @paramfor negated string searches. The NOT precedence is correct in both cases.
264-307: LGTM! Comprehensive BigQuery operator support.The arithmetic operators correctly place comparison operators between field and parameter. The
makeNumParamhelper properly converts strings to numbers for BigQuery. StartsWith, EndsWith, and empty checks use appropriate BigQuery syntax.
467-502: LGTM! Clean BigQuery filter result structure.The
BQFilterResulttype andToBQStrmethod correctly build parameterized BigQuery WHERE clauses, tracking parameter indices to ensure unique parameter names and properly accumulating all parameters.
38-39: LGTM! Clear operator constant additions.The new
OperatorHasEntryandOperatorHasEntryContainingconstants are well-named and properly integrated throughout the filtering logic.
sosiouxme
force-pushed
the
20251105-TRT-2376-sippy-UI-labels
branch
from
2b1edab to
0ad1c2b
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@sosiouxme: This pull request references TRT-2376 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/api/autocomplete.go (1)
25-45: Labels autocomplete implementation looks good.The special handling for the "labels" field correctly:
- Queries the
job_run_labelstable with proper column aliasing- Supports search filtering on both ID and title
- Applies consistent limit of 50 results
- Returns early with the structured response
As clarified in the previous review, the
idfield isvarchar(80), so usingILIKEdirectly without casting is appropriate.
🧹 Nitpick comments (6)
sippy-ng/src/jobs/JobRunsTable.js (1)
359-380: Silent failure on non-200 responses could be more informative.When the
/api/labelsendpoint returns a non-200 status, the code silently returns an empty object without logging. This makes debugging harder if labels aren't displaying correctly.Consider logging non-200 responses for better observability:
useEffect(() => { fetch(process.env.REACT_APP_API_URL + '/api/labels') .then((response) => { if (response.status !== 200) { + console.warn('Labels API returned status:', response.status) return {} } return response.json() })pkg/db/models/jobrunscan/symptom.go (2)
19-22: Comment mentions matcher types not defined as constants.The comment references "jq" and "xpath" as valid matcher types, but no corresponding constants are defined (lines 59-64). Either add the missing constants or update the comment to reflect the currently supported types.
// Type of matcher - // Simple types: "string", "regex", "jq", "xpath", "none" + // Simple types: "string", "regex", "none" // Compound type: "cel" (Common Expression Language against label names) MatcherType string `gorm:"type:varchar(50);not null" json:"matcher_type"`
58-64: Consider renamingMatcherTypeFilefor clarity.The constant name
MatcherTypeFileis misleading since its value is"none". The name suggests file-based matching, while the value suggests no matching. Consider renaming to align with either the value or the purpose:
MatcherTypeNone(aligns with value)MatcherTypeFileExists(aligns with comment/purpose)const ( MatcherTypeString = "string" // Simple substring match MatcherTypeRegex = "regex" // Regular expression match - MatcherTypeFile = "none" // File exists (no content match) + MatcherTypeNone = "none" // File exists (no content match) MatcherTypeCEL = "cel" // Common Expression Language for compound logic )pkg/filter/filterable.go (3)
113-117: Consider adding array type validation for consistency.The
HasEntryoperator handling inorFilterToSQLandandFilterToSQLassumes the field is an array type without checking (usingANY/ALLoperators), whiletoBQStr(lines 253-256) validates the field type and falls back to "UnknownFilterOperator()" for non-arrays.While the database will catch the error at runtime, adding consistent type validation in the SQL paths would provide earlier feedback and match the BigQuery path's behavior.
Example for
orFilterToSQL:case OperatorHasEntry: + if filterable == nil || filterable.GetFieldType(f.Field) != apitype.ColumnTypeArray { + return "UnknownFilterOperator()", nil + } if f.Not { return fmt.Sprintf("%s IS NULL OR ? != ALL(%s)", field, field), f.Value } return fmt.Sprintf("? = ANY(%s)", field), f.ValueAlso applies to: 170-175
244-250: Consider logging numeric conversion errors.The
makeNumParamhelper handles conversion errors by embedding the error message in the parameter value (line 247), which will cause BigQuery to fail. While this achieves the intended fail-fast behavior, logging the error would aid debugging without relying solely on the downstream BigQuery error.makeNumParam := func() []bigquery.QueryParameter { num, err := strconv.ParseFloat(f.Value, 64) if err != nil { + log.Errorf("Failed to parse numeric filter value %q for field %s: %v", f.Value, f.Field, err) return makeParam("NOT A NUMBER: " + f.Value) // which will break appropriately } return makeParam(num) }
160-160: Optional: Log unknown operators before SQL failure.The fallback to
"UnknownFilterOperator()"will cause obvious SQL failures, which is the intended behavior. However, logging the unknown operator before returning would improve debuggability by providing immediate feedback rather than relying on downstream SQL errors.+ log.Errorf("Unknown filter operator %q for field %s", f.Operator, f.Field) return "UnknownFilterOperator()", nil // cause SQL to fail in obvious wayAlso applies to: 309-309
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (12)
cmd/sippy/seed_data.go(4 hunks)pkg/api/autocomplete.go(1 hunks)pkg/apis/api/types.go(3 hunks)pkg/db/db.go(2 hunks)pkg/db/models/jobrunscan/label.go(1 hunks)pkg/db/models/jobrunscan/symptom.go(1 hunks)pkg/db/models/prow.go(1 hunks)pkg/db/views.go(1 hunks)pkg/filter/filterable.go(7 hunks)pkg/sippyserver/server.go(3 hunks)sippy-ng/src/datagrid/GridToolbarAutocomplete.js(2 hunks)sippy-ng/src/jobs/JobRunsTable.js(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/db/views.go
- pkg/apis/api/types.go
- pkg/db/models/jobrunscan/label.go
- cmd/sippy/seed_data.go
- sippy-ng/src/datagrid/GridToolbarAutocomplete.js
🧰 Additional context used
🧬 Code graph analysis (5)
pkg/sippyserver/server.go (1)
pkg/sippyserver/capabilities.go (1)
LocalDBCapability(9-9)
pkg/db/db.go (2)
pkg/db/models/jobrunscan/label.go (2)
Label(11-33)Label(41-43)pkg/db/models/jobrunscan/symptom.go (2)
Symptom(11-52)Symptom(54-56)
pkg/api/autocomplete.go (2)
pkg/db/db.go (1)
DB(27-33)vendor/github.com/gorilla/mux/mux.go (1)
Vars(430-435)
pkg/filter/filterable.go (1)
pkg/apis/api/types.go (2)
ColumnTypeArray(24-24)ColumnTypeTimestamp(25-25)
pkg/db/models/jobrunscan/symptom.go (1)
vendor/github.com/invopop/jsonschema/id.go (1)
ID(12-12)
🔇 Additional comments (16)
sippy-ng/src/jobs/JobRunsTable.js (3)
156-195: LGTM! Well-implemented Labels column.The Labels column implementation handles edge cases properly:
- Empty/null arrays return empty string
- Label IDs gracefully fall back to showing the ID when lookup fails
- Tooltip provides quick preview of label titles
- Button opens detailed dialog with full explanations
535-576: LGTM! Labels dialog is well-structured.The dialog provides a clean UX with:
- Dynamic title showing job name and timestamp
- List of labels with titles and markdown-rendered explanations
- Fallback display when a label isn't found in the lookup
237-257: Good addition oftype: 'array'for array fields.Adding explicit
type: 'array'tovariants,failed_test_names, andflaked_test_namescolumns aligns with the filter/operator changes mentioned in the PR for handling array-based filtering.pkg/db/models/prow.go (1)
60-62: LGTM! Well-designed Labels field addition.The
Labelsfield is properly defined with:
pq.StringArrayfor PostgreSQLtext[]compatibility- GIN index for efficient array containment queries
- Clear documentation explaining the field's purpose
pkg/db/db.go (1)
69-104: Good refactor to loop-based migrations.The migration logic is now cleaner and more maintainable:
- Consolidated list makes it easy to see all migrated models
- Loop preserves fail-fast error handling
- New
jobrunscan.Labelandjobrunscan.Symptommodels are properly includedpkg/sippyserver/server.go (2)
982-995: LGTM! Clean handler implementation.The handler correctly:
- Queries all labels from the database
- Logs errors with context before returning
- Wraps response in a consistent structure matching frontend expectations
One minor consideration: If the labels table grows large, you may want to add pagination. However, labels are typically a small, controlled set, so this is likely fine for now.
2004-2010: LGTM! Endpoint properly registered.The
/api/labelsendpoint is correctly configured with:
LocalDBCapabilityrequirement- 24-hour cache (appropriate for relatively static label definitions)
pkg/api/autocomplete.go (1)
11-15: LGTM! Clean type definition for label autocomplete.The
LabelOptionstruct provides the necessary fields for the frontend autocomplete component to display both the label ID (for filtering) and the human-readable title.pkg/db/models/jobrunscan/symptom.go (2)
54-56: LGTM!The
TableNamemethod correctly overrides the default GORM table name convention.
10-52: Well-designed model addressing prior feedback.The struct properly defines its own primary key (
ID string) and metadata fields (CreatedAt,UpdatedAt,DeletedAt) without embeddinggorm.Model, which resolves the previously raised issue about conflicting primary keys. The GORM tags and field types are appropriate for PostgreSQL.pkg/filter/filterable.go (6)
10-10: LGTM: Well-designed BigQuery parameterization infrastructure.The
BQFilterResultstruct and updatedToBQStrmethod signature properly support parameterized queries with parameter index tracking across multiple filter calls. The pointer-basedparamIndexallows correct parameter numbering when filters are nested or called multiple times.Also applies to: 467-502
34-41: LGTM: Clear operator definitions for array handling.The new
OperatorHasEntryandOperatorHasEntryContainingoperators are well-named and follow the existing pattern. The distinction between exact and partial matching is clear.
68-104: LGTM: Clean helper abstractions for filter logic.The helper functions properly abstract:
- Conditional NOT prefix generation (
optNot)- ILIKE pattern matching for both string and array fields (
ilikeFilter,applyIlikeFilter)- Null/empty checks with BigQuery-specific array handling (
isEmptyFilter)The fix for the double negation bug (always using
db.Whereon line 88) is correctly implemented per past review comments.
257-263: LGTM: Correct case-insensitive pattern matching for arrays and strings.The
ContainsandHasEntryContainingoperators properly handle both array fields (usingEXISTSwithUNNEST) and string fields, with consistent case-insensitive matching viaLOWER(). Negation is correctly applied viaoptNot.
233-242: LGTM: Proper SQL injection protection via parameter binding.All user-supplied filter values are safely bound as named BigQuery parameters through
makeParamandmakeNumParamhelpers. The 1-based parameter naming scheme (filterParam1,filterParam2, etc.) is clear and consistent.
477-502: LGTM: Robust parameter index management.The pointer-based
paramIndexparameter correctly tracks parameter numbers across multiple filter items, preventing naming collisions. The enhanced logging (line 496) with parameter counts is helpful for debugging parameterized queries.
|
Scheduling required tests: |
🤖 Assisted by Claude Code
* add labels column with tooltip * add popup with full label explanations * add labels to filtering 🤖 Assisted by Claude Code
This looked ugly anyway. Turn repetitive code into an array... 🤖 Assisted by Claude Code
|
@sosiouxme: This pull request references TRT-2376 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@sosiouxme: This pull request references TRT-2376 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@sosiouxme: This pull request references TRT-2376 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@sosiouxme: This pull request references TRT-2376 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
sosiouxme
force-pushed
the
20251105-TRT-2376-sippy-UI-labels
branch
from
0ad1c2b to
c76313a
Compare
|
@sosiouxme: This pull request references TRT-2376 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
pkg/filter/filterable.go (1)
311-311: LGTM! Consider adding similar logging to line 160 for consistency.The error logging for unknown operators is a good improvement for debugging filter issues.
For consistency, consider adding similar logging to line 160 in
orFilterToSQL, which has identical error handling but no logging:+ log.Errorf("Unknown filter operator %q for field %s", f.Operator, f.Field) return "UnknownFilterOperator()", nil // cause SQL to fail in obvious waypkg/api/autocomplete.go (1)
26-45: Consider filtering out soft-deleted labels.The
Labelmodel has aDeletedAtfield for soft deletes. The current query may return deleted labels in autocomplete results.Apply this diff to exclude soft-deleted labels:
if field == "labels" { labelResult := make([]LabelOption, 0) q := dbc.DB.Table("job_run_labels"). Select("id as name, label_title as title"). + Where("deleted_at IS NULL"). Order("name")sippy-ng/src/jobs/JobRunsTable.js (2)
359-383: Consider handling the race condition when labels are used before they're loaded.The labels are fetched independently from the job runs data. If a user clicks on a label button before the
/api/labelsfetch completes,allLabelswill be an empty object, causing all labels to show as "Label not found" in the dialog.Consider either:
- Disabling the label button while
allLabelsis empty- Adding a loading state for labels
- Ensuring the labels fetch completes before rendering the table (though this adds latency)
The current fallback to show
labelIdwhen label is not found (line 169, 559) provides a reasonable degraded experience, so this is a minor concern.
173-178: Prefer using labelId as the key instead of array index.Using
labelIdas the key would provide more stable React reconciliation than array indices, especially if labels were reordered.- {labelTitles.map((title, idx) => ( - <div key={idx}>{title}</div> + {params.value.map((labelId, idx) => ( + <div key={labelId}> + {allLabels[labelId]?.label_title || labelId} + </div> ))}cmd/sippy/seed_data.go (2)
372-376: Minor: "New record" detection logic is inverted.The condition
existing.CreatedAt.IsZero() || existing.CreatedAt.Equal(existing.UpdatedAt)for detecting a newly created record is incorrect:
CreatedAt.IsZero()would only be true if GORM failed to set it (unlikely)CreatedAt.Equal(UpdatedAt)is true for newly created records, but the log message says "Created new" when this is trueSince
FirstOrCreatesets bothCreatedAtandUpdatedAtto the same value for new records, the condition happens to work by accident for new records, but the "already exists" branch would never be reached because existing records loaded from DB also have equal timestamps initially. This only affects debug logging, so it's not critical.A cleaner approach would be to check if the record was actually created:
- if existing.CreatedAt.IsZero() || existing.CreatedAt.Equal(existing.UpdatedAt) { - log.Debugf("Created new Label: %s", label.ID) - } else { - log.Debugf("Label already exists: %s", label.ID) - } + // FirstOrCreate returns the existing record if found + // We can compare to see if we created or found + if existing.LabelTitle == label.LabelTitle { + log.Debugf("Label already exists or created: %s", label.ID) + }Or simply log without the distinction since it's debug-level.
Also applies to: 420-424
430-487: Consider batching updates for better performance in large datasets.The current implementation loads all job runs into memory and performs individual
Savecalls for each labeled run. For a seed data tool with moderate data volumes, this is acceptable. However, if the dataset grows large, this could become slow.For future improvement, consider:
- Batching updates (e.g., using
dbc.DB.Save(&batch)with slices)- Using a raw SQL UPDATE with array concatenation for bulk operations
Since this is a development/seeding tool, the current approach is reasonable.
pkg/db/models/jobrunscan/symptom.go (2)
58-64: Consider clarifying the MatcherTypeFile constant naming.The constant
MatcherTypeFile = "none"could be confusing since "none" doesn't clearly indicate "file existence check." The comment helps, but the value "none" might be misinterpreted as "no matching" rather than "match by file existence."Consider renaming for clarity, though this may have migration implications if the value is already stored in databases:
const ( MatcherTypeString = "string" // Simple substring match MatcherTypeRegex = "regex" // Regular expression match - MatcherTypeFile = "none" // File exists (no content match) + MatcherTypeFile = "file" // File exists (no content match) MatcherTypeCEL = "cel" // Common Expression Language for compound logic )If changing the value isn't feasible, the current naming with the comment is acceptable.
19-22: Documentation mentions matcher types without corresponding constants.The comment lists "jq" and "xpath" as valid simple types, but there are no corresponding constants defined. Either these are planned for future implementation (in which case a TODO comment would help) or the documentation should be updated to reflect only the currently supported types.
// Type of matcher - // Simple types: "string", "regex", "jq", "xpath", "none" + // Simple types: "string", "regex", "none" // Compound type: "cel" (Common Expression Language against label names) MatcherType string `gorm:"type:varchar(50);not null" json:"matcher_type"`Or add TODO for planned types:
// TODO: Future types may include "jq" and "xpath"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (12)
cmd/sippy/seed_data.go(4 hunks)pkg/api/autocomplete.go(1 hunks)pkg/apis/api/types.go(3 hunks)pkg/db/db.go(2 hunks)pkg/db/models/jobrunscan/label.go(1 hunks)pkg/db/models/jobrunscan/symptom.go(1 hunks)pkg/db/models/prow.go(1 hunks)pkg/db/views.go(1 hunks)pkg/filter/filterable.go(2 hunks)pkg/sippyserver/server.go(3 hunks)sippy-ng/src/datagrid/GridToolbarAutocomplete.js(2 hunks)sippy-ng/src/jobs/JobRunsTable.js(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/sippyserver/server.go
- sippy-ng/src/datagrid/GridToolbarAutocomplete.js
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/db/db.go (2)
pkg/db/models/jobrunscan/label.go (2)
Label(11-33)Label(41-43)pkg/db/models/jobrunscan/symptom.go (2)
Symptom(11-52)Symptom(54-56)
cmd/sippy/seed_data.go (4)
pkg/apis/sippyprocessing/v1/types.go (6)
JobOverallResult(69-69)JobTestFailure(77-77)JobUpgradeFailure(76-76)JobInstallFailure(75-75)JobInfrastructureFailure(74-74)JobSucceeded(72-72)pkg/db/models/jobrunscan/label.go (2)
Label(11-33)Label(41-43)pkg/db/models/jobrunscan/symptom.go (5)
Symptom(11-52)Symptom(54-56)MatcherTypeString(60-60)MatcherTypeRegex(61-61)MatcherTypeFile(62-62)pkg/db/models/prow.go (1)
ProwJobRun(36-66)
pkg/api/autocomplete.go (2)
pkg/db/db.go (1)
DB(27-33)vendor/github.com/gorilla/mux/mux.go (1)
Vars(430-435)
🔇 Additional comments (12)
pkg/filter/filterable.go (1)
247-248: LGTM! Good addition for observability.The error logging provides helpful diagnostic information when numeric filter values fail to parse. This will make debugging filter issues much easier without changing the existing error-handling behavior.
pkg/apis/api/types.go (2)
358-358: LGTM!The
Labelsfield addition follows the established pattern for array fields in theJobRunstruct, with appropriate GORM and JSON tags matching otherpq.StringArrayfields likeTagsandVariants.
377-378: Field type and accessor methods correctly extended.The
GetFieldTypeandGetArrayValueimplementations are consistent with the existing pattern for array fields, and properly integrate with the filtering infrastructure.Also applies to: 444-445
pkg/db/views.go (1)
208-208: LGTM!The
labelscolumn is correctly added to the materialized view, exposing the new field fromprow_job_runsfor downstream API queries.pkg/db/models/prow.go (1)
60-62: LGTM!The
Labelsfield is well-documented and follows the established pattern for indexed array fields in this codebase. The GIN index will support efficient array containment queries for label filtering.pkg/db/db.go (1)
69-104: Clean refactor to list-driven migrations.The consolidation of
AutoMigratecalls into a loop improves maintainability. The newjobrunscan.Labelandjobrunscan.Symptommodels are correctly included.pkg/api/autocomplete.go (1)
11-15: LGTM!The
LabelOptionstruct provides the necessary fields for the UI to display both the label identifier and a human-readable title in the autocomplete dropdown.pkg/db/models/jobrunscan/label.go (1)
10-43: LGTM! Clean model definition without embedded gorm.Model.The Label struct correctly defines explicit fields for the primary key and timestamps, avoiding the previous issue with conflicting primary keys from embedding
gorm.Model. The use ofpq.StringArrayfor theHideDisplayContextsfield is appropriate for PostgreSQL'stext[]type, and the display context constants are well-documented.sippy-ng/src/jobs/JobRunsTable.js (1)
538-579: LGTM! Well-structured dialog with proper accessibility handling.The dialog implementation is clean with appropriate MUI components. Using
ReactMarkdownfor rendering label explanations allows for rich formatting in the explanation content. The key prop correctly useslabelIdfor stable list rendering.cmd/sippy/seed_data.go (2)
298-323: LGTM! OverallResult assignment logic is correct.The random distribution correctly covers all failure types with sensible proportions (70% test, 15% upgrade, 7% install, 8% infrastructure). The conditional boundaries are accurate and the
v1.JobOverallResultconstants are used appropriately.
338-365: Good sample data demonstrating various label features.The sample labels effectively demonstrate the different capabilities: markdown formatting with links and lists, and the
HideDisplayContextsfeature for controlling where labels appear. This will be helpful for development and testing.pkg/db/models/jobrunscan/symptom.go (1)
10-56: LGTM! Well-structured Symptom model with proper GORM configuration.The Symptom struct correctly defines explicit fields without embedding
gorm.Model, resolving the previous review concern. The use ofpq.StringArrayfor multiple array fields is consistent, and the filter/validity window fields provide good flexibility for scoping symptom matching.
|
Scheduling required tests: |
|
@sosiouxme: all tests passed! Full PR test history. Your PR dashboard. Instructions 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. |
3 participants
This defines symptom/label data structures and displays labels in the job runs table. There is also an update to the seed data generator to add some labels and randomly apply them to job runs generated so you can see labels in action.
Adds a column to job runs table exposing any labels:
Clicking on it displays the full label explanation:
...and of course you can filter by it as well:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.