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

feat: Add name filters for workflow list (exact/prefix/pattern) #13160

Merged
merged 10 commits into from
Oct 17, 2024
Merged

feat: Add name filters for workflow list (exact/prefix/pattern) #13160

merged 10 commits into from
Oct 17, 2024

Conversation

Adrien-D
Copy link
Member

@Adrien-D Adrien-D commented Jun 10, 2024 •
edited
Loading

Fixes #12161

Motivation

We used to have name/prefix filtering for archived workflows before unifying APIs. We need to implement it back.

Modifications

API :

  • Name filtering was already implemented
  • I plugged back the NamePrefix to make it work
  • I added NamePattern the same way prefix is implemented

UI :

  • As its three way of filtering, and no reason to have those three at the same time, I made UX choice with a picker on the label. This is inspired from grafana's UI.

Screenshot 2024-06-10 at 14 40 15
Screenshot 2024-06-10 at 14 40 22

Verification

I tested the feature locally and I added tests for the API.

@Adrien-D Adrien-D mentioned this pull request Jun 10, 2024
Closed
3 tasks
@Adrien-D Adrien-D force-pushed the name-search-workflows branch 2 times, most recently from 05a697b to 66ff9f7 Compare June 10, 2024 14:36
@agilgur5 agilgur5 self-assigned this Jun 10, 2024
@agilgur5 agilgur5 added area/api Argo Server API area/ui labels Jun 10, 2024
agilgur5
agilgur5 previously requested changes Jun 10, 2024
View reviewed changes
Copy link

@agilgur5 agilgur5 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Left several suggestions, simplifications, style fixes, etc in-line below.

Might have a few more after that, particularly on the UI component and CSS changes that I did not check closely on this pass

persist/sqldb/selector.go Outdated Show resolved Hide resolved
server/utils/list_options.go Outdated Show resolved Hide resolved
ui/src/app/shared/utils.ts Outdated Show resolved Hide resolved
ui/src/app/workflows/components/workflow-filters/workflow-filters.tsx Outdated Show resolved Hide resolved
ui/src/app/workflows/components/workflow-filters/workflow-filters.tsx Outdated Show resolved Hide resolved
ui/src/app/workflows/components/workflow-filters/workflow-filters.tsx Outdated Show resolved Hide resolved
ui/src/app/workflows/components/workflows-list/workflows-list.tsx Outdated Show resolved Hide resolved
pkg/apiclient/workflow/workflow.proto Outdated Show resolved Hide resolved
ui/src/app/shared/utils.ts Outdated Show resolved Hide resolved
server/artifacts/artifact_server.go Outdated Show resolved Hide resolved
@agilgur5 agilgur5 added this to the v3.5.x patches milestone Jun 10, 2024
@agilgur5 agilgur5 changed the title feat: Add name filtering for workflows (exact/prefix/pattern) feat: Add name filters for workflow list (exact/prefix/pattern) Jun 10, 2024
jiachengxu
jiachengxu suggested changes Jun 11, 2024
View reviewed changes
pkg/apiclient/workflow/workflow.proto Outdated Show resolved Hide resolved
@Adrien-D Adrien-D requested review from agilgur5 and jiachengxu June 11, 2024 16:31
@Adrien-D
Copy link
Member Author

I fixed my PR with your suggestions. Please note that I updated argo-ui package to last merged commit to be able to remove autoHighlight (one of my other fixed issue) on this particular autocomplete component

@agilgur5
Copy link

Please note that I updated argo-ui package to last merged commit to be able to remove autoHighlight (one of my other fixed issue) on this particular autocomplete component

Mmm I wouldn't include that as part of this PR since it is independent. Like we'd probably want to cherry-pick the argo-ui update/fixes into 3.4, which this PR will ofc not make it into.

Also there isn't an autocomplete component used in this PR?

@Adrien-D Adrien-D force-pushed the name-search-workflows branch from 2b89fed to 05c2890 Compare June 12, 2024 07:36
@Adrien-D
Copy link
Member Author

Adrien-D commented Jun 12, 2024 •
edited
Loading

Also there isn't an autocomplete component used in this PR?

There is, it's the component behind InputFilter used for filtering

But I get your point, we can merge this PR without it and I'll make two other PR to update the argo-ui package, and fix the behavior here

