-
Notifications
You must be signed in to change notification settings - Fork 31
SRVKP-12328: Add date range filter for PipelineRuns list page #1133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ export interface CheckboxFilterConfig { | |
| placeholder?: string; | ||
| options: FilterOption[]; | ||
| defaultValues?: string[]; | ||
| singleSelect?: boolean; | ||
| } | ||
|
|
||
| export interface FilterValues { | ||
|
|
@@ -73,6 +74,11 @@ const CheckboxFilterInput: FC<{ | |
|
|
||
| const onMenuSelect = (_event: unknown, itemId: string | number) => { | ||
| const id = String(itemId); | ||
| if (config.singleSelect) { | ||
| onSelect([id]); | ||
| setIsOpen(false); | ||
| return; | ||
| } | ||
| const newSelected = selected.includes(id) | ||
| ? selected.filter((s) => s !== id) | ||
| : [...selected, id]; | ||
|
|
@@ -110,14 +116,16 @@ const CheckboxFilterInput: FC<{ | |
| onClick={handleToggleClick} | ||
| isExpanded={isOpen} | ||
| isFullWidth | ||
| icon={<FilterIcon />} | ||
| icon={config.singleSelect ? undefined : <FilterIcon />} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need to pass |
||
| badge={ | ||
| selected.length > 0 ? ( | ||
| !config.singleSelect && selected.length > 0 ? ( | ||
| <Badge isRead>{selected.length}</Badge> | ||
| ) : undefined | ||
| } | ||
| > | ||
| {config.placeholder || config.title} | ||
| {config.singleSelect && selected.length > 0 | ||
| ? config.options.find((o) => o.value === selected[0])?.label | ||
| : config.placeholder || config.title} | ||
| </MenuToggle> | ||
| } | ||
| triggerRef={toggleRef} | ||
|
|
@@ -134,15 +142,17 @@ const CheckboxFilterInput: FC<{ | |
| <MenuItem | ||
| key={option.value} | ||
| itemId={option.value} | ||
| hasCheckbox | ||
| hasCheckbox={!config.singleSelect} | ||
| isSelected={selected.includes(option.value)} | ||
| > | ||
| {option.label} | ||
| <Badge isRead className="pf-v6-u-ml-sm"> | ||
| {option.totalCount !== undefined | ||
| ? `${option.count}/${option.totalCount}` | ||
| : option.count} | ||
| </Badge> | ||
| {!config.singleSelect && ( | ||
| <Badge isRead className="pf-v6-u-ml-sm"> | ||
| {option.totalCount !== undefined | ||
| ? `${option.count}/${option.totalCount}` | ||
| : option.count} | ||
| </Badge> | ||
| )} | ||
| </MenuItem> | ||
| ))} | ||
| </MenuList> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| import { | ||
| useFlag, | ||
| useUserPreference, | ||
| } from '@openshift-console/dynamic-plugin-sdk'; | ||
| import { testHook } from '../../../test-data/utils/hooks-utils'; | ||
| import { useDateRangeFilter } from '../useDateRangeFilter'; | ||
|
|
||
| jest.mock('@openshift-console/dynamic-plugin-sdk', () => ({ | ||
| useFlag: jest.fn(), | ||
| useUserPreference: jest.fn(), | ||
| })); | ||
|
|
||
| const useFlagMock = useFlag as jest.Mock; | ||
| const useUserPreferenceMock = useUserPreference as jest.Mock; | ||
|
|
||
| const ONE_DAY_MS = 86400000; | ||
| const ONE_WEEK_MS = 604800000; | ||
|
|
||
| describe('useDateRangeFilter', () => { | ||
| let setTimespanMock: jest.Mock; | ||
|
|
||
| beforeEach(() => { | ||
| setTimespanMock = jest.fn(); | ||
| useFlagMock.mockReturnValue(true); | ||
| useUserPreferenceMock.mockReturnValue([ONE_DAY_MS, setTimespanMock]); | ||
| }); | ||
|
|
||
| it('should return timespan from user preference', () => { | ||
| const { result } = testHook(() => useDateRangeFilter()); | ||
| expect(result.current.timespan).toBe(ONE_DAY_MS); | ||
| }); | ||
|
|
||
| it('should compute startDate as Date.now() - timespan', () => { | ||
| const now = Date.now(); | ||
| const { result } = testHook(() => useDateRangeFilter()); | ||
| const diff = now - result.current.timespan; | ||
| expect(result.current.startDate).toBeGreaterThanOrEqual(diff - 100); | ||
| expect(result.current.startDate).toBeLessThanOrEqual(diff + 100); | ||
| }); | ||
|
|
||
| it('should generate a valid CEL expression', () => { | ||
| const { result } = testHook(() => useDateRangeFilter()); | ||
| expect(result.current.dateFilterCEL).toMatch( | ||
| /^data\.status\.startTime > timestamp\(".*"\)$/, | ||
| ); | ||
| }); | ||
|
|
||
| it('should default to 1 day when useUserPreference returns undefined', () => { | ||
| useUserPreferenceMock.mockReturnValue([undefined, setTimespanMock]); | ||
| const { result } = testHook(() => useDateRangeFilter()); | ||
| expect(result.current.timespan).toBe(ONE_DAY_MS); | ||
| expect(result.current.dateFilterCEL).not.toBe(''); | ||
| }); | ||
|
|
||
| it('should expose setTimespan from the preference hook', () => { | ||
| const { result } = testHook(() => useDateRangeFilter()); | ||
| result.current.setTimespan(ONE_WEEK_MS); | ||
| expect(setTimespanMock).toHaveBeenCalledWith(ONE_WEEK_MS); | ||
| }); | ||
|
|
||
| it('should reflect the isTektonResultEnabled flag', () => { | ||
| useFlagMock.mockReturnValue(false); | ||
| const { result } = testHook(() => useDateRangeFilter()); | ||
| expect(result.current.isTektonResultEnabled).toBe(false); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import { useMemo } from 'react'; | ||
| import { | ||
| useFlag, | ||
| useUserPreference, | ||
| } from '@openshift-console/dynamic-plugin-sdk'; | ||
| import { FLAG_PIPELINE_TEKTON_RESULT_INSTALLED } from '../../consts'; | ||
| import { parsePrometheusDuration } from '../pipelines-overview/dateTime'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are we using this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The name is misleading (it's borrowed from Prometheus-style duration format), but the function itself is just string to milliseconds conversion.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @adityavshinde
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should I import the unit constants directly from
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can create your own helper method which is more readable and maintainable
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We should update this probably, if we do this what essentially happens is it does not compute the whole of the earlier day, example |
||
|
|
||
| export type DateRangeFilterResult = { | ||
| timespan: number; | ||
| setTimespan: (ms: number) => void; | ||
| startDate: number | undefined; | ||
| dateFilterCEL: string; | ||
| isTektonResultEnabled: boolean; | ||
| preferenceLoaded: boolean; | ||
| }; | ||
|
|
||
| const SETTINGS_KEY = 'plugin__pipelines-console-plugin.dateRangeFilter'; | ||
|
|
||
| export const useDateRangeFilter = (): DateRangeFilterResult => { | ||
| const isTektonResultEnabled = useFlag(FLAG_PIPELINE_TEKTON_RESULT_INSTALLED); | ||
| const [timespan, setTimespan, preferenceLoaded] = useUserPreference<number>( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if this api fails ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If this api fails, then it falls back to the default value which is "Last day". This will not throw an error as it is defined in SDK
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suppressing the error makes it harder to debug when an issue comes in later, so we will need to change this
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also adding this in the doc in detail.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update |
||
| SETTINGS_KEY, | ||
| parsePrometheusDuration('1d'), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are we setting 1d as default ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes |
||
| true, | ||
| ); | ||
|
|
||
| const ts = timespan ?? parsePrometheusDuration('1d'); | ||
|
|
||
| const startDate = useMemo(() => { | ||
| if (!ts) return undefined; | ||
| return Date.now() - ts; | ||
| }, [ts]); | ||
|
|
||
| const dateFilterCEL = useMemo(() => { | ||
| if (!startDate) return ''; | ||
| return `data.status.startTime > timestamp("${new Date( | ||
| startDate, | ||
| ).toISOString()}")`; | ||
| }, [startDate]); | ||
|
|
||
| return { | ||
| timespan: ts, | ||
| setTimespan, | ||
| startDate, | ||
| dateFilterCEL, | ||
| isTektonResultEnabled, | ||
| preferenceLoaded, | ||
| }; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ import { | |
| } from '../../models'; | ||
| import { ApprovalTaskKind, PipelineRunKind, TaskRunKind } from '../../types'; | ||
| import { useDeepCompareMemoize } from '../utils/common-utils'; | ||
| import { EQ } from '../utils/tekton-results'; | ||
| import { AND, EQ } from '../utils/tekton-results'; | ||
| import { useMultiClusterProxyService } from './useMultiClusterProxyService'; | ||
| import { useMultiClusterTaskRuns } from './useMultiClusterTaskRuns'; | ||
| import { useTRRuns } from './useTektonResults'; | ||
|
|
@@ -191,12 +191,14 @@ export const usePipelineRuns = ( | |
| selector?: Selector; | ||
| limit?: number; | ||
| name?: string; | ||
| filter?: string; | ||
| }, | ||
| ): [PipelineRunKind[], boolean, boolean, Error | undefined, boolean, boolean] => | ||
| useRuns<PipelineRunKind>(PIPELINE_RUN_GVK, namespace, { | ||
| selector: options?.selector, | ||
| limit: options?.limit /* similar to one present in UseTaskRunsOptions */, | ||
| name: options?.name /* similar to one present in UseTaskRunsOptions */, | ||
| filter: options?.filter, | ||
| }); | ||
|
|
||
| export const useRuns = <Kind extends K8sResourceKind>( | ||
|
|
@@ -207,6 +209,7 @@ export const useRuns = <Kind extends K8sResourceKind>( | |
| limit?: number; | ||
| name?: string; | ||
| skipFetch?: boolean; | ||
| filter?: string; // CEL expression sent to Tekton Results to retrieve PRs within the date range | ||
| }, | ||
| pipelineRunFinished?: boolean, | ||
| pipelineRunManagedBy?: string, | ||
|
|
@@ -315,10 +318,10 @@ export const useRuns = <Kind extends K8sResourceKind>( | |
|
|
||
| const trOptions: typeof optionsMemo = useMemo(() => { | ||
| if (optionsMemo?.name) { | ||
| const { name, ...rest } = optionsMemo; | ||
| const { name, filter, ...rest } = optionsMemo; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this needs to be rebased with master branch
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this rebased yet ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I have rebased this. Now making changes accordingly. |
||
| return { | ||
| ...rest, | ||
| filter: EQ('data.metadata.name', name), | ||
| filter: AND(EQ('data.metadata.name', name), filter), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the expected format for the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's a CEL string for the Tekton Results API. The format looks like: This is generated by
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please add verification
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| }; | ||
| } | ||
| return optionsMemo; | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the purpose of single select
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing
DataViewFilterToolbaronly supported checkbox-style filters where users can select multiple values (e.g., selecting both "Succeeded" and "Failed" statuses). But for the date range filter, selecting multiple time ranges simultaneously doesn't make sense.