TRT-2364: Fix timestamp and date type inconsistencies#3716
Open
mstaeble wants to merge 1 commit into
Open
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@mstaeble: This pull request references TRT-2364 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 "5.0.0" version, but no target version was set. DetailsIn 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-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
The actionable item here is for the linked story |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mstaeble 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 |
mstaeble
force-pushed
the
trt-2364-timestamp-consistency
branch
2 times, most recently
from
bb354da to
0ce2e75
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
WalkthroughThe PR standardizes timestamp and date handling across backend types, database/query logic, API responses, tests, and frontend controls. It replaces epoch-millisecond and ad hoc string handling with UTC timestamps, civil dates, ISO 8601 strings, and ChangesTimestamp and date normalization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 19 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (19 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
sippy-ng/src/component_readiness/TriagedRegressionTestList.js (1)
215-235: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSet
type: 'date'on theLast Failurecolumn.The
valueGetternow returns aDateobject, but the column lackstype: 'date', so DataGrid sorting/filtering will not treat it as a date.♻️ Proposed change
{ field: 'last_failure', headerName: 'Last Failure', flex: 12, filterable: false, + type: 'date', valueGetter: (params) => {As per coding guidelines: "For MUI DataGrid timestamp columns, use
type: 'date'with avalueGetterthat returns aDateobject."🤖 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 `@sippy-ng/src/component_readiness/TriagedRegressionTestList.js` around lines 215 - 235, The Last Failure column in TriagedRegressionTestList is returning a Date from its valueGetter but is missing the DataGrid date type. Update the column definition for the last_failure field to include type: 'date' so MUI DataGrid sorts and filters it as a date; keep the existing valueGetter and renderCell behavior unchanged.Source: Coding guidelines
sippy-ng/src/component_readiness/RegressedTestsPanel.js (1)
215-265: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSet
type: 'date'on these timestamp columns.Both
Regressed SinceandLast Failurenow returnDateobjects fromvalueGetter, but neither column declarestype: 'date'. Per the frontend guideline, MUI DataGrid timestamp columns should usetype: 'date'with avalueGetterreturning aDateso sorting and any date filtering behave correctly.♻️ Proposed change
{ field: 'regression', headerName: 'Regressed Since', flex: 12, filterable: false, + type: 'date', valueGetter: (params) => {{ field: 'last_failure', headerName: 'Last Failure', flex: 12, filterable: false, + type: 'date', valueGetter: (params) => {As per coding guidelines: "For MUI DataGrid timestamp columns, use
type: 'date'with avalueGetterthat returns aDateobject."🤖 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 `@sippy-ng/src/component_readiness/RegressedTestsPanel.js` around lines 215 - 265, The RegressedTestsPanel DataGrid columns for Regessed Since and Last Failure return Date objects from their valueGetter functions but do not declare the column type, so update both column definitions to use type: 'date' while keeping the existing valueGetter logic in place. This applies to the column objects that define field 'regression' and field 'last_failure', so sorting and date-aware behavior work consistently with the Date values being returned.Source: Coding guidelines
pkg/api/test_analysis.go (1)
25-43: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAnchor the overall analysis window to
reportEnd.The grouped queries use the caller-provided
reportEnd, but the overall query still uses wall-clocktime.Now()and has no upper bound. Historical report requests can return anoverallseries for a different 14-day window than the job or variant series.Proposed fix
- Where("date >= ?", time.Now().Add(-24*14*time.Hour)). + Where("date <= ?", reportEnd). + Where("date >= ?", reportEnd.Add(-24*14*time.Hour)).🤖 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 `@pkg/api/test_analysis.go` around lines 25 - 43, The overall query in GetTestAnalysisOverallFromDB is using wall-clock time instead of the caller-provided reportEnd, so its date window can drift from the other series. Update the time filter in GetTestAnalysisOverallFromDB to anchor the 14-day lookback on reportEnd, and add an upper bound at reportEnd so the overall series matches the grouped queries’ window; use the existing query builder chain in GetTestAnalysisOverallFromDB to make the change.
🤖 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 `@sippy-ng/src/datagrid/GridToolbarFilterItem.js`:
- Around line 107-112: The date filter update in GridToolbarFilterItem’s
onChange handler is rebuilding the filter model without carrying over
props.filterModel.not, which causes negated date filters to lose their negation
state after a picker change. Update the props.setFilterModel call in this date
value change path to preserve the existing not flag from props.filterModel
alongside columnField, operatorValue, and value so the filter behavior stays
consistent.
In `@sippy-ng/src/jobs/JobsDetail.js`:
- Around line 57-58: The job details flow in JobsDetail.js uses
Temporal.PlainDate.from directly, but the frontend does not guarantee native
Temporal support. Import the Temporal polyfill in this module (or a shared entry
point used before JobsDetail runs) and ensure the existing
setStartDate/setEndDate logic uses that polyfilled Temporal reference so opening
job details works in browsers without native support.
---
Outside diff comments:
In `@pkg/api/test_analysis.go`:
- Around line 25-43: The overall query in GetTestAnalysisOverallFromDB is using
wall-clock time instead of the caller-provided reportEnd, so its date window can
drift from the other series. Update the time filter in
GetTestAnalysisOverallFromDB to anchor the 14-day lookback on reportEnd, and add
an upper bound at reportEnd so the overall series matches the grouped queries’
window; use the existing query builder chain in GetTestAnalysisOverallFromDB to
make the change.
In `@sippy-ng/src/component_readiness/RegressedTestsPanel.js`:
- Around line 215-265: The RegressedTestsPanel DataGrid columns for Regessed
Since and Last Failure return Date objects from their valueGetter functions but
do not declare the column type, so update both column definitions to use type:
'date' while keeping the existing valueGetter logic in place. This applies to
the column objects that define field 'regression' and field 'last_failure', so
sorting and date-aware behavior work consistently with the Date values being
returned.
In `@sippy-ng/src/component_readiness/TriagedRegressionTestList.js`:
- Around line 215-235: The Last Failure column in TriagedRegressionTestList is
returning a Date from its valueGetter but is missing the DataGrid date type.
Update the column definition for the last_failure field to include type: 'date'
so MUI DataGrid sorts and filters it as a date; keep the existing valueGetter
and renderCell behavior 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2edbb857-8aaf-4fc5-8ae1-8d17c2410c15
⛔ Files ignored due to path filters (8)
.claude/rules/backend.mdis excluded by!.claude/**.claude/rules/frontend.mdis excluded by!.claude/**.cursor/rules/backend.mdcis excluded by!.cursor/**.cursor/rules/frontend.mdcis excluded by!.cursor/**AGENTS.mdis excluded by!AGENTS.mdCLAUDE.mdis excluded by!CLAUDE.mdsippy-ng/AGENTS.mdis excluded by!sippy-ng/AGENTS.mdsippy-ng/CLAUDE.mdis excluded by!sippy-ng/CLAUDE.md
📒 Files selected for processing (35)
.apm/instructions/backend.instructions.md.apm/instructions/frontend.instructions.mdapm.lock.yamlpkg/api/README.mdpkg/api/componentreadiness/dataprovider/bigquery/releasedates.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/queryparamparser_test.gopkg/api/componentreadiness/triage_test.gopkg/api/componentreadiness/utils/utils_test.gopkg/api/job_runs.gopkg/api/jobs.gopkg/api/releases.gopkg/api/releases_test.gopkg/api/test_analysis.gopkg/apis/api/types.gopkg/apis/sippy/v1/types.gopkg/apis/sippyprocessing/v1/types.gopkg/db/db.gopkg/db/views.gopkg/filter/filterable.gopkg/sippyserver/chat_conversations.gopkg/sippyserver/parameters.gopkg/sippyserver/server.gopkg/util/utils.gopkg/util/utils_test.gosippy-ng/src/App.jssippy-ng/src/build_clusters/BuildClusterDetails.jssippy-ng/src/component_readiness/RegressedTestsPanel.jssippy-ng/src/component_readiness/TriagedRegressionTestList.jssippy-ng/src/datagrid/GridToolbarFilterItem.jssippy-ng/src/datagrid/utils.jssippy-ng/src/helpers.jssippy-ng/src/jobs/JobRunsTable.jssippy-ng/src/jobs/JobStackedChart.jssippy-ng/src/jobs/JobsDetail.js
💤 Files with no reviewable changes (1)
- sippy-ng/src/App.js
mstaeble
force-pushed
the
trt-2364-timestamp-consistency
branch
from
0ce2e75 to
4c44aed
Compare
openshift-ci
Bot
added
the
ready-for-human-review
mstaeble
force-pushed
the
trt-2364-timestamp-consistency
branch
from
4c44aed to
fddef49
Compare
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sippy-ng/src/tests/FeatureGates.js (1)
213-225: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPotential in-place mutation of the memoized default.
When the
filtersparam is absent,filterModelis the memoizeddefaultFilterModel.requestSearchmutatescurrentFilters.itemsin place (filterModel.items.filter(...)assigned back), andbookmarks[0].modelalso points atdefaultFilterModel.items(Line 127). Mutating the shared default can corrupt the bookmark/default state. Consider cloning before mutating.🤖 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 `@sippy-ng/src/tests/FeatureGates.js` around lines 213 - 225, The requestSearch helper in FeatureGates is mutating the shared memoized defaultFilterModel through filterModel/currentFilters, which can corrupt bookmark/default state because bookmarks[0].model also points at that same items array. Update requestSearch to work on a cloned filter model and cloned items before filtering/pushing the feature_gate criterion, then pass the new object to setFilterModel so defaultFilterModel is never modified in place.pkg/testidentification/ocp_variants.go (1)
110-114: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winBug: existence check uses the wrong map key, dropping accumulated variant values.
variantKeyValuesis keyed by variant name (e.g.AllPlatforms()readsvariantValues["Platform"]), and the insert/create branches both key byv.VariantName. Changing the lookup tovariantKeyValues[v.VariantValue]means theokcheck almost never matches the key actually written. As a result, theelsebranch runs on every value for a given name and overwrites the set instead of inserting, so only the last value per variant name survives.The check must use the same key as the writes (
v.VariantName):🐛 Proposed fix
- if _, ok := variantKeyValues[v.VariantValue]; ok { + if _, ok := variantKeyValues[v.VariantName]; ok { variantKeyValues[v.VariantName].Insert(v.VariantValue) } else { variantKeyValues[v.VariantName] = sets.NewString(v.VariantValue) }🤖 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 `@pkg/testidentification/ocp_variants.go` around lines 110 - 114, The existence check in the variant accumulation logic uses the wrong map key, causing previously collected values to be lost. In the code that updates variantKeyValues, make sure the lookup matches the same key used for writes, namely v.VariantName, so the insert branch is taken when that variant name already exists and values are accumulated instead of overwritten.
🧹 Nitpick comments (3)
sippy-ng/src/App.js (1)
498-502: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore the
[isLoaded]dependency array.Dropping the dependency array makes this effect run after every render. The
if (!isLoaded)guard prevents repeated fetches, so behavior is effectively unchanged, but running the callback on every render is unnecessary and diverges from the prior intent (run whenisLoadedtransitions). Re-adding[isLoaded]is clearer and keepsreact-hooks/exhaustive-depshappy.♻️ Suggested change
useEffect(() => { if (!isLoaded) { fetchData() } - }) + }, [isLoaded])🤖 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 `@sippy-ng/src/App.js` around lines 498 - 502, Restore the missing dependency array on the useEffect in App.js so the fetch logic only runs when isLoaded changes, not after every render. Update the effect that calls fetchData to include [isLoaded], keeping the existing !isLoaded guard intact and preserving the intended transition-based behavior while satisfying react-hooks/exhaustive-deps.sippy-ng/src/tests/TestAnalysis.js (1)
63-72: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winInline default recreates
filterModelevery render.When the
filtersparam is absent, this destructure default produces a brand-new object on each render (unlike the prior stable-reference helper). It feeds the dependency array of the page-context effect (Lines 162-170), sosetPageContextForChatwill rerun on every render. Consider wrapping the default inuseMemokeyed ontestName.🤖 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 `@sippy-ng/src/tests/TestAnalysis.js` around lines 63 - 72, The inline fallback for `filterModel` in `TestAnalysis` creates a new object on every render when `filters` is missing, which makes the `useEffect` that calls `setPageContextForChat` keep retriggering. Move that default construction into a `useMemo` in `TestAnalysis`, keyed on `testName`, and use the memoized value in the `useQueryParam('filters', SafeJSONParam)` destructure so the `filterModel` reference stays stable across renders.pkg/filter/filterable.go (1)
58-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
sets.StringforjobRunFields. This is a hand-rolled set; switch tok8s.io/apimachinery/pkg/util/setsandHas()for consistency with the Go guidelines.🤖 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 `@pkg/filter/filterable.go` around lines 58 - 68, The `jobRunFields` lookup in `StripJobRunFilters` is a hand-rolled set and should be converted to `k8s.io/apimachinery/pkg/util/sets.String` for consistency. Update the `jobRunFields` declaration to use a `sets.String` value, then change the membership check inside `StripJobRunFilters` to use `Has()` instead of map indexing. Keep the rest of the filter-copy logic unchanged.Source: Coding guidelines
🤖 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 `@pkg/filter/filterable.go`:
- Line 670: Update Filter.Filter in filterable.go to handle
apitype.ColumnTypeTimestamp in-memory alongside the existing string, numerical,
and array branches, since JobRun.GetFieldType("timestamp") now resolves to a
timestamp type. Add a dedicated timestamp branch that parses the filter value as
RFC3339, compares it against the item's timestamp field, and returns the same
style of match/non-match result as the other field-type handlers. Also add
regression coverage around the Filter.Filter path to verify timestamp filtering
works and no longer falls through to the “unknown field or field type” case.
In `@sippy-ng/src/component_readiness/TriagedRegressionTestList.js`:
- Around line 221-226: The `valueGetter` in `TriagedRegressionTestList` should
guard against `params.row.last_failure` being null or undefined before accessing
`.Valid`. Update the existing `valueGetter` logic to first check that
`last_failure` exists, then return null when it is missing or invalid, and only
construct the `Date` from `params.row.last_failure.Time` when the object is
present and valid.
In `@sippy-ng/src/jobs/JobAnalysis.js`:
- Around line 160-162: The fetch effect in JobAnalysis is missing props.release
in its dependency list, so navigation between releases can reuse stale data.
Update the useEffect that calls fetchData() to depend on props.release as well
as filterModel and period, and ensure fetchData uses the current release value
from props.release when building the request.
In `@sippy-ng/src/jobs/JobRunsTable.js`:
- Around line 165-172: The timestamp handling in JobRunsTable is missing a
null/invalid guard, so `valueGetter` and `renderCell` can produce `Invalid Date`
when `params.value` is absent or bad. Update the column logic in `JobRunsTable`
to match the defensive pattern used in `RegressedTestsPanel`: return `null` from
`valueGetter` when the timestamp is missing/invalid, and short-circuit
`renderCell` so it renders nothing instead of calling `toLocaleString()` or
`relativeTime()` on an invalid date.
In `@sippy-ng/src/jobs/JobTable.js`:
- Around line 549-551: JobTable does not refetch data when props.release
changes, so stale rows can remain in release-scoped views. Update the useEffect
that calls fetchData() in JobTable to include props.release in its dependency
list, ensuring the effect reruns whenever the release context changes. Keep the
fix localized to the fetchData-triggering effect and preserve the existing
period, filterModel, sort, and sortField dependencies.
In `@sippy-ng/src/pull_requests/PullRequestsTable.js`:
- Around line 371-373: The fetch effect in PullRequestsTable is missing
props.release from its dependency list, so release changes can leave stale pull
request data on screen. Update the useEffect that calls fetchData to include
props.release alongside filterModel, sort, sortField, and view so the request
reruns whenever the active release changes, especially when rendered from
RepositoryDetails.
In `@sippy-ng/src/releases/PayloadStreamTestFailures.js`:
- Around line 198-200: The fetch effect is missing the release stream query
inputs it actually depends on, so updates to release, arch, or stream can leave
PayloadStreamTestFailures showing stale results. Update the useEffect in
PayloadStreamTestFailures so its dependency array includes the values read by
fetchData(), alongside the existing filterModel, sort, and sortField
dependencies, ensuring the table refetches whenever those query params change.
In `@sippy-ng/src/releases/ReleasePayloadPullRequests.js`:
- Around line 159-161: The effect in ReleasePayloadPullRequests is missing the
server-side sort dependencies, so changing sort no longer triggers a refetch.
Update the useEffect that calls fetchData to depend on filterModel, sort, and
sortField, matching the other table components and ensuring the query rebuilt by
fetchData stays in sync with sorting changes. Keep the logic in
ReleasePayloadPullRequests aligned with PayloadStreamsTable,
ReleasePayloadTable, and ReleasePayloadJobRuns so server-side sorting continues
to work.
---
Outside diff comments:
In `@pkg/testidentification/ocp_variants.go`:
- Around line 110-114: The existence check in the variant accumulation logic
uses the wrong map key, causing previously collected values to be lost. In the
code that updates variantKeyValues, make sure the lookup matches the same key
used for writes, namely v.VariantName, so the insert branch is taken when that
variant name already exists and values are accumulated instead of overwritten.
In `@sippy-ng/src/tests/FeatureGates.js`:
- Around line 213-225: The requestSearch helper in FeatureGates is mutating the
shared memoized defaultFilterModel through filterModel/currentFilters, which can
corrupt bookmark/default state because bookmarks[0].model also points at that
same items array. Update requestSearch to work on a cloned filter model and
cloned items before filtering/pushing the feature_gate criterion, then pass the
new object to setFilterModel so defaultFilterModel is never modified in place.
---
Nitpick comments:
In `@pkg/filter/filterable.go`:
- Around line 58-68: The `jobRunFields` lookup in `StripJobRunFilters` is a
hand-rolled set and should be converted to
`k8s.io/apimachinery/pkg/util/sets.String` for consistency. Update the
`jobRunFields` declaration to use a `sets.String` value, then change the
membership check inside `StripJobRunFilters` to use `Has()` instead of map
indexing. Keep the rest of the filter-copy logic unchanged.
In `@sippy-ng/src/App.js`:
- Around line 498-502: Restore the missing dependency array on the useEffect in
App.js so the fetch logic only runs when isLoaded changes, not after every
render. Update the effect that calls fetchData to include [isLoaded], keeping
the existing !isLoaded guard intact and preserving the intended transition-based
behavior while satisfying react-hooks/exhaustive-deps.
In `@sippy-ng/src/tests/TestAnalysis.js`:
- Around line 63-72: The inline fallback for `filterModel` in `TestAnalysis`
creates a new object on every render when `filters` is missing, which makes the
`useEffect` that calls `setPageContextForChat` keep retriggering. Move that
default construction into a `useMemo` in `TestAnalysis`, keyed on `testName`,
and use the memoized value in the `useQueryParam('filters', SafeJSONParam)`
destructure so the `filterModel` reference stays stable across renders.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d990cb2f-d180-42be-82df-c4e55ba8c139
⛔ Files ignored due to path filters (8)
.claude/rules/backend.mdis excluded by!.claude/**.claude/rules/frontend.mdis excluded by!.claude/**.cursor/rules/backend.mdcis excluded by!.cursor/**.cursor/rules/frontend.mdcis excluded by!.cursor/**AGENTS.mdis excluded by!AGENTS.mdCLAUDE.mdis excluded by!CLAUDE.mdsippy-ng/AGENTS.mdis excluded by!sippy-ng/AGENTS.mdsippy-ng/CLAUDE.mdis excluded by!sippy-ng/CLAUDE.md
📒 Files selected for processing (51)
.apm/instructions/backend.instructions.md.apm/instructions/frontend.instructions.mdapm.lock.yamlconfig/openshift.yamlpkg/api/README.mdpkg/api/componentreadiness/dataprovider/bigquery/releasedates.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/queryparamparser_test.gopkg/api/componentreadiness/triage_test.gopkg/api/componentreadiness/utils/utils_test.gopkg/api/job_runs.gopkg/api/jobs.gopkg/api/releases.gopkg/api/releases_test.gopkg/api/test_analysis.gopkg/apis/api/types.gopkg/apis/sippy/v1/types.gopkg/apis/sippyprocessing/v1/types.gopkg/db/db.gopkg/db/views.gopkg/filter/filterable.gopkg/sippyserver/chat_conversations.gopkg/sippyserver/parameters.gopkg/sippyserver/server.gopkg/testidentification/ocp_variants.gopkg/util/utils.gopkg/util/utils_test.gopkg/variantregistry/snapshot.yamlsippy-ng/src/App.jssippy-ng/src/build_clusters/BuildClusterDetails.jssippy-ng/src/component_readiness/RegressedTestsPanel.jssippy-ng/src/component_readiness/TriagedRegressionTestList.jssippy-ng/src/datagrid/GridToolbarFilterItem.jssippy-ng/src/datagrid/utils.jssippy-ng/src/helpers.jssippy-ng/src/jobs/JobAnalysis.jssippy-ng/src/jobs/JobRunsTable.jssippy-ng/src/jobs/JobStackedChart.jssippy-ng/src/jobs/JobTable.jssippy-ng/src/jobs/JobsDetail.jssippy-ng/src/pull_requests/PullRequestsTable.jssippy-ng/src/releases/PayloadStreamTestFailures.jssippy-ng/src/releases/PayloadStreamsTable.jssippy-ng/src/releases/PayloadTestFailures.jssippy-ng/src/releases/ReleasePayloadJobRuns.jssippy-ng/src/releases/ReleasePayloadPullRequests.jssippy-ng/src/releases/ReleasePayloadTable.jssippy-ng/src/repositories/RepositoriesTable.jssippy-ng/src/tests/FeatureGates.jssippy-ng/src/tests/TestAnalysis.jssippy-ng/src/tests/TestTable.js
Contributor
|
Scheduling required tests: |
mstaeble
force-pushed
the
trt-2364-timestamp-consistency
branch
from
fddef49 to
2937c21
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/variantregistry/snapshot.yaml`:
- Around line 26693-26695: The perfscale `multi-ns` jobs are being recorded with
`Architecture: multi`, which causes x86-only jobs to be treated as multi-arch in
downstream variant consumers. Update the snapshot generation in
`pkg/variantregistry/snapshot.go` so these `*-x86-*` entries keep `Architecture`
as `amd64`, and move the `multi-ns` distinction into a separate variant field or
the job key instead. Then regenerate the affected `snapshot.yaml` entries for
the
`periodic-ci-openshift-eng-ocp-qe-perfscale-ci-main-aws-4.22-nightly-x86-cudn-density-multi-ns-500-24nodes`
family.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 634f7723-be81-4a5e-b5b0-3b1a6059594c
⛔ Files ignored due to path filters (8)
.claude/rules/backend.mdis excluded by!.claude/**.claude/rules/frontend.mdis excluded by!.claude/**.cursor/rules/backend.mdcis excluded by!.cursor/**.cursor/rules/frontend.mdcis excluded by!.cursor/**AGENTS.mdis excluded by!AGENTS.mdCLAUDE.mdis excluded by!CLAUDE.mdsippy-ng/AGENTS.mdis excluded by!sippy-ng/AGENTS.mdsippy-ng/CLAUDE.mdis excluded by!sippy-ng/CLAUDE.md
📒 Files selected for processing (38)
.apm/instructions/backend.instructions.md.apm/instructions/frontend.instructions.mdapm.lock.yamlconfig/openshift.yamlpkg/api/README.mdpkg/api/componentreadiness/dataprovider/bigquery/releasedates.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/queryparamparser_test.gopkg/api/componentreadiness/triage_test.gopkg/api/componentreadiness/utils/utils_test.gopkg/api/job_runs.gopkg/api/jobs.gopkg/api/releases.gopkg/api/releases_test.gopkg/api/test_analysis.gopkg/apis/api/types.gopkg/apis/sippy/v1/types.gopkg/apis/sippyprocessing/v1/types.gopkg/db/db.gopkg/db/views.gopkg/filter/filterable.gopkg/sippyserver/chat_conversations.gopkg/sippyserver/parameters.gopkg/sippyserver/server.gopkg/testidentification/ocp_variants.gopkg/util/utils.gopkg/util/utils_test.gopkg/variantregistry/snapshot.yamlsippy-ng/src/App.jssippy-ng/src/build_clusters/BuildClusterDetails.jssippy-ng/src/component_readiness/RegressedTestsPanel.jssippy-ng/src/component_readiness/TriagedRegressionTestList.jssippy-ng/src/datagrid/GridToolbarFilterItem.jssippy-ng/src/datagrid/utils.jssippy-ng/src/helpers.jssippy-ng/src/jobs/JobRunsTable.jssippy-ng/src/jobs/JobStackedChart.jssippy-ng/src/jobs/JobsDetail.js
✅ Files skipped from review due to trivial changes (6)
- pkg/db/db.go
- pkg/api/componentreadiness/queryparamparser_test.go
- .apm/instructions/frontend.instructions.md
- .apm/instructions/backend.instructions.md
- apm.lock.yaml
- pkg/api/README.md
🚧 Files skipped from review as they are similar to previous changes (31)
- pkg/api/componentreadiness/dataprovider/bigquery/releasedates.go
- pkg/sippyserver/chat_conversations.go
- pkg/api/componentreadiness/utils/utils_test.go
- pkg/util/utils_test.go
- pkg/api/job_runs.go
- sippy-ng/src/jobs/JobStackedChart.js
- sippy-ng/src/datagrid/utils.js
- pkg/apis/sippy/v1/types.go
- sippy-ng/src/component_readiness/RegressedTestsPanel.js
- pkg/db/views.go
- sippy-ng/src/build_clusters/BuildClusterDetails.js
- sippy-ng/src/App.js
- sippy-ng/src/component_readiness/TriagedRegressionTestList.js
- pkg/testidentification/ocp_variants.go
- sippy-ng/src/jobs/JobRunsTable.js
- sippy-ng/src/datagrid/GridToolbarFilterItem.js
- pkg/api/componentreadiness/dataprovider/postgres/provider.go
- sippy-ng/src/jobs/JobsDetail.js
- pkg/api/releases_test.go
- pkg/util/utils.go
- pkg/api/componentreadiness/triage_test.go
- pkg/sippyserver/server.go
- pkg/apis/sippyprocessing/v1/types.go
- sippy-ng/src/helpers.js
- pkg/api/test_analysis.go
- pkg/api/releases.go
- pkg/sippyserver/parameters.go
- pkg/filter/filterable.go
- pkg/apis/api/types.go
- pkg/api/jobs.go
- config/openshift.yaml
mstaeble
force-pushed
the
trt-2364-timestamp-consistency
branch
4 times, most recently
from
16f7c71 to
bc4cd1a
Compare
mstaeble
force-pushed
the
trt-2364-timestamp-consistency
branch
3 times, most recently
from
ae5ec5d to
7df6cbc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@pkg/api/test_analysis.go`:
- Around line 42-43: The DATE filter in the analysis query is using time-based
bounds from reportEnd, which can shift the intended day range; update the date
predicates to compare DATE to DATE using civil.Date values. Normalize reportEnd
and the start boundary once in the test setup, then pass those date-only values
into the Where clauses in the analysis query near the reportEnd.Add-based logic.
In `@pkg/filter/filterable.go`:
- Around line 65-66: StripJobRunFilters currently dereferences fil immediately,
so passing nil will panic and break the no-filter case. Update
StripJobRunFilters to handle a nil *Filter input up front by returning nil
before any field access, keeping the existing behavior intact for non-nil
filters. Use the StripJobRunFilters function as the main fix point and ensure
any future dereferences in the function are guarded the same way.
In `@sippy-ng/src/datagrid/GridToolbarFilterItem.js`:
- Around line 103-114: The timestamp filter hydration in GridToolbarFilterItem
is too strict and breaks older bookmarked/shared URLs that still persist
millisecond strings in the filters query param. Update the read path around the
value passed into the date picker to accept both ISO 8601 and numeric-string
timestamps, and reuse the same parsing logic in sippy-ng/src/datagrid/utils.js
so JobRunsTable and other consumers stay consistent. Keep the existing write
path intact, but ensure the parser converts legacy values into a valid Date
before rendering or round-tripping the filter.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4d73c2e5-2ddd-49f0-a824-115dd32b3314
⛔ Files ignored due to path filters (8)
.claude/rules/backend.mdis excluded by!.claude/**.claude/rules/frontend.mdis excluded by!.claude/**.cursor/rules/backend.mdcis excluded by!.cursor/**.cursor/rules/frontend.mdcis excluded by!.cursor/**AGENTS.mdis excluded by!AGENTS.mdCLAUDE.mdis excluded by!CLAUDE.mdsippy-ng/AGENTS.mdis excluded by!sippy-ng/AGENTS.mdsippy-ng/CLAUDE.mdis excluded by!sippy-ng/CLAUDE.md
📒 Files selected for processing (35)
.apm/instructions/backend.instructions.md.apm/instructions/frontend.instructions.mdapm.lock.yamlpkg/api/README.mdpkg/api/componentreadiness/dataprovider/bigquery/releasedates.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/queryparamparser_test.gopkg/api/componentreadiness/triage_test.gopkg/api/componentreadiness/utils/utils_test.gopkg/api/job_runs.gopkg/api/jobs.gopkg/api/releases.gopkg/api/releases_test.gopkg/api/test_analysis.gopkg/apis/api/types.gopkg/apis/sippy/v1/types.gopkg/apis/sippyprocessing/v1/types.gopkg/db/db.gopkg/db/views.gopkg/filter/filterable.gopkg/sippyserver/chat_conversations.gopkg/sippyserver/parameters.gopkg/sippyserver/server.gopkg/util/utils.gopkg/util/utils_test.gosippy-ng/src/App.jssippy-ng/src/build_clusters/BuildClusterDetails.jssippy-ng/src/component_readiness/RegressedTestsPanel.jssippy-ng/src/component_readiness/TriagedRegressionTestList.jssippy-ng/src/datagrid/GridToolbarFilterItem.jssippy-ng/src/datagrid/utils.jssippy-ng/src/helpers.jssippy-ng/src/jobs/JobRunsTable.jssippy-ng/src/jobs/JobStackedChart.jssippy-ng/src/jobs/JobsDetail.js
💤 Files with no reviewable changes (1)
- sippy-ng/src/App.js
mstaeble
force-pushed
the
trt-2364-timestamp-consistency
branch
2 times, most recently
from
eee8932 to
7536045
Compare
Replace epoch millisecond integers with proper timestamp and date types across the database schema, API layer, and frontend. Backend: - Enforce UTC timezone on all PostgreSQL connections via pgx RuntimeParams - Change matview timestamp from bigint epoch to TIMESTAMP WITH TIME ZONE - Use civil.Date for date-only fields (GA dates, development start dates, CountByDate, jobDetailAPIResult start/end) - Change CalendarEvent.Start/End from string to time.Time - Change ChatConversationResponse.CreatedAt from string to time.Time - Change JobRun.Timestamp from int to time.Time (RFC 3339 in JSON) - Remove epoch extraction from SQL filters; compare timestamptz directly - Handle ColumnTypeTimestamp in Compare() via GetNumericalValue - Fix PrintJobsReportFromDB to strip job-run filters (timestamp, cluster) before querying the prow_jobs table (pre-existing bug) - Add filter.StripJobRunFilters for reusable job-run filter removal - Fix GetTestAnalysisOverallFromDB to use reportEnd instead of time.Now() for consistent date windowing with pinned time (pre-existing bug) Frontend: - Use ISO 8601 strings for timestamp filter values throughout - Remove dead ga_dates timezone hack in App.js - Use valueGetter returning Date objects for DataGrid timestamp columns - Add type: 'date' to all date/timestamp DataGrid columns - Preserve 'not' flag when updating date filter values - Rewrite JobsDetail day bucketing with Temporal.PlainDate - Update DateTimePicker and filter display to use ISO strings Documentation: - Add timestamp/date type guidelines to backend and frontend instructions - Update API docs to show RFC 3339 and YYYY-MM-DD formats Note: production databases should also have their default timezone set to UTC via ALTER DATABASE <name> SET timezone = 'UTC'. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mstaeble
force-pushed
the
trt-2364-timestamp-consistency
branch
from
7536045 to
8906ae3
Compare
Contributor
|
Scheduling required tests: |
Contributor
|
@mstaeble: 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. |
mstaeble
added a commit
to mstaeble/sippy
that referenced
this pull request
Jul 1, 2026
Cherry-pick from PR openshift#3716 with conflict resolution: - Use civil.Date for date-only fields (GA dates, development start dates) - Use TIMESTAMP WITH TIME ZONE and DATE types in PostgreSQL - RFC 3339 for API timestamps, YYYY-MM-DD for dates - Resolve conflict in postgres provider (keep GetReleasesFromDB from PR openshift#3679) - Add time.Time to civil.Date conversion in DefinitionToRelease Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
2 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.
Summary
TIMESTAMP WITH TIME ZONEandDATEtypes across the database schema, API layer, and frontendcivil.Datefor date-only fields (GA dates, development start dates, CountByDate, CalendarEvent)CalendarEvent.Start/Endfromstringtotime.TimeChatConversationResponse.CreatedAtfromstringtotime.TimeJobRun.Timestampfrominttotime.Time(RFC 3339 in JSON)/api/jobsfailed when timestamp filters were present (addedfilter.StripJobRunFilters)JobsDetailday bucketing withTemporal.PlainDateNote: Production databases should also have their default timezone set to UTC via
ALTER DATABASE <name> SET timezone = 'UTC'.Depends on (will need conflict resolution):
ReleaseDefinitionmodel date fields will needcivil.Datetest_analysis.gofunctionsTest plan
make lintpassesmake testpasses (Go + JS + MCP)make verify-apmpassesManual verification against staging-2 (prod database clone)
Ran
sippy migrateandsippy refreshagainst a clone of the production database, then served locally with--data-provider postgres.API responses verified:
/api/jobs/runs- timestamp is RFC 3339 string (was epoch ms integer)/api/releases- ga_dates are YYYY-MM-DD, dates.ga are YYYY-MM-DD, last_updated is RFC 3339/api/jobs/details- start/end are YYYY-MM-DD (civil.Date), JobRunResult.timestamp is RFC 3339/api/tests/analysis/overall- CountByDate.date is YYYY-MM-DD (civil.Date)/api/releases/tags/events- CalendarEvent.start is RFC 3339/api/incidents- CalendarEvent.start/end are RFC 3339UI pages verified:
Suitevariant error with postgres data provider, unrelated to this PR)🤖 Generated with Claude Code
Summary by CodeRabbit
YYYY-MM-DDconsistently across API and UI views.