Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(frontend): fixes filter pipeline text box shows error when typing anything in it. Fixes #10241 #11096

Merged
merged 3 commits into from
Sep 3, 2024
Merged

fix(frontend): fixes filter pipeline text box shows error when typing anything in it. Fixes #10241 #11096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
403f4d5
Filter pipeline text box shows error when typing anything in it #10241
Aug 13, 2024
31ccd0a
Filter pipeline text box shows error when typing anything in it #10241
Aug 13, 2024
037739c
Merge branch 'Filter-pipeline-text-box-shows-error-when-typing-anythi…
Aug 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion frontend/src/pages/ResourceSelector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class ResourceSelector extends React.Component<ResourceSelectorProps, ResourceSe
{title && <Toolbar actions={toolbarActionMap} breadcrumbs={[]} pageTitle={title} />}

<CustomTable
isCalledByV1={true}
isCalledByV1={false}
columns={columns}
rows={rows}
selectedIds={selectedIds}
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/pages/__snapshots__/ResourceSelector.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ exports[`ResourceSelector displays resource selector 1`] = `
emptyMessage="Test - Sorry, no resources."
filterLabel="test filter label"
initialSortColumn="created_at"
isCalledByV1={true}
isCalledByV1={false}
Copy link
Collaborator

@HumairAK HumairAK Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this does work for listing v2 pipelines, I'm guessing it breaks it for v1 pipelines. I'm trying to wrap my head around why this isCalledByV1 exists, because you can either listPipelines based off v1 api or v2 api, but not both, yet we can have v1/v2 pipelines uploaded together in KFP, which makes me think that we actually don't properly support listing/filtering for both kfp v1 and v2 pipelines together

@zijianjoy does that sound right to you?

either way I would rather we have this work with v2 rather than v1, and discuss a better design approach on how to handle this case for when we have both pipelines uploaded and need to list/filter simultaneously

@zijianjoy wdyt?

Copy link
Collaborator

@zijianjoy zijianjoy Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isCalledByV1 was introduced to differentiate API request payload schema between V1 API and V2 API.

pipelines/frontend/src/components/CustomTable.tsx

Lines 229 to 246 in 1c7954d

this.state = {
currentPage: 0,
filterString: this.props.initialFilterString || '',
filterStringEncoded:
this.props.initialFilterString && this.props.isCalledByV1
? this._createAndEncodeFilterV1(this.props.initialFilterString)
: this.props.initialFilterString
? this._createAndEncodeFilterV2(this.props.initialFilterString)
: '',
isBusy: false,
maxPageIndex: Number.MAX_SAFE_INTEGER,
pageSize: 10,
sortBy:
props.initialSortColumn || (props.columns.length ? props.columns[0].sortKey || '' : ''),
sortOrder: props.initialSortOrder || 'desc',
tokenList: [''],
};
}
. Because we have a flag to control whether to show v2 UI by default during the initial migration from v1 to v2.

Ideally v2 should support filer for V1 names filtering as well. So it makes sense to me for switching to V2 filter mechanism.

reload={[Function]}
rows={
Array [
Expand Down
Loading