argo-workflows icon indicating copy to clipboard operation
argo-workflows copied to clipboard

there is no name or name prefix filter in workflows ui

Open tooptoop4 opened this issue 1 year ago • 1 comments

Pre-requisites

  • [X] I have double-checked my configuration
  • [X] I can confirm the issues exists when I tested with :latest
  • [ ] I'd like to contribute the fix myself (see contributing guide)

What happened/what you expected to happen?

v3.4.11 had 'name' and 'name prefix' filters in the 'archived workflows' ui, this is extremely useful to filter on workflows starting with a name or an exact name. particularly if its a sensor generated workflow or you just want to jump to a known workflow name without needing to construct a url in the browser address bar (not user friendly)

v3.5.1 has no such option in the UI!

Version

3.5.1

Paste a small workflow that reproduces the issue. We must be able to run the workflow; don't enter a workflows that uses private images.

n/a

Logs from the workflow controller

kubectl logs -n argo deploy/workflow-controller | grep ${workflow}

Logs from in your workflow's wait container

kubectl logs -n argo -c wait -l workflows.argoproj.io/workflow=${workflow},workflow.argoproj.io/phase!=Succeeded

tooptoop4 avatar Nov 07 '23 04:11 tooptoop4

We also need a filter for archive items. We didn't need it because It was a separated view.

maxisam avatar Dec 27 '23 16:12 maxisam

I can work on this one. Just to make things clear, here's what I can implement :

  • Filtering by workflow name, using Contains, Exact and Prefix. I'll implement as a dropdown to make it lighter on the UI and because it doesn't make sens to have those three at the same time
  • Checkbox filtering by state Active and Archived

Visual preview proposal :

Screenshot 2024-06-03 at 11 08 25 Screenshot 2024-06-03 at 11 10 33

Edit: I added the Contains filtering and the dropdown select options

Adrien-D avatar May 31 '24 09:05 Adrien-D

doable now

This is now doable with the SQLite DB from #13021 / #12736. It previously was not possible for non-archived Workflows as k8s/etcd has no such native filter per https://github.com/argoproj/argo-workflows/issues/12025#issuecomment-1914065370, part 3.

implementation details

archived vs. non-archived filter

naming

@Adrien-D "State" is a bit ambiguous naming; that part we did actually discuss this in https://github.com/argoproj/argo-workflows/issues/12025#issuecomment-1865458377 et al, where I suggested "Workflow Storage" and "In-cluster" + "Archived" as the options. "Active" is a bit ambiguous as well, as was the original terminology of "Live" workflows. "Live" has less connotations around things like the phase of a Workflow though. To be fair, you can have an Archive DB in your cluster too, so "In-cluster" is not necessarily fully precise either, but "etcd" is an implementation detail of k8s and so could be both confusing and imprecise on some distros as well. "k8s resources" sounds a bit confusing user-side as well, but is the most precise 😕

Another option is "Unarchived" + "Archived". That might actually be the least confusing / least ambiguous option 🤔

other impl details

Ideally also the checkboxes shouldn't display if you don't have the Workflow Archive enabled. This is easier said than done as the UI does not currently know this information; consider that a stretch goal and possibly separate follow-on PR as such.

Also, this requires adding a new option to the API method, it is not a UI-only change, as I learned in https://github.com/argoproj/argo-workflows/issues/12025#issuecomment-1914065370 part 1 and others in #12237. The API implementation should roughly follow https://github.com/argoproj/argo-workflows/pull/12237#discussion_r1433459929

name filter

That one looks good to me, I like it. Seems more efficient and less confusing than the original in #6801 as well. Although users will have to learn that the name of the field is clickable to access the dropdown.

agilgur5 avatar Jun 06 '24 17:06 agilgur5

@agilgur5 I see the confusion with "State" and "Active", I'm not very familiar with all of this wording (and this repo yet) so thanks for giving me some details, helps a lot ! "Workflow Storage" "Unarchived" + "Archived" is good for me, it's indeed the less confusing

