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.

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: Unified workflows list UI and API #11121

Merged
merged 27 commits into from
Jul 10, 2023

Conversation

terrytangyuan
Copy link
Member

@terrytangyuan terrytangyuan commented May 23, 2023

Fixes #10781.

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
@terrytangyuan terrytangyuan changed the title feat: Unified workflows and archived workflows UI/API feat: Unified workflows list UI and API May 24, 2023
@terrytangyuan
Copy link
Member Author

For workflow deletion, we need to delete workflow in K8s cluster and in the DB. Here's what I propose we should do:

  1. When deleting a workflow, we always delete the workflow in the K8s cluster.
  2. In addition, there's a popup in the UI (when clicking the delete button) where the user can select whether to delete the workflow in the DB.

For the backend, we can choose either of the following:

  1. Keep the existing behavior of workflowServer.DeleteWorkflow() and UI should call both workflowServer.DeleteWorkflow() and archivedWorkflowServer.DeleteWorkflow().
  2. Add an argument deleteArchived bool to workflowServer.DeleteWorkflow(). If deleteArchived=true, workflowServer.DeleteWorkflow() will also call archivedWorkflowServer.DeleteWorkflow() under the hood.

Any thoughts? @jessesuen @sarabala1979

@terrytangyuan
Copy link
Member Author

There should also be a setting (e.g. environment variable) to enable/disable the popup.

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
@sarabala1979
Copy link
Member

we need to delete workflow in K8s cluster and in the DB

For workflow deletion, we need to delete workflow in K8s cluster and in the DB. Here's what I propose we should do:

  1. When deleting a workflow, we always delete the workflow in the K8s cluster.
  2. In addition, there's a popup in the UI (when clicking the delete button) where the user can select whether to delete the workflow in the DB.

For the backend, we can choose either of the following:

  1. Keep the existing behavior of workflowServer.DeleteWorkflow() and UI should call both workflowServer.DeleteWorkflow() and archivedWorkflowServer.DeleteWorkflow().
  2. Add an argument deleteArchived bool to workflowServer.DeleteWorkflow(). If deleteArchived=true, workflowServer.DeleteWorkflow() will also call archivedWorkflowServer.DeleteWorkflow() under the hood.

Any thoughts? @jessesuen @sarabala1979

I like it.
if the workflow is in both. a popup will show the checkboxdo you want to delete in the archive

if the workflow is in one place(either k8s/DB), just delete it

Are you going to show the icon in the workflow list archived?

@sarabala1979 sarabala1979 marked this pull request as ready for review June 1, 2023 14:56
@terrytangyuan terrytangyuan marked this pull request as draft June 1, 2023 15:07
@terrytangyuan
Copy link
Member Author

Discussed with Bala, for the backend we'll use this approach:

Keep the existing behavior of workflowServer.DeleteWorkflow() and UI should call both workflowServer.DeleteWorkflow() and archivedWorkflowServer.DeleteWorkflow().

Are you going to show the icon in the workflow list archived?

Right now it's text "true/false" in the column. The icon doesn't look good in the column and the meaning of the icon is not intuitive.

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
@terrytangyuan terrytangyuan marked this pull request as ready for review June 11, 2023 01:10
Copy link
Member

@rbreeze rbreeze left a comment

Choose a reason for hiding this comment

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

LGTM! Agree with you that global var UI solution isn't ideal, but for now works and shouldn't have side effects.

Copy link
Member

@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.

Added some back-links to fixes for issues here for posterity

persist/sqldb/workflow_archive.go Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

This seems to have no JSX in it? Not sure why it was renamed

initialPodName={podName}
nodeId={parsedSidePanel.nodeId}
container={parsedSidePanel.container}
archived={isArchivedWorkflow(workflow)}
Copy link
Member

Choose a reason for hiding this comment

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

this appears to cause a subtle regression for logs for Workflows that are both live and archived (e.g. ones that have not TTL'd yet): #12948 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify the workflow + workflow archive UI/API
6 participants