-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
feat: Add name filters for workflow list (exact/prefix/pattern) #13160
base: main
Are you sure you want to change the base?
Conversation
05a697b
to
66ff9f7
Compare
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.
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
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
I fixed my PR with your suggestions. Please note that I updated |
Mmm I wouldn't include that as part of this PR since it is independent. Like we'd probably want to cherry-pick the Also there isn't an autocomplete component used in this PR? |
Signed-off-by: Adrien Delannoy <[email protected]>
Signed-off-by: Adrien Delannoy <[email protected]>
Signed-off-by: Adrien Delannoy <[email protected]>
Signed-off-by: Adrien Delannoy <[email protected]>
This reverts commit 8cbb262. Signed-off-by: Adrien Delannoy <[email protected]>
2b89fed
to
05c2890
Compare
There is, it's the component behind But I get your point, we can merge this PR without it and I'll make two other PR to update the |
Signed-off-by: Adrien Delannoy <[email protected]>
Signed-off-by: Adrien Delannoy <[email protected]>
05c2890
to
383c782
Compare
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.
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
// Return workflows having this name prefix | ||
string namePrefix = 4; | ||
// Return workflows with name containing this value | ||
string namePattern = 5; |
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.
I'm wondering if we should make the API match the UI actually; name
as a string and nameFilter
as an enum that defaults to "Exact"
Right now, it's not clear that these are mutually exclusive. We could add that in the comments, but having it only allow one at a time feels much more explicit
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.
Yeah I like that idea, so we could have nameFilter
as Exact
| Contains
| Prefix
, and then nameValue
as string
I'm not familiar with the use case, but does it make more sense to have Exact
as default instead of Contains
?
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.
Exact
as default would be backward compatible, since previously it was the only usage of name
.
We don't have to keep that default in the UI, but it would be necessary in the API for backward compat
ui/src/app/workflows/components/workflow-filters/workflow-filters.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Adrien Delannoy <[email protected]>
Fixes #12161 1/2 (I'll make a PR for filtering archived/non-archived workflows on a seperate PR)
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 implementedNamePrefix
to make it workNamePattern
the same way prefix is implementedUI :
Verification
I tested the feature locally and I added tests for the API.