For the implementation details I'm trying to understand what you're saying. I did some work here on name filter (Didn't start the archived filtering yet) so maybe it can be a good start to discuss my understanding of the implementation

I'm already facing some issues that I need to understand for consistency :

  • Why for workflow templates do we have NamePattern as it's own a query parameter instead of a listOptions ? Should I implement it as it's own query parameters as well ? See here
  • Why for ListWorkflows, CountWorkflows are we forwarding namePrefix as it looks like it's none even used [1] [2]
  • Why for CanI are we forwarding "Name" as we don't seem to use it ?

For the UI I inspired from grafana's interface as they have a good UX on filter in my opinion, hopefully the down arrow will help to understand that the field is clickable

Adrien-D avatar Jun 07 '24 09:06 Adrien-D

Why for ListWorkflows, CountWorkflows are we forwarding namePrefix as it looks like it's none even used [1] [2]

@Adrien-D Thank you for working on this feature. I think I can provide more details about this since I was working on https://github.com/argoproj/argo-workflows/pull/13021 / https://github.com/argoproj/argo-workflows/pull/12736.

Before Unified workflows list UI and API was introduced, we supported listing archived workflow using a name prefix. However, due to some limitation of kubernetes(kubernetes doesn't allow us to list live workflows by using a name prefix), after unifying the APIs, we lost the ability to list by using nameprefix(ref: setting the name prefix to "" for list archived workflows in the unifying PR)

With https://github.com/argoproj/argo-workflows/pull/13021 / https://github.com/argoproj/argo-workflows/pull/12736., we use a sqlite-based in memory store for live workflows, and this provide us a more unified query availability, and we can use SQL queries for both archived and live workflows, thus adding name prefix support becomes possible. and I already unified query experience between live workflow and archived workflow by using name prefix: https://github.com/argoproj/argo-workflows/pull/13021/files#diff-8a497f2f2e0827919658f6a6b4f18e3113cfeb24744b8ac1fd05ccdbf2b1c3b4R89-R157, and that means it is already supported to query both live workflows and archived workflows with a name prefix from database level.

The reason that we pass them as empty is because API doesn't support that. I think in your change, you might need to add namePrefix to the WorkflowListRequest:https://github.com/argoproj/argo-workflows/blob/bda02806cdc27216cd0ce33418997bf5a4215ae1/pkg/apiclient/workflow/workflow.proto#L31-L36 so that API can take the namePrefix and pass to [1] [2]. I didn't do it in https://github.com/argoproj/argo-workflows/pull/13021 because that PR was already too large and I wanted to make it easier for folks to review, also, adding name prefix in both API and UI worths a dedicated PR.

jiachengxu avatar Jun 07 '24 17:06 jiachengxu

  • Why for workflow templates do we have NamePattern as it's own a query parameter instead of a listOptions ? Should I implement it as it's own query parameters as well ? See here

I believe that's because it's using k8s's metav1.ListOptions as the type, not the module's ListOptions type, no?

That actually might be a mistake; it used to be used prior to #2670 as this line was removed in the refactor. (git blame is very useful if you spot something off like this)

If I'm understanding correctly, the name would only make a difference when resourceNames is used, so it may have just happened to work fine in most cases. But I'm not 100% sure if that's exactly how it works, I don't think I've ever tried with a specific name.

Also note that all of this far predates me being a contributor so I am guesstimating a rationale and would not know concretely. Unfortunately that happens a lot with older codebases, context gets lost and newer contributors have to figure it out (so do older contributors, you will forget if you, for instance, didn't write it down in a PR description or somewhere) 😕

For the UI I inspired from grafana's interface as they have a good UX on filter in my opinion, hopefully the down arrow will help to understand that the field is clickable

Related: #11421

agilgur5 avatar Jun 08 '24 07:06 agilgur5

@jiachengxu @agilgur5 Thanks for your answers. I implemented your feedback on my PR https://github.com/argoproj/argo-workflows/pull/13160

I'll make the filtering archived/non-archived WF on a separate PR to make the review easier.

Adrien-D avatar Jun 10 '24 12:06 Adrien-D

I'll make the filtering archived/non-archived WF on a separate PR to make the review easier.

Let's split that out into a separate issue as well and link back to here for reference. This issue does not include it in its title and we have #13151 for native date filtering already as well.

agilgur5 avatar Jun 10 '24 16:06 agilgur5

Archive filtering will be done in this issue https://github.com/argoproj/argo-workflows/issues/13171

Adrien-D avatar Jun 12 '24 07:06 Adrien-D