@Adrien-D Adrien-D force-pushed the name-search-workflows branch from 05c2890 to 383c782 Compare June 12, 2024 08:25
agilgur5
agilgur5 reviewed Jun 13, 2024
View reviewed changes
Copy link

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

A few more comments on the filters below.

I haven't had time to take a look at the "contains" wording or CSS since I'm on vacation for the rest of the week and not at my computer

ui/src/app/workflows/components/workflows-list/workflows-list.tsx Outdated Show resolved Hide resolved
ui/src/app/workflows/components/workflows-list/workflows-list.tsx Outdated Show resolved Hide resolved
ui/src/app/workflows/components/workflows-list/workflows-list.tsx Outdated Show resolved Hide resolved
ui/src/app/workflows/components/workflows-list/workflows-list.tsx Outdated Show resolved Hide resolved
ui/src/app/workflows/components/workflows-list/workflows-list.tsx Outdated Show resolved Hide resolved
pkg/apiclient/workflow/workflow.proto Outdated Show resolved Hide resolved
ui/src/app/workflows/components/workflow-filters/workflow-filters.tsx Outdated Show resolved Hide resolved
@Adrien-D Adrien-D force-pushed the name-search-workflows branch 2 times, most recently from a01131b to b09b28e Compare June 30, 2024 10:16
@Adrien-D Adrien-D requested a review from agilgur5 June 30, 2024 11:22
@Adrien-D Adrien-D force-pushed the name-search-workflows branch from 738268c to 9ca5b05 Compare July 15, 2024 08:28
@Adrien-D Adrien-D mentioned this pull request Jul 31, 2024
Closed
6 tasks
@Adrien-D
Copy link
Member Author

@agilgur5 are you still waiting some work from me on this one ?

@Adrien-D Adrien-D force-pushed the name-search-workflows branch 3 times, most recently from 5e330b5 to 56ed817 Compare October 3, 2024 16:11
@Adrien-D
feat: Add NamePrefix and NamePattern filtering to workflows
1cf13de 
Signed-off-by: Adrien Delannoy <[email protected]>
@Adrien-D
feat: Add generated files
83e241e 
Signed-off-by: Adrien Delannoy <[email protected]>
@Adrien-D
feat: UI add name/prefix/pattern to filter worflows
c518906 
Signed-off-by: Adrien Delannoy <[email protected]>
@Adrien-D
Revert "fix: Remove useless CanI name parameter"
5b7336f 
This reverts commit 8cbb262.

Signed-off-by: Adrien Delannoy <[email protected]>
@Adrien-D
feat: simply name filtering on UI & fix API proto
d8efce3 
Signed-off-by: Adrien Delannoy <[email protected]>
@Adrien-D
feat: generated files
bf5cfc9 
Signed-off-by: Adrien Delannoy <[email protected]>
@Adrien-D
fix: use nameFilter to specify name filtering type
f95914a 
Signed-off-by: Adrien Delannoy <[email protected]>
@Adrien-D
feat: update autoHighlight to false
7710e8c 
Signed-off-by: Adrien Delannoy <[email protected]>
@Adrien-D
fix: linting & pre-commit
4f15dee 
Signed-off-by: Adrien Delannoy <[email protected]>
@Adrien-D Adrien-D force-pushed the name-search-workflows branch from 56ed817 to 4f15dee Compare October 3, 2024 16:24
@Joibel Joibel dismissed agilgur5’s stale review October 17, 2024 09:40

Comments have been addressed

@Joibel Joibel merged commit d8f2d85 into argoproj:main Oct 17, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tooptoop4 tooptoop4 tooptoop4 approved these changes

@Joibel Joibel Joibel approved these changes

@terrytangyuan terrytangyuan Awaiting requested review from terrytangyuan terrytangyuan is a code owner

@sarabala1979 sarabala1979 Awaiting requested review from sarabala1979 sarabala1979 is a code owner

@jiachengxu jiachengxu Awaiting requested review from jiachengxu

@agilgur5 agilgur5 Awaiting requested review from agilgur5

Assignees

@agilgur5 agilgur5

Labels
area/api Argo Server API area/ui
Projects
None yet
Milestone
v3.5.x patches
Development

Successfully merging this pull request may close these issues.

UI: 3.5 missing name + name prefix filters for archived workflows
5 participants
@Adrien-D @agilgur5 @Joibel @jiachengxu @tooptoop4