From 234b09f85964c57d7e516e694b9ede209bc585ad Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Fri, 19 May 2023 15:56:31 -0400 Subject: [PATCH 01/27] feat: wip Signed-off-by: Yuan Tang --- server/workflow/workflow_server.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go index b465e411f97e..4bc245d113d6 100644 --- a/server/workflow/workflow_server.go +++ b/server/workflow/workflow_server.go @@ -20,6 +20,7 @@ import ( workflowpkg "github.com/argoproj/argo-workflows/v3/pkg/apiclient/workflow" workflowarchivepkg "github.com/argoproj/argo-workflows/v3/pkg/apiclient/workflowarchive" "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow" + "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1" wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1" "github.com/argoproj/argo-workflows/v3/pkg/client/clientset/versioned" "github.com/argoproj/argo-workflows/v3/server/auth" @@ -128,6 +129,15 @@ func (s *workflowServer) GetWorkflow(ctx context.Context, req *workflowpkg.Workf return wf, nil } +func containsWorkflow(s v1alpha1.WorkflowList, e v1alpha1.Workflow) bool { + for _, wf := range s.Items { + if wf.UID == e.UID { + return true + } + } + return false +} + func (s *workflowServer) ListWorkflows(ctx context.Context, req *workflowpkg.WorkflowListRequest) (*wfv1.WorkflowList, error) { wfClient := auth.GetWfClient(ctx) @@ -140,6 +150,16 @@ func (s *workflowServer) ListWorkflows(ctx context.Context, req *workflowpkg.Wor if err != nil { return nil, sutils.ToStatusError(err, codes.Internal) } + archivedWfList, err := s.wfArchiveServer.ListArchivedWorkflows(ctx, &workflowarchivepkg.ListArchivedWorkflowsRequest{ + ListOptions: listOption, + NamePrefix: "", + Namespace: req.Namespace, + }) + for _, item := range archivedWfList.Items { + if !containsWorkflow(*wfList, item) { + wfList.Items = append(wfList.Items, item) + } + } cleaner := fields.NewCleaner(req.Fields) if s.offloadNodeStatusRepo.IsEnabled() && !cleaner.WillExclude("items.status.nodes") { offloadedNodes, err := s.offloadNodeStatusRepo.List(req.Namespace) From 86c41023716dbb0c3c4e7874c59d92c2085764c9 Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Mon, 22 May 2023 10:36:09 -0400 Subject: [PATCH 02/27] fix: return complete wfs Signed-off-by: Yuan Tang --- persist/sqldb/workflow_archive.go | 25 ++++++++----------------- server/workflow/workflow_server.go | 3 +++ 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/persist/sqldb/workflow_archive.go b/persist/sqldb/workflow_archive.go index 35251f1ac3fe..984d72fb0add 100644 --- a/persist/sqldb/workflow_archive.go +++ b/persist/sqldb/workflow_archive.go @@ -8,9 +8,7 @@ import ( log "github.com/sirupsen/logrus" "google.golang.org/grpc/codes" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/types" "upper.io/db.v3" "upper.io/db.v3/lib/sqlbuilder" @@ -144,7 +142,7 @@ func (r *workflowArchive) ArchiveWorkflow(wf *wfv1.Workflow) error { } func (r *workflowArchive) ListWorkflows(namespace string, name string, namePrefix string, minStartedAt, maxStartedAt time.Time, labelRequirements labels.Requirements, limit int, offset int) (wfv1.Workflows, error) { - var archivedWfs []archivedWorkflowMetadata + var archivedWfs []archivedWorkflowRecord clause, err := labelsClause(r.dbType, labelRequirements) if err != nil { return nil, err @@ -158,7 +156,7 @@ func (r *workflowArchive) ListWorkflows(namespace string, name string, namePrefi } err = r.session. - Select("name", "namespace", "uid", "phase", "startedat", "finishedat"). + Select("workflow"). From(archiveTableName). Where(r.clusterManagedNamespaceAndInstanceID()). And(namespaceEqual(namespace)). @@ -174,20 +172,13 @@ func (r *workflowArchive) ListWorkflows(namespace string, name string, namePrefi return nil, err } wfs := make(wfv1.Workflows, len(archivedWfs)) - for i, md := range archivedWfs { - wfs[i] = wfv1.Workflow{ - ObjectMeta: v1.ObjectMeta{ - Name: md.Name, - Namespace: md.Namespace, - UID: types.UID(md.UID), - CreationTimestamp: v1.Time{Time: md.StartedAt}, - }, - Status: wfv1.WorkflowStatus{ - Phase: md.Phase, - StartedAt: v1.Time{Time: md.StartedAt}, - FinishedAt: v1.Time{Time: md.FinishedAt}, - }, + for i, archivedWf := range archivedWfs { + wf := wfs[i] + err = json.Unmarshal([]byte(archivedWf.Workflow), &wf) + if err != nil { + return nil, err } + wfs[i] = wf } return wfs, nil } diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go index 4bc245d113d6..c15032257e90 100644 --- a/server/workflow/workflow_server.go +++ b/server/workflow/workflow_server.go @@ -155,6 +155,9 @@ func (s *workflowServer) ListWorkflows(ctx context.Context, req *workflowpkg.Wor NamePrefix: "", Namespace: req.Namespace, }) + if err != nil { + return nil, sutils.ToStatusError(err, codes.Internal) + } for _, item := range archivedWfList.Items { if !containsWorkflow(*wfList, item) { wfList.Items = append(wfList.Items, item) From 0ab6747125f29b9444e1877d127d774e1fee54d8 Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Mon, 22 May 2023 15:36:42 -0400 Subject: [PATCH 03/27] fix: add archived column Signed-off-by: Yuan Tang --- .../app/workflows/components/workflows-list/workflows-list.tsx | 1 + ui/src/app/workflows/components/workflows-row/workflows-row.tsx | 1 + 2 files changed, 2 insertions(+) diff --git a/ui/src/app/workflows/components/workflows-list/workflows-list.tsx b/ui/src/app/workflows/components/workflows-list/workflows-list.tsx index 9c5a9589e5ec..3651a4f5c1bf 100644 --- a/ui/src/app/workflows/components/workflows-list/workflows-list.tsx +++ b/ui/src/app/workflows/components/workflows-list/workflows-list.tsx @@ -380,6 +380,7 @@ export class WorkflowsList extends BasePage, State> {
PROGRESS
MESSAGE
DETAILS
+
ARCHIVED
{(this.state.columns || []).map(col => { return (
diff --git a/ui/src/app/workflows/components/workflows-row/workflows-row.tsx b/ui/src/app/workflows/components/workflows-row/workflows-row.tsx index 6e24d1f1fbb4..7f93d42dd07b 100644 --- a/ui/src/app/workflows/components/workflows-row/workflows-row.tsx +++ b/ui/src/app/workflows/components/workflows-row/workflows-row.tsx @@ -87,6 +87,7 @@ export class WorkflowsRow extends React.Component
+
{(wf.metadata.labels && wf.metadata.labels['workflows.argoproj.io/workflow-archiving-status'] === 'Archived') || '-'}
{(this.props.columns || []).map(column => { const value = wf.metadata.labels[column.key]; return ( From 2988184fbbba6960182ae74c5a529e10b0340aed Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Mon, 22 May 2023 15:53:48 -0400 Subject: [PATCH 04/27] fix: true or false Signed-off-by: Yuan Tang --- .../app/workflows/components/workflows-row/workflows-row.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ui/src/app/workflows/components/workflows-row/workflows-row.tsx b/ui/src/app/workflows/components/workflows-row/workflows-row.tsx index 7f93d42dd07b..cf0bf950bb35 100644 --- a/ui/src/app/workflows/components/workflows-row/workflows-row.tsx +++ b/ui/src/app/workflows/components/workflows-row/workflows-row.tsx @@ -87,7 +87,9 @@ export class WorkflowsRow extends React.Component -
{(wf.metadata.labels && wf.metadata.labels['workflows.argoproj.io/workflow-archiving-status'] === 'Archived') || '-'}
+
+ {wf.metadata.labels && wf.metadata.labels['workflows.argoproj.io/workflow-archiving-status'] === 'Archived' ? 'true' : 'false'} +
{(this.props.columns || []).map(column => { const value = wf.metadata.labels[column.key]; return ( From 3d9185e1266bc1fd16a7ba4d0ca54123f0f910cc Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Mon, 22 May 2023 16:20:10 -0400 Subject: [PATCH 05/27] fix: remove archived-wf from UI Signed-off-by: Yuan Tang --- ui/src/app/app-router.tsx | 8 - .../archived-workflow-container.tsx | 11 - .../archived-workflow-details.tsx | 315 ------------------ .../archived-workflow-filters.scss | 28 -- .../archived-workflow-filters.tsx | 206 ------------ .../archived-workflow-list.tsx | 312 ----------------- ui/src/app/archived-workflows/index.ts | 5 - ui/src/app/reports/components/reports.tsx | 74 +--- .../services/archived-workflows-service.ts | 63 ---- ui/src/app/shared/services/index.ts | 3 - 10 files changed, 18 insertions(+), 1007 deletions(-) delete mode 100644 ui/src/app/archived-workflows/components/archived-workflow-container.tsx delete mode 100644 ui/src/app/archived-workflows/components/archived-workflow-details/archived-workflow-details.tsx delete mode 100644 ui/src/app/archived-workflows/components/archived-workflow-filters/archived-workflow-filters.scss delete mode 100644 ui/src/app/archived-workflows/components/archived-workflow-filters/archived-workflow-filters.tsx delete mode 100644 ui/src/app/archived-workflows/components/archived-workflow-list/archived-workflow-list.tsx delete mode 100644 ui/src/app/archived-workflows/index.ts delete mode 100644 ui/src/app/shared/services/archived-workflows-service.ts diff --git a/ui/src/app/app-router.tsx b/ui/src/app/app-router.tsx index 3db7e2d811d0..52c8933681c1 100644 --- a/ui/src/app/app-router.tsx +++ b/ui/src/app/app-router.tsx @@ -6,7 +6,6 @@ import {useEffect, useState} from 'react'; import {Redirect, Route, Router, Switch} from 'react-router'; import {Version} from '../models'; import apidocs from './apidocs'; -import archivedWorkflows from './archived-workflows'; import clusterWorkflowTemplates from './cluster-workflow-templates'; import cronWorkflows from './cron-workflows'; import eventflow from './event-flow'; @@ -35,7 +34,6 @@ const workflowsEventBindingsUrl = uiUrl('workflow-event-bindings'); const workflowTemplatesUrl = uiUrl('workflow-templates'); const clusterWorkflowTemplatesUrl = uiUrl('cluster-workflow-templates'); const cronWorkflowsUrl = uiUrl('cron-workflows'); -const archivedWorkflowsUrl = uiUrl('archived-workflows'); const eventSourceUrl = uiUrl('event-sources'); const pluginsUrl = uiUrl('plugins'); const helpUrl = uiUrl('help'); @@ -130,11 +128,6 @@ export const AppRouter = ({popupManager, history, notificationsManager}: {popupM path: workflowsEventBindingsUrl + namespaceSuffix, iconClassName: 'fa fa-link' }, - { - title: 'Archived Workflows', - path: archivedWorkflowsUrl + namespaceSuffix, - iconClassName: 'fa fa-archive' - }, { title: 'Reports', path: reportsUrl + namespaceSuffix, @@ -176,7 +169,6 @@ export const AppRouter = ({popupManager, history, notificationsManager}: {popupM - diff --git a/ui/src/app/archived-workflows/components/archived-workflow-container.tsx b/ui/src/app/archived-workflows/components/archived-workflow-container.tsx deleted file mode 100644 index 5b7b63e7e012..000000000000 --- a/ui/src/app/archived-workflows/components/archived-workflow-container.tsx +++ /dev/null @@ -1,11 +0,0 @@ -import * as React from 'react'; -import {Route, RouteComponentProps, Switch} from 'react-router'; -import {ArchivedWorkflowDetails} from './archived-workflow-details/archived-workflow-details'; -import {ArchivedWorkflowList} from './archived-workflow-list/archived-workflow-list'; - -export const ArchivedWorkflowContainer = (props: RouteComponentProps) => ( - - - - -); diff --git a/ui/src/app/archived-workflows/components/archived-workflow-details/archived-workflow-details.tsx b/ui/src/app/archived-workflows/components/archived-workflow-details/archived-workflow-details.tsx deleted file mode 100644 index d88a98b6646d..000000000000 --- a/ui/src/app/archived-workflows/components/archived-workflow-details/archived-workflow-details.tsx +++ /dev/null @@ -1,315 +0,0 @@ -import {NotificationType, Page, SlidingPanel} from 'argo-ui'; -import * as classNames from 'classnames'; -import * as React from 'react'; -import {useContext, useEffect, useState} from 'react'; -import {RouteComponentProps} from 'react-router'; -import {ArtifactRepository, execSpec, Link, Workflow} from '../../../../models'; -import {artifactRepoHasLocation, findArtifact} from '../../../shared/artifacts'; -import {uiUrl} from '../../../shared/base'; -import {ErrorNotice} from '../../../shared/components/error-notice'; -import {ProcessURL} from '../../../shared/components/links'; -import {Loading} from '../../../shared/components/loading'; -import {Context} from '../../../shared/context'; -import {services} from '../../../shared/services'; -import {useQueryParams} from '../../../shared/use-query-params'; -import {WorkflowArtifacts} from '../../../workflows/components/workflow-artifacts'; - -import {ANNOTATION_KEY_POD_NAME_VERSION} from '../../../shared/annotations'; -import {getPodName, getTemplateNameFromNode} from '../../../shared/pod-name'; -import {getResolvedTemplates} from '../../../shared/template-resolution'; -import {ArtifactPanel} from '../../../workflows/components/workflow-details/artifact-panel'; -import {WorkflowResourcePanel} from '../../../workflows/components/workflow-details/workflow-resource-panel'; -import {WorkflowLogsViewer} from '../../../workflows/components/workflow-logs-viewer/workflow-logs-viewer'; -import {WorkflowNodeInfo} from '../../../workflows/components/workflow-node-info/workflow-node-info'; -import {WorkflowPanel} from '../../../workflows/components/workflow-panel/workflow-panel'; -import {WorkflowParametersPanel} from '../../../workflows/components/workflow-parameters-panel'; -import {WorkflowSummaryPanel} from '../../../workflows/components/workflow-summary-panel'; -import {WorkflowTimeline} from '../../../workflows/components/workflow-timeline/workflow-timeline'; -import {WorkflowYamlViewer} from '../../../workflows/components/workflow-yaml-viewer/workflow-yaml-viewer'; - -require('../../../workflows/components/workflow-details/workflow-details.scss'); - -const STEP_GRAPH_CONTAINER_MIN_WIDTH = 490; -const STEP_INFO_WIDTH = 570; - -export const ArchivedWorkflowDetails = ({history, location, match}: RouteComponentProps) => { - const ctx = useContext(Context); - const queryParams = new URLSearchParams(location.search); - const [workflow, setWorkflow] = useState(); - const [links, setLinks] = useState(); - const [error, setError] = useState(); - - const [namespace] = useState(match.params.namespace); - const [uid] = useState(match.params.uid); - const [tab, setTab] = useState(queryParams.get('tab') || 'workflow'); - const [nodeId, setNodeId] = useState(queryParams.get('nodeId')); - const [container, setContainer] = useState(queryParams.get('container') || 'main'); - const [sidePanel, setSidePanel] = useState(queryParams.get('sidePanel')); - const selectedArtifact = workflow && workflow.status && findArtifact(workflow.status, nodeId); - const [selectedTemplateArtifactRepo, setSelectedTemplateArtifactRepo] = useState(); - const node = nodeId && workflow.status.nodes[nodeId]; - - const podName = () => { - if (nodeId && workflow) { - const workflowName = workflow.metadata.name; - const annotations = workflow.metadata.annotations || {}; - const version = annotations[ANNOTATION_KEY_POD_NAME_VERSION]; - const templateName = getTemplateNameFromNode(node); - return getPodName(workflowName, node.name, templateName, nodeId, version); - } - }; - - useEffect( - useQueryParams(history, p => { - setSidePanel(p.get('sidePanel')); - setNodeId(p.get('nodeId')); - setContainer(p.get('container')); - }), - [history] - ); - - useEffect(() => { - services.info - .getInfo() - .then(info => setLinks(info.links)) - .then(() => - services.archivedWorkflows.get(uid, namespace).then(retrievedWorkflow => { - setError(null); - setWorkflow(retrievedWorkflow); - }) - ) - .catch(newError => setError(newError)); - services.info.collectEvent('openedArchivedWorkflowDetails').then(); - }, []); - - useEffect(() => { - // update the default Artifact Repository for the Template that corresponds to the selectedArtifact - // if there's an ArtifactLocation configured for the Template we use that - // otherwise we use the central one for the Workflow configured in workflow.status.artifactRepositoryRef.artifactRepository - // (Note that individual Artifacts may also override whatever this gets set to) - if (workflow?.status?.nodes && selectedArtifact) { - const template = getResolvedTemplates(workflow, workflow.status.nodes[selectedArtifact.nodeId]); - const artifactRepo = template?.archiveLocation; - if (artifactRepo && artifactRepoHasLocation(artifactRepo)) { - setSelectedTemplateArtifactRepo(artifactRepo); - } else { - setSelectedTemplateArtifactRepo(workflow.status.artifactRepositoryRef.artifactRepository); - } - } - }, [workflow, selectedArtifact]); - - const renderArchivedWorkflowDetails = () => { - if (error) { - return ; - } - if (!workflow) { - return ; - } - return ( - <> - {tab === 'summary' ? ( -
-
-
- - {execSpec(workflow).arguments && execSpec(workflow).arguments.parameters && ( - -
Parameters
- - - )} -
Artifacts
- - -
-
-
- ) : ( -
-
- {tab === 'workflow' ? ( - setNodeId(newNodeId)} - /> - ) : ( - setNodeId(newNode.id)} /> - )} -
- {nodeId && ( -
- - {node && ( - { - setSidePanel('yaml'); - setNodeId(newNodeId); - }} - onShowContainerLogs={(newNodeId, newContainer) => { - setSidePanel('logs'); - setNodeId(newNodeId); - setContainer(newContainer); - }} - archived={true} - /> - )} - {selectedArtifact && ( - - )} -
- )} -
- )} - setSidePanel(null)}> - {sidePanel === 'yaml' && } - {sidePanel === 'logs' && } - - - ); - }; - - const deleteArchivedWorkflow = () => { - if (!confirm('Are you sure you want to delete this archived workflow?\nThere is no undo.')) { - return; - } - services.archivedWorkflows - .delete(uid, workflow.metadata.namespace) - .then(() => { - document.location.href = uiUrl('archived-workflows'); - }) - .catch(e => { - ctx.notifications.show({ - content: 'Failed to delete archived workflow ' + e, - type: NotificationType.Error - }); - }); - }; - - const resubmitArchivedWorkflow = () => { - if (!confirm('Are you sure you want to resubmit this archived workflow?')) { - return; - } - services.archivedWorkflows - .resubmit(workflow.metadata.uid, workflow.metadata.namespace) - .then(newWorkflow => (document.location.href = uiUrl(`workflows/${newWorkflow.metadata.namespace}/${newWorkflow.metadata.name}`))) - .catch(e => { - ctx.notifications.show({ - content: 'Failed to resubmit archived workflow ' + e, - type: NotificationType.Error - }); - }); - }; - - const retryArchivedWorkflow = () => { - if (!confirm('Are you sure you want to retry this archived workflow?')) { - return; - } - services.archivedWorkflows - .retry(workflow.metadata.uid, workflow.metadata.namespace) - .then(newWorkflow => (document.location.href = uiUrl(`workflows/${newWorkflow.metadata.namespace}/${newWorkflow.metadata.name}`))) - .catch(e => { - ctx.notifications.show({ - content: 'Failed to retry archived workflow ' + e, - type: NotificationType.Error - }); - }); - }; - - const openLink = (link: Link) => { - const object = { - metadata: { - namespace: workflow.metadata.namespace, - name: workflow.metadata.name - }, - workflow, - status: { - startedAt: workflow.status.startedAt, - finishedAt: workflow.status.finishedAt - } - }; - const url = ProcessURL(link.url, object); - - if ((window.event as MouseEvent).ctrlKey || (window.event as MouseEvent).metaKey) { - window.open(url, '_blank'); - } else { - document.location.href = url; - } - }; - - const workflowPhase = workflow?.status?.phase; - const items = [ - { - title: 'Retry', - iconClassName: 'fa fa-undo', - disabled: workflowPhase === undefined || !(workflowPhase === 'Failed' || workflowPhase === 'Error'), - action: () => retryArchivedWorkflow() - }, - { - title: 'Resubmit', - iconClassName: 'fa fa-plus-circle', - disabled: false, - action: () => resubmitArchivedWorkflow() - }, - { - title: 'Delete', - iconClassName: 'fa fa-trash', - disabled: false, - action: () => deleteArchivedWorkflow() - } - ]; - if (links) { - links - .filter(link => link.scope === 'workflow') - .forEach(link => - items.push({ - title: link.name, - iconClassName: 'fa fa-external-link-alt', - disabled: false, - action: () => openLink(link) - }) - ); - } - - return ( - - setTab('summary')}> - - - setTab('timeline')}> - - - setTab('workflow')}> - - - - ) - }}> -
{renderArchivedWorkflowDetails()}
- - ); -}; diff --git a/ui/src/app/archived-workflows/components/archived-workflow-filters/archived-workflow-filters.scss b/ui/src/app/archived-workflows/components/archived-workflow-filters/archived-workflow-filters.scss deleted file mode 100644 index 65f18b9410ca..000000000000 --- a/ui/src/app/archived-workflows/components/archived-workflow-filters/archived-workflow-filters.scss +++ /dev/null @@ -1,28 +0,0 @@ -@import 'node_modules/argo-ui/src/styles/config'; - -.wf-filters-container { - overflow: visible; - position: relative; - border-radius: 5px; - box-shadow: 1px 1px 3px #8fa4b1; - padding: 0 1em 0.75em 1em; - margin: 12px 0; - background-color: white; -} - -.wf-filters-container p { - margin: 0; - margin-top: 1em; - color: #6d7f8b; - text-transform: uppercase; -} - -.wf-filters-container__title { - position: relative; - width: 100%; - max-width: 100%; - padding: 8px 0; - font-size: 15px; - background-color: transparent; - border: 0; -} diff --git a/ui/src/app/archived-workflows/components/archived-workflow-filters/archived-workflow-filters.tsx b/ui/src/app/archived-workflows/components/archived-workflow-filters/archived-workflow-filters.tsx deleted file mode 100644 index 111c2a52d404..000000000000 --- a/ui/src/app/archived-workflows/components/archived-workflow-filters/archived-workflow-filters.tsx +++ /dev/null @@ -1,206 +0,0 @@ -import * as React from 'react'; -import DatePicker from 'react-datepicker'; -import * as models from '../../../../models'; -import {CheckboxFilter} from '../../../shared/components/checkbox-filter/checkbox-filter'; -import {InputFilter} from '../../../shared/components/input-filter'; -import {NamespaceFilter} from '../../../shared/components/namespace-filter'; -import {TagsInput} from '../../../shared/components/tags-input/tags-input'; -import {services} from '../../../shared/services'; - -import 'react-datepicker/dist/react-datepicker.css'; - -require('./archived-workflow-filters.scss'); - -interface ArchivedWorkflowFilterProps { - workflows: models.Workflow[]; - namespace: string; - name: string; - namePrefix: string; - phaseItems: string[]; - selectedPhases: string[]; - selectedLabels: string[]; - minStartedAt?: Date; - maxStartedAt?: Date; - onChange: (namespace: string, name: string, namePrefix: string, selectedPhases: string[], labels: string[], minStartedAt: Date, maxStartedAt: Date) => void; -} - -interface State { - labels: string[]; -} - -export class ArchivedWorkflowFilters extends React.Component { - constructor(props: ArchivedWorkflowFilterProps) { - super(props); - this.state = { - labels: [] - }; - } - - public componentDidMount(): void { - this.fetchArchivedWorkflowsLabelKeys(); - } - - public render() { - return ( -
-
-
-

Namespace

- { - this.props.onChange( - ns, - this.props.name, - this.props.namePrefix, - this.props.selectedPhases, - this.props.selectedLabels, - this.props.minStartedAt, - this.props.maxStartedAt - ); - }} - /> -
-
-

Name

- { - this.props.onChange( - this.props.namespace, - wfname, - this.props.namePrefix, - this.props.selectedPhases, - this.props.selectedLabels, - this.props.minStartedAt, - this.props.maxStartedAt - ); - }} - /> -
-
-

Name Prefix

- { - this.props.onChange( - this.props.namespace, - this.props.name, - wfnamePrefix, - this.props.selectedPhases, - this.props.selectedLabels, - this.props.minStartedAt, - this.props.maxStartedAt - ); - }} - /> -
-
-

Labels

- { - this.props.onChange( - this.props.namespace, - this.props.name, - this.props.namePrefix, - this.props.selectedPhases, - tags, - this.props.minStartedAt, - this.props.maxStartedAt - ); - }} - /> -
-
-

Phases

- { - this.props.onChange( - this.props.namespace, - this.props.name, - this.props.namePrefix, - selected, - this.props.selectedLabels, - this.props.minStartedAt, - this.props.maxStartedAt - ); - }} - items={this.getPhaseItems(this.props.workflows)} - type='phase' - /> -
-
-

Started Time

- { - this.props.onChange( - this.props.namespace, - this.props.name, - this.props.namePrefix, - this.props.selectedPhases, - this.props.selectedLabels, - date, - this.props.maxStartedAt - ); - }} - placeholderText='From' - dateFormat='dd MMM yyyy' - todayButton='Today' - className='argo-field argo-textarea' - /> - { - this.props.onChange( - this.props.namespace, - this.props.name, - this.props.namePrefix, - this.props.selectedPhases, - this.props.selectedLabels, - this.props.minStartedAt, - date - ); - }} - placeholderText='To' - dateFormat='dd MMM yyyy' - todayButton='Today' - className='argo-field argo-textarea' - /> -
-
-
- ); - } - - private getPhaseItems(workflows: models.Workflow[]) { - const phasesMap = new Map(); - this.props.phaseItems.forEach(value => phasesMap.set(value, 0)); - workflows.filter(wf => wf.status.phase).forEach(wf => phasesMap.set(wf.status.phase, (phasesMap.get(wf.status.phase) || 0) + 1)); - const results = new Array<{name: string; count: number}>(); - phasesMap.forEach((val, key) => { - results.push({name: key, count: val}); - }); - return results; - } - - private fetchArchivedWorkflowsLabelKeys(): void { - services.archivedWorkflows.listLabelKeys(this.props.namespace).then(list => { - this.setState({ - labels: list.items?.sort((a, b) => a.localeCompare(b)) || [] - }); - }); - } - - private async fetchArchivedWorkflowsLabels(key: string): Promise { - const labels = await services.archivedWorkflows.listLabelValues(key, this.props?.namespace); - return labels.items.map(i => key + '=' + i).sort((a, b) => a.localeCompare(b)); - } -} diff --git a/ui/src/app/archived-workflows/components/archived-workflow-list/archived-workflow-list.tsx b/ui/src/app/archived-workflows/components/archived-workflow-list/archived-workflow-list.tsx deleted file mode 100644 index a718bae00c77..000000000000 --- a/ui/src/app/archived-workflows/components/archived-workflow-list/archived-workflow-list.tsx +++ /dev/null @@ -1,312 +0,0 @@ -import {Page} from 'argo-ui'; -import * as React from 'react'; -import {Link, RouteComponentProps} from 'react-router-dom'; -import * as models from '../../../../models'; -import {Workflow} from '../../../../models'; -import {uiUrl} from '../../../shared/base'; -import {BasePage} from '../../../shared/components/base-page'; -import {ErrorNotice} from '../../../shared/components/error-notice'; -import {Loading} from '../../../shared/components/loading'; -import {PaginationPanel} from '../../../shared/components/pagination-panel'; -import {PhaseIcon} from '../../../shared/components/phase-icon'; -import {Timestamp} from '../../../shared/components/timestamp'; -import {ZeroState} from '../../../shared/components/zero-state'; -import {formatDuration, wfDuration} from '../../../shared/duration'; -import {Pagination, parseLimit} from '../../../shared/pagination'; -import {ScopedLocalStorage} from '../../../shared/scoped-local-storage'; -import {services} from '../../../shared/services'; -import {Utils} from '../../../shared/utils'; -import {ArchivedWorkflowFilters} from '../archived-workflow-filters/archived-workflow-filters'; - -interface BrowserStorageOptions { - pagination: Pagination; - namespace: string; - name: string; - namePrefix: string; - selectedPhases: string[]; - selectedLabels: string[]; - minStartedAt?: Date; - maxStartedAt?: Date; - error?: Error; - deep: boolean; -} - -interface State extends BrowserStorageOptions { - workflows?: Workflow[]; -} - -const defaultPaginationLimit = 10; - -export class ArchivedWorkflowList extends BasePage, State> { - private storage: ScopedLocalStorage; - - constructor(props: RouteComponentProps, context: any) { - super(props, context); - this.storage = new ScopedLocalStorage('ArchiveListOptions'); - const savedOptions = this.storage.getItem('options', { - pagination: {limit: defaultPaginationLimit}, - selectedPhases: [], - selectedLabels: [] - } as State); - const phaseQueryParam = this.queryParams('phase'); - const labelQueryParam = this.queryParams('label'); - this.state = { - pagination: {offset: this.queryParam('offset'), limit: parseLimit(this.queryParam('limit')) || savedOptions.pagination.limit}, - namespace: Utils.getNamespace(this.props.match.params.namespace) || '', - name: this.queryParams('name').toString() || '', - namePrefix: this.queryParams('namePrefix').toString() || '', - selectedPhases: phaseQueryParam.length > 0 ? phaseQueryParam : savedOptions.selectedPhases, - selectedLabels: labelQueryParam.length > 0 ? labelQueryParam : savedOptions.selectedLabels, - minStartedAt: this.parseTime(this.queryParam('minStartedAt')) || this.lastMonth(), - maxStartedAt: this.parseTime(this.queryParam('maxStartedAt')) || this.nextDay(), - deep: this.queryParam('deep') === 'true' - }; - } - - public componentDidMount(): void { - this.fetchArchivedWorkflows( - this.state.namespace, - this.state.name, - this.state.namePrefix, - this.state.selectedPhases, - this.state.selectedLabels, - this.state.minStartedAt, - this.state.maxStartedAt, - this.state.pagination - ); - services.info.collectEvent('openedArchivedWorkflowList').then(); - } - - public componentDidUpdate(): void { - if (this.state.deep === true && this.state.workflows && this.state.workflows.length === 1) { - const workflow = this.state.workflows[0]; - const url = '/archived-workflows/' + workflow.metadata.namespace + '/' + (workflow.metadata.uid || ''); - this.props.history.push(url); - } - } - - public render() { - return ( - -
-
-
- - this.changeFilters(namespace, name, namePrefix, selectedPhases, selectedLabels, minStartedAt, maxStartedAt, { - limit: this.state.pagination.limit - }) - } - /> -
-
-
{this.renderWorkflows()}
-
- - ); - } - - private lastMonth() { - const dt = new Date(); - dt.setMonth(dt.getMonth() - 1); - dt.setHours(0, 0, 0, 0); - return dt; - } - - private nextDay() { - const dt = new Date(); - dt.setDate(dt.getDate() + 1); - dt.setHours(0, 0, 0, 0); - return dt; - } - - private parseTime(dateStr: string) { - if (dateStr != null) { - return new Date(dateStr); - } - } - - private changeFilters( - namespace: string, - name: string, - namePrefix: string, - selectedPhases: string[], - selectedLabels: string[], - minStartedAt: Date, - maxStartedAt: Date, - pagination: Pagination - ) { - this.fetchArchivedWorkflows(namespace, name, namePrefix, selectedPhases, selectedLabels, minStartedAt, maxStartedAt, pagination); - } - - private get filterParams() { - const params = new URLSearchParams(); - if (this.state.selectedPhases) { - this.state.selectedPhases.forEach(phase => { - params.append('phase', phase); - }); - } - if (this.state.selectedLabels) { - this.state.selectedLabels.forEach(label => { - params.append('label', label); - }); - } - if (this.state.name) { - params.append('name', this.state.name); - } - if (this.state.namePrefix) { - params.append('namePrefix', this.state.namePrefix); - } - params.append('minStartedAt', this.state.minStartedAt.toISOString()); - params.append('maxStartedAt', this.state.maxStartedAt.toISOString()); - if (this.state.pagination.offset) { - params.append('offset', this.state.pagination.offset); - } - if (this.state.pagination.limit !== null && this.state.pagination.limit !== defaultPaginationLimit) { - params.append('limit', this.state.pagination.limit.toString()); - } - return params; - } - - private fetchBrowserStorageStateObject(state: State): BrowserStorageOptions { - const browserStorageOptions: BrowserStorageOptions = {} as BrowserStorageOptions; - browserStorageOptions.deep = state.deep; - browserStorageOptions.error = state.error; - browserStorageOptions.maxStartedAt = state.maxStartedAt; - browserStorageOptions.minStartedAt = state.minStartedAt; - browserStorageOptions.name = state.name; - browserStorageOptions.namePrefix = state.namePrefix; - browserStorageOptions.namespace = state.namespace; - browserStorageOptions.pagination = state.pagination; - browserStorageOptions.selectedLabels = state.selectedLabels; - browserStorageOptions.selectedPhases = state.selectedPhases; - return browserStorageOptions; - } - - private saveHistory() { - this.storage.setItem('options', this.fetchBrowserStorageStateObject(this.state), {} as BrowserStorageOptions); - const newNamespace = Utils.managedNamespace ? '' : this.state.namespace; - this.url = uiUrl('archived-workflows' + (newNamespace ? '/' + newNamespace : '') + '?' + this.filterParams.toString()); - Utils.currentNamespace = this.state.namespace; - } - - private fetchArchivedWorkflows( - namespace: string, - name: string, - namePrefix: string, - selectedPhases: string[], - selectedLabels: string[], - minStartedAt: Date, - maxStartedAt: Date, - pagination: Pagination - ): void { - services.archivedWorkflows - .list(namespace, name, namePrefix, selectedPhases, selectedLabels, minStartedAt, maxStartedAt, pagination) - .then(list => { - this.setState( - { - error: null, - namespace, - name, - namePrefix, - workflows: list.items || [], - selectedPhases, - selectedLabels, - minStartedAt, - maxStartedAt, - pagination: { - limit: pagination.limit, - offset: pagination.offset, - nextOffset: list.metadata.continue - } - }, - this.saveHistory - ); - }) - .catch(error => this.setState({error})); - } - - private renderWorkflows() { - if (this.state.error) { - return ; - } - if (!this.state.workflows) { - return ; - } - const learnMore = Learn more; - if (this.state.workflows.length === 0) { - return ( - -

To add entries to the archive you must enable archiving in configuration. Records are created in the archive on workflow completion.

-

{learnMore}.

- - ); - } - - return ( - <> -
-
-
-
NAME
-
NAMESPACE
-
STARTED
-
FINISHED
-
DURATION
-
- {this.state.workflows.map(w => ( - -
- -
-
{w.metadata.name}
-
{w.metadata.namespace}
-
- -
-
- -
-
{formatDuration(wfDuration(w.status))}
- - ))} -
- - this.changeFilters( - this.state.namespace, - this.state.name, - this.state.namePrefix, - this.state.selectedPhases, - this.state.selectedLabels, - this.state.minStartedAt, - this.state.maxStartedAt, - pagination - ) - } - pagination={this.state.pagination} - numRecords={(this.state.workflows || []).length} - /> -

- Records are created in the archive when a workflow completes. {learnMore}. -

- - ); - } -} diff --git a/ui/src/app/archived-workflows/index.ts b/ui/src/app/archived-workflows/index.ts deleted file mode 100644 index 36b663139f39..000000000000 --- a/ui/src/app/archived-workflows/index.ts +++ /dev/null @@ -1,5 +0,0 @@ -import {ArchivedWorkflowContainer} from './components/archived-workflow-container'; - -export default { - component: ArchivedWorkflowContainer -}; diff --git a/ui/src/app/reports/components/reports.tsx b/ui/src/app/reports/components/reports.tsx index e1eee65df22e..a4feac527964 100644 --- a/ui/src/app/reports/components/reports.tsx +++ b/ui/src/app/reports/components/reports.tsx @@ -1,4 +1,4 @@ -import {Checkbox, Page} from 'argo-ui/src/index'; +import {Page} from 'argo-ui/src/index'; import {ChartOptions} from 'chart.js'; import 'chartjs-plugin-annotation'; import * as React from 'react'; @@ -25,7 +25,6 @@ interface Chart { } interface State { - archivedWorkflows: boolean; namespace: string; labels: string[]; autocompleteLabels: string[]; @@ -58,7 +57,6 @@ export class Reports extends BasePage, State> { constructor(props: RouteComponentProps, context: any) { super(props, context); this.state = { - archivedWorkflows: !!this.queryParam('archivedWorkflows'), namespace: Utils.getNamespace(this.props.match.params.namespace) || '', labels: (this.queryParam('labels') || '').split(',').filter(v => v !== ''), autocompleteLabels: [''] @@ -66,8 +64,7 @@ export class Reports extends BasePage, State> { } public componentDidMount() { - this.fetchReport(this.state.namespace, this.state.labels, this.state.archivedWorkflows); - this.fetchWorkflowsLabels(this.state.archivedWorkflows); + this.fetchReport(this.state.namespace, this.state.labels); services.info.collectEvent('openedReports').then(); } @@ -99,38 +96,30 @@ export class Reports extends BasePage, State> { } private setLabel(name: string, value: string) { - this.fetchReport(this.state.namespace, this.state.labels.filter(label => !label.startsWith(name)).concat(name + '=' + value), this.state.archivedWorkflows); + this.fetchReport(this.state.namespace, this.state.labels.filter(label => !label.startsWith(name)).concat(name + '=' + value)); } - private fetchReport(namespace: string, labels: string[], archivedWorkflows: boolean) { + private fetchReport(namespace: string, labels: string[]) { if (namespace === '' || labels.length === 0) { - this.setState({namespace, labels, archivedWorkflows, charts: null}); + this.setState({namespace, labels, charts: null}); return; } - (archivedWorkflows - ? services.archivedWorkflows.list(namespace, '', '', [], labels, null, null, {limit}) - : services.workflows.list(namespace, [], labels, {limit}, [ - 'items.metadata.name', - 'items.status.phase', - 'items.status.startedAt', - 'items.status.finishedAt', - 'items.status.resourcesDuration' - ]) - ) + services.workflows + .list(namespace, [], labels, {limit}, [ + 'items.metadata.name', + 'items.status.phase', + 'items.status.startedAt', + 'items.status.finishedAt', + 'items.status.resourcesDuration' + ]) .then(list => this.getExtractDatasets(list.items || [])) - .then(charts => this.setState({error: null, charts, namespace, labels, archivedWorkflows}, this.saveHistory)) + .then(charts => this.setState({error: null, charts, namespace, labels}, this.saveHistory)) .catch(error => this.setState({error})); } private saveHistory() { const newNamespace = Utils.managedNamespace ? '' : this.state.namespace; - this.url = uiUrl( - 'reports' + - (newNamespace ? '/' + newNamespace : '') + - '?labels=' + - this.state.labels.join(',') + - (this.state.archivedWorkflows ? '&archivedWorkflows=' + this.state.archivedWorkflows : '') - ); + this.url = uiUrl('reports' + (newNamespace ? '/' + newNamespace : '') + '?labels=' + this.state.labels.join(',')); Utils.currentNamespace = this.state.namespace; } @@ -257,42 +246,16 @@ export class Reports extends BasePage, State> { ]; } - private fetchWorkflowsLabels(isArchivedWorkflows: boolean): void { - if (isArchivedWorkflows) { - services.archivedWorkflows.listLabelKeys(this.state.namespace).then(list => { - this.setState({ - autocompleteLabels: list.items?.sort((a, b) => a.localeCompare(b)) || [] - }); - }); - } - } - - private fetchArchivedWorkflowsLabels(key: string): Promise { - return services.archivedWorkflows.listLabelValues(key, this.state.namespace).then(list => { - return list.items.map(i => key + '=' + i).sort((a, b) => a.localeCompare(b)); - }); - } - private renderFilters() { return (
-
-

Archived Workflows

- { - this.fetchReport(this.state.namespace, this.state.labels, checked); - this.fetchWorkflowsLabels(checked); - }} - /> -

Namespace

{ - this.fetchReport(namespace, this.state.labels, this.state.archivedWorkflows); + this.fetchReport(namespace, this.state.labels); }} />
@@ -301,9 +264,8 @@ export class Reports extends BasePage, State> { this.fetchReport(this.state.namespace, labels, this.state.archivedWorkflows)} + autocomplete={this.state.autocompleteLabels} + onChange={labels => this.fetchReport(this.state.namespace, labels)} />
diff --git a/ui/src/app/shared/services/archived-workflows-service.ts b/ui/src/app/shared/services/archived-workflows-service.ts deleted file mode 100644 index ecda26a10c27..000000000000 --- a/ui/src/app/shared/services/archived-workflows-service.ts +++ /dev/null @@ -1,63 +0,0 @@ -import * as models from '../../../models'; -import {Pagination} from '../pagination'; -import {Utils} from '../utils'; -import requests from './requests'; -export const ArchivedWorkflowsService = { - list(namespace: string, name: string, namePrefix: string, phases: string[], labels: string[], minStartedAt: Date, maxStartedAt: Date, pagination: Pagination) { - if (namespace === '') { - return requests - .get(`api/v1/archived-workflows?${Utils.queryParams({name, namePrefix, phases, labels, minStartedAt, maxStartedAt, pagination}).join('&')}`) - .then(res => res.body as models.WorkflowList); - } else { - return requests - .get(`api/v1/archived-workflows?namespace=${namespace}&${Utils.queryParams({name, namePrefix, phases, labels, minStartedAt, maxStartedAt, pagination}).join('&')}`) - .then(res => res.body as models.WorkflowList); - } - }, - - get(uid: string, namespace: string) { - if (namespace === '') { - return requests.get(`api/v1/archived-workflows/${uid}`).then(res => res.body as models.Workflow); - } else { - return requests.get(`api/v1/archived-workflows/${uid}?namespace=${namespace}`).then(res => res.body as models.Workflow); - } - }, - - delete(uid: string, namespace: string) { - if (namespace === '') { - return requests.delete(`api/v1/archived-workflows/${uid}`); - } else { - return requests.delete(`api/v1/archived-workflows/${uid}?namespace=${namespace}`); - } - }, - - listLabelKeys(namespace: string) { - if (namespace === '') { - return requests.get(`api/v1/archived-workflows-label-keys`).then(res => res.body as models.Labels); - } else { - return requests.get(`api/v1/archived-workflows-label-keys?namespace=${namespace}`).then(res => res.body as models.Labels); - } - }, - - async listLabelValues(key: string, namespace: string): Promise { - let url = `api/v1/archived-workflows-label-values?listOptions.labelSelector=${key}`; - if (namespace !== '') { - url += `&namespace=${namespace}`; - } - return (await requests.get(url)).body as models.Labels; - }, - - resubmit(uid: string, namespace: string) { - return requests - .put(`api/v1/archived-workflows/${uid}/resubmit`) - .send({namespace}) - .then(res => res.body as models.Workflow); - }, - - retry(uid: string, namespace: string) { - return requests - .put(`api/v1/archived-workflows/${uid}/retry`) - .send({namespace}) - .then(res => res.body as models.Workflow); - } -}; diff --git a/ui/src/app/shared/services/index.ts b/ui/src/app/shared/services/index.ts index d44e7364be1c..29e9b1326ea5 100644 --- a/ui/src/app/shared/services/index.ts +++ b/ui/src/app/shared/services/index.ts @@ -1,4 +1,3 @@ -import {ArchivedWorkflowsService} from './archived-workflows-service'; import {ClusterWorkflowTemplateService} from './cluster-workflow-template-service'; import {CronWorkflowService} from './cron-workflow-service'; import {EventService} from './event-service'; @@ -16,7 +15,6 @@ interface Services { workflows: typeof WorkflowsService; workflowTemplate: typeof WorkflowTemplateService; clusterWorkflowTemplate: typeof ClusterWorkflowTemplateService; - archivedWorkflows: typeof ArchivedWorkflowsService; cronWorkflows: typeof CronWorkflowService; } @@ -28,6 +26,5 @@ export const services: Services = { event: EventService, eventSource: EventSourceService, sensor: SensorService, - archivedWorkflows: ArchivedWorkflowsService, cronWorkflows: CronWorkflowService }; From d267bab342c3de24bdd3f30d83ef6928bfb7d14b Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Tue, 23 May 2023 12:26:50 -0400 Subject: [PATCH 06/27] fix: fix wf detail Signed-off-by: Yuan Tang --- .../workflow-details/workflow-details.tsx | 37 ++++++++++++------- .../workflows-row/workflows-row.tsx | 6 +-- ui/src/models/workflows.ts | 9 +++++ 3 files changed, 34 insertions(+), 18 deletions(-) diff --git a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx index f61bdce5011d..6e76852f3d2d 100644 --- a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx +++ b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx @@ -3,7 +3,7 @@ import * as classNames from 'classnames'; import * as React from 'react'; import {useContext, useEffect, useRef, useState} from 'react'; import {RouteComponentProps} from 'react-router'; -import {ArtifactRepository, execSpec, Link, NodeStatus, Parameter, Workflow} from '../../../../models'; +import {ArtifactRepository, execSpec, isArchivedWorkflow, Link, NodeStatus, Parameter, Workflow} from '../../../../models'; import {ANNOTATION_KEY_POD_NAME_VERSION} from '../../../shared/annotations'; import {artifactRepoHasLocation, findArtifact} from '../../../shared/artifacts'; import {uiUrl} from '../../../shared/base'; @@ -57,6 +57,7 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< const queryParams = new URLSearchParams(location.search); const [namespace] = useState(match.params.namespace); + const [isArchived, setIsArchived] = useState(false); const [name, setName] = useState(match.params.name); const [tab, setTab] = useState(queryParams.get('tab') || 'workflow'); const [nodeId, setNodeId] = useState(queryParams.get('nodeId')); @@ -277,7 +278,7 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< )}
Artifacts
- +
@@ -286,7 +287,6 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< ); }; - useEffect(() => { const retryWatch = new RetryWatch( () => services.workflows.watch({name, namespace}), @@ -294,6 +294,7 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< e => { if (e.type === 'DELETED') { setError(new Error('Workflow gone')); + setIsArchived(true); } else { if (hasArtifactGCError(e.object.status.conditions)) { setError(new Error('Artifact garbage collection failed')); @@ -302,22 +303,24 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< } }, err => { - services.workflows - .get(namespace, name) - .then() - .catch(e => { - if (e.status === 404) { - navigation.goto(historyUrl('archived-workflows', {namespace, name, deep: true})); - } - }); - setError(err); + setIsArchived(true); } ); retryWatch.start(); return () => retryWatch.stop(); }, [namespace, name]); + useEffect(() => { + services.workflows + .get(namespace, name) + .then(wf => { + setError(null); + setWorkflow(wf); + }) + .catch(newError => setError(newError)); + }, [namespace, name, isArchived]); + const openLink = (link: Link) => { const object = { metadata: { @@ -462,7 +465,7 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< onShowContainerLogs={(x, container) => setSidePanel(`logs:${x}:${container}`)} onShowEvents={() => setSidePanel(`events:${nodeId}`)} onShowYaml={() => setSidePanel(`yaml:${nodeId}`)} - archived={false} + archived={isArchivedWorkflow(workflow)} onResume={() => renderResumePopup()} /> )} @@ -474,7 +477,13 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< {workflow && ( setSidePanel(null)}> {parsedSidePanel.type === 'logs' && ( - + )} {parsedSidePanel.type === 'events' && } {parsedSidePanel.type === 'share' && } diff --git a/ui/src/app/workflows/components/workflows-row/workflows-row.tsx b/ui/src/app/workflows/components/workflows-row/workflows-row.tsx index cf0bf950bb35..7e6b44b0ae07 100644 --- a/ui/src/app/workflows/components/workflows-row/workflows-row.tsx +++ b/ui/src/app/workflows/components/workflows-row/workflows-row.tsx @@ -2,7 +2,7 @@ import {Ticker} from 'argo-ui/src/index'; import * as React from 'react'; import {Link} from 'react-router-dom'; import * as models from '../../../../models'; -import {Workflow} from '../../../../models'; +import {isArchivedWorkflow, Workflow} from '../../../../models'; import {ANNOTATION_DESCRIPTION, ANNOTATION_TITLE} from '../../../shared/annotations'; import {uiUrl} from '../../../shared/base'; import {DurationPanel} from '../../../shared/components/duration-panel'; @@ -87,9 +87,7 @@ export class WorkflowsRow extends React.Component
-
- {wf.metadata.labels && wf.metadata.labels['workflows.argoproj.io/workflow-archiving-status'] === 'Archived' ? 'true' : 'false'} -
+
{isArchivedWorkflow(wf) ? 'true' : 'false'}
{(this.props.columns || []).map(column => { const value = wf.metadata.labels[column.key]; return ( diff --git a/ui/src/models/workflows.ts b/ui/src/models/workflows.ts index b5286a62c5b8..e8a653e492db 100644 --- a/ui/src/models/workflows.ts +++ b/ui/src/models/workflows.ts @@ -539,6 +539,15 @@ export interface Workflow { export const execSpec = (w: Workflow) => Object.assign({}, w.status.storedWorkflowTemplateSpec, w.spec); +// The label may not have been updated on time but usually this indicates that they are already archived. +export function isArchivedWorkflow(wf: Workflow): boolean { + return ( + wf.metadata.labels && + (wf.metadata.labels['workflows.argoproj.io/workflow-archiving-status'] === 'Archived' || + wf.metadata.labels['workflows.argoproj.io/workflow-archiving-status'] === 'Pending') + ); +} + export type NodeType = 'Pod' | 'Container' | 'Steps' | 'StepGroup' | 'DAG' | 'Retry' | 'Skipped' | 'TaskGroup' | 'Suspend'; export interface NodeStatus { From 9cbdb5995aa087e0374270d012e76cec912031f0 Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Wed, 24 May 2023 13:45:11 -0400 Subject: [PATCH 07/27] fix: fix Signed-off-by: Yuan Tang --- server/workflow/workflow_server.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go index c15032257e90..4817c54e0023 100644 --- a/server/workflow/workflow_server.go +++ b/server/workflow/workflow_server.go @@ -156,13 +156,17 @@ func (s *workflowServer) ListWorkflows(ctx context.Context, req *workflowpkg.Wor Namespace: req.Namespace, }) if err != nil { - return nil, sutils.ToStatusError(err, codes.Internal) - } - for _, item := range archivedWfList.Items { - if !containsWorkflow(*wfList, item) { - wfList.Items = append(wfList.Items, item) + log.Warnf("unable to list archived workflows:%v", err) + } else { + if archivedWfList != nil { + for _, item := range archivedWfList.Items { + if !containsWorkflow(*wfList, item) { + wfList.Items = append(wfList.Items, item) + } + } } } + cleaner := fields.NewCleaner(req.Fields) if s.offloadNodeStatusRepo.IsEnabled() && !cleaner.WillExclude("items.status.nodes") { offloadedNodes, err := s.offloadNodeStatusRepo.List(req.Namespace) From 5e2f5eb76b4f3d07a77be9341e699aba5f8ca736 Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Thu, 1 Jun 2023 10:28:57 -0400 Subject: [PATCH 08/27] feat: Add delete popup Signed-off-by: Yuan Tang --- .../app/shared/services/workflows-service.ts | 8 +++ .../workflow-details/workflow-details.tsx | 65 +++++++++++++++---- 2 files changed, 60 insertions(+), 13 deletions(-) diff --git a/ui/src/app/shared/services/workflows-service.ts b/ui/src/app/shared/services/workflows-service.ts index 9900a10920fa..cbccc297551a 100644 --- a/ui/src/app/shared/services/workflows-service.ts +++ b/ui/src/app/shared/services/workflows-service.ts @@ -151,6 +151,14 @@ export const WorkflowsService = { return requests.delete(`api/v1/workflows/${namespace}/${name}`).then(res => res.body as WorkflowDeleteResponse); }, + deleteArchived(uid: string, namespace: string): Promise { + if (namespace === '') { + return requests.delete(`api/v1/archived-workflows/${uid}`).then(res => res.body as WorkflowDeleteResponse); + } else { + return requests.delete(`api/v1/archived-workflows/${uid}?namespace=${namespace}`).then(res => res.body as WorkflowDeleteResponse); + } + }, + submit(kind: string, name: string, namespace: string, submitOptions?: SubmitOpts) { return requests .post(`api/v1/workflows/${namespace}/submit`) diff --git a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx index 6e76852f3d2d..5e170bd09cce 100644 --- a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx +++ b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx @@ -58,6 +58,7 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< const [namespace] = useState(match.params.namespace); const [isArchived, setIsArchived] = useState(false); + const [deleteArchived, setDeleteArchived] = useState(false); const [name, setName] = useState(match.params.name); const [tab, setTab] = useState(queryParams.get('tab') || 'workflow'); const [nodeId, setNodeId] = useState(queryParams.get('nodeId')); @@ -148,20 +149,39 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< title: workflowOperation.title.charAt(0).toUpperCase() + workflowOperation.title.slice(1), iconClassName: workflowOperation.iconClassName, action: () => { - popup.confirm('Confirm', `Are you sure you want to ${workflowOperation.title.toLowerCase()} this workflow?`).then(yes => { - if (yes) { - workflowOperation - .action(workflow) - .then((wf: Workflow) => { - if (workflowOperation.title === 'DELETE') { - navigation.goto(uiUrl(`workflows/${workflow.metadata.namespace}`)); - } else { + if (workflowOperation.title === 'DELETE') { + popup.confirm('Confirm', renderDeleteCheck).then(yes => { + if (yes) { + services.workflows + .delete(workflow.metadata.name, workflow.metadata.namespace) + .then(() => { + if (!deleteArchived) { + navigation.goto(uiUrl(`workflows/${workflow.metadata.namespace}`)); + } + }) + .catch(setError); + if (deleteArchived) { + services.workflows + .deleteArchived(workflow.metadata.uid, workflow.metadata.namespace) + .then(() => { + navigation.goto(uiUrl(`workflows/${workflow.metadata.namespace}`)); + }) + .catch(setError); + } + } + }); + } else { + popup.confirm('Confirm', `Are you sure you want to ${workflowOperation.title.toLowerCase()} this workflow?`).then(yes => { + if (yes) { + workflowOperation + .action(workflow) + .then((wf: Workflow) => { setName(wf.metadata.name); - } - }) - .catch(setError); - } - }); + }) + .catch(setError); + } + }); + } } }; }); @@ -387,6 +407,25 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< }); }; + const renderDeleteCheck = () => { + return ( + <> +

Are you sure you want to delete this workflow?

+ + + ); + }; + const ensurePodName = (wf: Workflow, node: NodeStatus, nodeID: string): string => { if (workflow && node) { const annotations = workflow.metadata.annotations || {}; From ed6542a2914cb07c9e9b92a6c0d1fc3345d3ebc5 Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Mon, 5 Jun 2023 11:04:34 -0400 Subject: [PATCH 09/27] fix: check if wf is in both Signed-off-by: Yuan Tang --- .../workflow-details/workflow-details.tsx | 55 ++++++++++++------- 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx index 5e170bd09cce..6fbdb1a4c368 100644 --- a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx +++ b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx @@ -57,7 +57,8 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< const queryParams = new URLSearchParams(location.search); const [namespace] = useState(match.params.namespace); - const [isArchived, setIsArchived] = useState(false); + const [isWfInDB, setIsWfInDB] = useState(false); + const [isWfInCluster, setIsWfInCluster] = useState(false); const [deleteArchived, setDeleteArchived] = useState(false); const [name, setName] = useState(match.params.name); const [tab, setTab] = useState(queryParams.get('tab') || 'workflow'); @@ -310,11 +311,14 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< useEffect(() => { const retryWatch = new RetryWatch( () => services.workflows.watch({name, namespace}), - () => setError(null), + () => { + setIsWfInCluster(true); + setError(null); + }, e => { if (e.type === 'DELETED') { setError(new Error('Workflow gone')); - setIsArchived(true); + setIsWfInCluster(false); } else { if (hasArtifactGCError(e.object.status.conditions)) { setError(new Error('Artifact garbage collection failed')); @@ -324,7 +328,7 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< }, err => { setError(err); - setIsArchived(true); + setIsWfInCluster(false); } ); retryWatch.start(); @@ -337,9 +341,10 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< .then(wf => { setError(null); setWorkflow(wf); + setIsWfInDB(true); }) .catch(newError => setError(newError)); - }, [namespace, name, isArchived]); + }, [namespace, name, !isWfInCluster]); const openLink = (link: Link) => { const object = { @@ -408,22 +413,30 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< }; const renderDeleteCheck = () => { - return ( - <> -

Are you sure you want to delete this workflow?

- - - ); + if (isWfInDB && isWfInCluster) { + return ( + <> +

Are you sure you want to delete this workflow?

+ + + ); + } else { + return ( + <> +

Are you sure you want to delete this workflow?

+ + ); + } }; const ensurePodName = (wf: Workflow, node: NodeStatus, nodeID: string): string => { From fb4fd14e3dc616f1370c0525327c317180d4d99b Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Mon, 5 Jun 2023 11:11:02 -0400 Subject: [PATCH 10/27] fix: redirect after deletion Signed-off-by: Yuan Tang --- .../components/workflow-details/workflow-details.tsx | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx index 6fbdb1a4c368..3e4ac8b4048f 100644 --- a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx +++ b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx @@ -156,19 +156,20 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< services.workflows .delete(workflow.metadata.name, workflow.metadata.namespace) .then(() => { - if (!deleteArchived) { - navigation.goto(uiUrl(`workflows/${workflow.metadata.namespace}`)); - } + setIsWfInCluster(false); }) .catch(setError); if (deleteArchived) { services.workflows .deleteArchived(workflow.metadata.uid, workflow.metadata.namespace) .then(() => { - navigation.goto(uiUrl(`workflows/${workflow.metadata.namespace}`)); + setIsWfInDB(false); }) .catch(setError); } + if (!isWfInDB && !isWfInCluster) { + navigation.goto(uiUrl(`workflows/${workflow.metadata.namespace}`)); + } } }); } else { From 50b2e086995cabf502711d49f78cd50f5f0d3345 Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Mon, 5 Jun 2023 12:22:48 -0400 Subject: [PATCH 11/27] fix: edge cases Signed-off-by: Yuan Tang --- .../workflow-details/workflow-details.tsx | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx index 3e4ac8b4048f..8d58c3c7769b 100644 --- a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx +++ b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx @@ -153,13 +153,15 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< if (workflowOperation.title === 'DELETE') { popup.confirm('Confirm', renderDeleteCheck).then(yes => { if (yes) { - services.workflows - .delete(workflow.metadata.name, workflow.metadata.namespace) - .then(() => { - setIsWfInCluster(false); - }) - .catch(setError); - if (deleteArchived) { + if (isWfInCluster) { + services.workflows + .delete(workflow.metadata.name, workflow.metadata.namespace) + .then(() => { + setIsWfInCluster(false); + }) + .catch(setError); + } + if (isWfInDB && deleteArchived) { services.workflows .deleteArchived(workflow.metadata.uid, workflow.metadata.namespace) .then(() => { @@ -334,7 +336,7 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< ); retryWatch.start(); return () => retryWatch.stop(); - }, [namespace, name]); + }, [namespace, name, !isWfInDB]); useEffect(() => { services.workflows @@ -344,7 +346,10 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< setWorkflow(wf); setIsWfInDB(true); }) - .catch(newError => setError(newError)); + .catch(newErr => { + setError(newErr); + navigation.goto(uiUrl(`workflows/${namespace}`)); + }); }, [namespace, name, !isWfInCluster]); const openLink = (link: Link) => { @@ -432,6 +437,7 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< ); } else { + setDeleteArchived(true); return ( <>

Are you sure you want to delete this workflow?

From ae040e87bcb166c5416446619e47b23418d8399e Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Mon, 5 Jun 2023 16:56:31 -0400 Subject: [PATCH 12/27] fix: wip Signed-off-by: Yuan Tang --- .../workflow-details/workflow-details.tsx | 81 +++++++++++-------- 1 file changed, 46 insertions(+), 35 deletions(-) diff --git a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx index 8d58c3c7769b..0a8366f830cc 100644 --- a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx +++ b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx @@ -151,29 +151,31 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< iconClassName: workflowOperation.iconClassName, action: () => { if (workflowOperation.title === 'DELETE') { - popup.confirm('Confirm', renderDeleteCheck).then(yes => { - if (yes) { - if (isWfInCluster) { - services.workflows - .delete(workflow.metadata.name, workflow.metadata.namespace) - .then(() => { - setIsWfInCluster(false); - }) - .catch(setError); - } - if (isWfInDB && deleteArchived) { - services.workflows - .deleteArchived(workflow.metadata.uid, workflow.metadata.namespace) - .then(() => { - setIsWfInDB(false); - }) - .catch(setError); + popup + .confirm('Confirm', () => renderDeleteCheck(deleteArchived, setDeleteArchived)) + .then(yes => { + if (yes) { + if (isWfInCluster) { + services.workflows + .delete(workflow.metadata.name, workflow.metadata.namespace) + .then(() => { + setIsWfInCluster(false); + }) + .catch(setError); + } + if (deleteArchived) { + services.workflows + .deleteArchived(workflow.metadata.uid, workflow.metadata.namespace) + .then(() => { + setIsWfInDB(false); + }) + .catch(setError); + } + if (!isWfInDB && !isWfInCluster) { + navigation.goto(uiUrl(`workflows/${workflow.metadata.namespace}`)); + } } - if (!isWfInDB && !isWfInCluster) { - navigation.goto(uiUrl(`workflows/${workflow.metadata.namespace}`)); - } - } - }); + }); } else { popup.confirm('Confirm', `Are you sure you want to ${workflowOperation.title.toLowerCase()} this workflow?`).then(yes => { if (yes) { @@ -418,26 +420,35 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< }); }; - const renderDeleteCheck = () => { + const renderDeleteCheck = (da: boolean, sda: (da: boolean) => void) => { if (isWfInDB && isWfInCluster) { return ( <>

Are you sure you want to delete this workflow?

- +
+ +
); } else { - setDeleteArchived(true); + if (isWfInDB) { + setDeleteArchived(true); + } return ( <>

Are you sure you want to delete this workflow?

From 5f67f7bb5c7dc2c9af2fa67138a10fa7ce83d03c Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Fri, 9 Jun 2023 13:18:37 -0400 Subject: [PATCH 13/27] fix: wip Signed-off-by: Yuan Tang --- .../workflow-details/workflow-details.tsx | 96 +++++++++++-------- 1 file changed, 58 insertions(+), 38 deletions(-) diff --git a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx index 0a8366f830cc..c486e111d977 100644 --- a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx +++ b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx @@ -51,6 +51,62 @@ const INITIAL_SIDE_PANEL_WIDTH = 570; const ANIMATION_MS = 200; const ANIMATION_BUFFER_MS = 20; +const DeleteCheck = (props: {onChange: (changed: boolean) => void}) => { + const [da, sda] = React.useState(false); + React.useEffect(() => { + props.onChange(da); + }, [da]); + return ( + <> +

Are you sure you want to delete this workflow?

+
+ { + sda(!da); + }} + id='delete-check' + /> + +
+ + ); + // if (isWfInDB && isWfInCluster) { + // return ( + // <> + //

Are you sure you want to delete this workflow?

+ //
+ // { + // sda(!da); + // }} + // // onChange={() => { + // // checked = !checked; + // // // sda(!da); + // // }} + // id='delete-check' + // /> + // + //
+ // + // ); + // } else { + // if (isWfInDB) { + // setDeleteArchived(true); + // } + // return ( + // <> + //

Are you sure you want to delete this workflow?

+ // + // ); + // } +}; + export const WorkflowDetails = ({history, location, match}: RouteComponentProps) => { // boiler-plate const {navigation, popup} = useContext(Context); @@ -152,8 +208,9 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< action: () => { if (workflowOperation.title === 'DELETE') { popup - .confirm('Confirm', () => renderDeleteCheck(deleteArchived, setDeleteArchived)) + .confirm('Confirm', () => ) .then(yes => { + // localIsWfInCluster if (yes) { if (isWfInCluster) { services.workflows @@ -420,43 +477,6 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< }); }; - const renderDeleteCheck = (da: boolean, sda: (da: boolean) => void) => { - if (isWfInDB && isWfInCluster) { - return ( - <> -

Are you sure you want to delete this workflow?

-
- -
- - ); - } else { - if (isWfInDB) { - setDeleteArchived(true); - } - return ( - <> -

Are you sure you want to delete this workflow?

- - ); - } - }; - const ensurePodName = (wf: Workflow, node: NodeStatus, nodeID: string): string => { if (workflow && node) { const annotations = workflow.metadata.annotations || {}; From add32f3d472ef9646a202294cc954117d633dcd2 Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Fri, 9 Jun 2023 13:57:29 -0400 Subject: [PATCH 14/27] fix: wip Signed-off-by: Yuan Tang --- .../workflow-details/workflow-details.tsx | 128 ++++++++---------- 1 file changed, 56 insertions(+), 72 deletions(-) diff --git a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx index c486e111d977..ef66df07db55 100644 --- a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx +++ b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx @@ -51,60 +51,40 @@ const INITIAL_SIDE_PANEL_WIDTH = 570; const ANIMATION_MS = 200; const ANIMATION_BUFFER_MS = 20; -const DeleteCheck = (props: {onChange: (changed: boolean) => void}) => { - const [da, sda] = React.useState(false); - React.useEffect(() => { +const DeleteCheck = (props: {onChange: (changed: boolean) => void; isWfInDB: boolean; isWfInCluster: boolean}) => { + // The local states are created intentionally so that the checkbox works as expected + const [da, sda] = useState(false); + useEffect(() => { props.onChange(da); }, [da]); - return ( - <> -

Are you sure you want to delete this workflow?

-
- { - sda(!da); - }} - id='delete-check' - /> - -
- - ); - // if (isWfInDB && isWfInCluster) { - // return ( - // <> - //

Are you sure you want to delete this workflow?

- //
- // { - // sda(!da); - // }} - // // onChange={() => { - // // checked = !checked; - // // // sda(!da); - // // }} - // id='delete-check' - // /> - // - //
- // - // ); - // } else { - // if (isWfInDB) { - // setDeleteArchived(true); - // } - // return ( - // <> - //

Are you sure you want to delete this workflow?

- // - // ); - // } + if (props.isWfInDB && props.isWfInCluster) { + return ( + <> +

Are you sure you want to delete this workflow?

+
+ { + sda(!da); + }} + id='delete-check' + /> + +
+ + ); + } else { + if (props.isWfInDB) { + sda(true); + } + return ( + <> +

Are you sure you want to delete this workflow?

+ + ); + } }; export const WorkflowDetails = ({history, location, match}: RouteComponentProps) => { @@ -206,29 +186,31 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< title: workflowOperation.title.charAt(0).toUpperCase() + workflowOperation.title.slice(1), iconClassName: workflowOperation.iconClassName, action: () => { + // These are intentionally to be local variables since we cannot rely on the async calls. + let localIsWfInDB = isWfInDB; + let localIsWfInCluster = isWfInCluster; if (workflowOperation.title === 'DELETE') { popup - .confirm('Confirm', () => ) + .confirm('Confirm', () => ) .then(yes => { - // localIsWfInCluster if (yes) { - if (isWfInCluster) { + if (localIsWfInCluster) { services.workflows .delete(workflow.metadata.name, workflow.metadata.namespace) .then(() => { - setIsWfInCluster(false); + localIsWfInCluster = false; }) .catch(setError); } - if (deleteArchived) { + if (localIsWfInDB && deleteArchived) { services.workflows .deleteArchived(workflow.metadata.uid, workflow.metadata.namespace) .then(() => { - setIsWfInDB(false); + localIsWfInDB = false; }) .catch(setError); } - if (!isWfInDB && !isWfInCluster) { + if (!localIsWfInDB && !localIsWfInCluster) { navigation.goto(uiUrl(`workflows/${workflow.metadata.namespace}`)); } } @@ -395,21 +377,23 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< ); retryWatch.start(); return () => retryWatch.stop(); - }, [namespace, name, !isWfInDB]); + }, [namespace, name]); useEffect(() => { - services.workflows - .get(namespace, name) - .then(wf => { - setError(null); - setWorkflow(wf); - setIsWfInDB(true); - }) - .catch(newErr => { - setError(newErr); - navigation.goto(uiUrl(`workflows/${namespace}`)); - }); - }, [namespace, name, !isWfInCluster]); + if (!isWfInCluster) { + services.workflows + .get(namespace, name) + .then(wf => { + setError(null); + setWorkflow(wf); + setIsWfInDB(true); + }) + .catch(newErr => { + setError(newErr); + navigation.goto(uiUrl(`workflows/${namespace}`)); + }); + } + }, [namespace, name, isWfInCluster]); const openLink = (link: Link) => { const object = { From 351cc5c0c1468bd84b25efffd2a936dfd2d8b9ea Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Fri, 9 Jun 2023 15:13:13 -0400 Subject: [PATCH 15/27] fix: blank page Signed-off-by: Yuan Tang --- .../components/workflow-details/workflow-details.tsx | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx index ef66df07db55..afcfe45b3d21 100644 --- a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx +++ b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx @@ -76,9 +76,6 @@ const DeleteCheck = (props: {onChange: (changed: boolean) => void; isWfInDB: boo ); } else { - if (props.isWfInDB) { - sda(true); - } return ( <>

Are you sure you want to delete this workflow?

@@ -202,7 +199,7 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< }) .catch(setError); } - if (localIsWfInDB && deleteArchived) { + if (localIsWfInDB && (deleteArchived || !localIsWfInCluster)) { services.workflows .deleteArchived(workflow.metadata.uid, workflow.metadata.namespace) .then(() => { From 8a89cb5edbd301bc62aee4779fa7ac69fc2c837e Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Fri, 9 Jun 2023 22:27:42 -0400 Subject: [PATCH 16/27] fix: couple of fixes and TODO Signed-off-by: Yuan Tang --- .../app/shared/services/workflows-service.ts | 4 +++ .../workflow-details/workflow-details.tsx | 29 +++++++++---------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/ui/src/app/shared/services/workflows-service.ts b/ui/src/app/shared/services/workflows-service.ts index cbccc297551a..df70d6d60171 100644 --- a/ui/src/app/shared/services/workflows-service.ts +++ b/ui/src/app/shared/services/workflows-service.ts @@ -66,6 +66,10 @@ export const WorkflowsService = { return requests.get(`api/v1/workflows/${namespace}/${name}`).then(res => res.body as Workflow); }, + getArchived(namespace: string, name: string) { + return requests.get(`api/v1/archived-workflows/?name=${name}&namespace=${namespace}`).then(res => res.body as models.Workflow); + }, + watch(query: { namespace?: string; name?: string; diff --git a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx index afcfe45b3d21..abaf5780cf12 100644 --- a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx +++ b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx @@ -55,6 +55,7 @@ const DeleteCheck = (props: {onChange: (changed: boolean) => void; isWfInDB: boo // The local states are created intentionally so that the checkbox works as expected const [da, sda] = useState(false); useEffect(() => { + // TODO: Somehow this is only effective after I clicked the checkbox and then hit "cancel" and then try delete again (without clicking the checkbox again). props.onChange(da); }, [da]); if (props.isWfInDB && props.isWfInCluster) { @@ -183,33 +184,29 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< title: workflowOperation.title.charAt(0).toUpperCase() + workflowOperation.title.slice(1), iconClassName: workflowOperation.iconClassName, action: () => { - // These are intentionally to be local variables since we cannot rely on the async calls. - let localIsWfInDB = isWfInDB; - let localIsWfInCluster = isWfInCluster; if (workflowOperation.title === 'DELETE') { popup - .confirm('Confirm', () => ) + .confirm('Confirm', () => ) .then(yes => { if (yes) { - if (localIsWfInCluster) { + if (isWfInCluster) { services.workflows .delete(workflow.metadata.name, workflow.metadata.namespace) .then(() => { - localIsWfInCluster = false; + setIsWfInCluster(false); }) .catch(setError); } - if (localIsWfInDB && (deleteArchived || !localIsWfInCluster)) { + if (isWfInDB && (deleteArchived || !isWfInCluster)) { services.workflows .deleteArchived(workflow.metadata.uid, workflow.metadata.namespace) .then(() => { - localIsWfInDB = false; + setIsWfInDB(false); }) .catch(setError); } - if (!localIsWfInDB && !localIsWfInCluster) { - navigation.goto(uiUrl(`workflows/${workflow.metadata.namespace}`)); - } + navigation.goto(uiUrl(`workflows/${workflow.metadata.namespace}`)); + window.location.reload(); } }); } else { @@ -365,6 +362,7 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< setError(new Error('Artifact garbage collection failed')); } setWorkflow(e.object); + setIsWfInCluster(true); } }, err => { @@ -377,17 +375,18 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< }, [namespace, name]); useEffect(() => { - if (!isWfInCluster) { + if (!workflow && !isWfInCluster) { services.workflows - .get(namespace, name) + .getArchived(namespace, name) .then(wf => { setError(null); setWorkflow(wf); setIsWfInDB(true); }) .catch(newErr => { - setError(newErr); - navigation.goto(uiUrl(`workflows/${namespace}`)); + if (newErr.status !== 404) { + setError(newErr); + } }); } }, [namespace, name, isWfInCluster]); From b51d81076a5ffcd0a199ba44b22b9714c7fae30f Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Sat, 10 Jun 2023 21:08:37 -0400 Subject: [PATCH 17/27] fix: working with global var instead of state Signed-off-by: Yuan Tang --- .../workflow-details/workflow-details.tsx | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx index abaf5780cf12..e13d1e771df0 100644 --- a/ui/src/app/workflows/components/workflow-details/workflow-details.tsx +++ b/ui/src/app/workflows/components/workflow-details/workflow-details.tsx @@ -51,13 +51,13 @@ const INITIAL_SIDE_PANEL_WIDTH = 570; const ANIMATION_MS = 200; const ANIMATION_BUFFER_MS = 20; -const DeleteCheck = (props: {onChange: (changed: boolean) => void; isWfInDB: boolean; isWfInCluster: boolean}) => { +// This is used instead of React state since the state update is async and there's a delay for parent +// component to render with the updated state. +let globalDeleteArchived = false; + +const DeleteCheck = (props: {isWfInDB: boolean; isWfInCluster: boolean}) => { // The local states are created intentionally so that the checkbox works as expected const [da, sda] = useState(false); - useEffect(() => { - // TODO: Somehow this is only effective after I clicked the checkbox and then hit "cancel" and then try delete again (without clicking the checkbox again). - props.onChange(da); - }, [da]); if (props.isWfInDB && props.isWfInCluster) { return ( <> @@ -69,6 +69,7 @@ const DeleteCheck = (props: {onChange: (changed: boolean) => void; isWfInDB: boo checked={da} onClick={() => { sda(!da); + globalDeleteArchived = !globalDeleteArchived; }} id='delete-check' /> @@ -93,7 +94,6 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< const [namespace] = useState(match.params.namespace); const [isWfInDB, setIsWfInDB] = useState(false); const [isWfInCluster, setIsWfInCluster] = useState(false); - const [deleteArchived, setDeleteArchived] = useState(false); const [name, setName] = useState(match.params.name); const [tab, setTab] = useState(queryParams.get('tab') || 'workflow'); const [nodeId, setNodeId] = useState(queryParams.get('nodeId')); @@ -186,7 +186,7 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< action: () => { if (workflowOperation.title === 'DELETE') { popup - .confirm('Confirm', () => ) + .confirm('Confirm', () => ) .then(yes => { if (yes) { if (isWfInCluster) { @@ -197,7 +197,7 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< }) .catch(setError); } - if (isWfInDB && (deleteArchived || !isWfInCluster)) { + if (isWfInDB && (globalDeleteArchived || !isWfInCluster)) { services.workflows .deleteArchived(workflow.metadata.uid, workflow.metadata.namespace) .then(() => { @@ -206,6 +206,8 @@ export const WorkflowDetails = ({history, location, match}: RouteComponentProps< .catch(setError); } navigation.goto(uiUrl(`workflows/${workflow.metadata.namespace}`)); + // TODO: This is a temporary workaround so that the list of workflows + // is correctly displayed. Workflow list page needs to be more responsive. window.location.reload(); } }); From 544aefb64c455f72193cf5e38256c251d92e8229 Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Fri, 16 Jun 2023 23:02:56 -0400 Subject: [PATCH 18/27] fix: feedback Signed-off-by: Yuan Tang --- server/workflow/workflow_server.go | 29 ++++----- server/workflow/workflow_server_test.go | 25 ++++++++ ...ons-map.ts => workflow-operations-map.tsx} | 61 ++++++++++++++++++- 3 files changed, 100 insertions(+), 15 deletions(-) rename ui/src/app/shared/{workflow-operations-map.ts => workflow-operations-map.tsx} (52%) diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go index 4817c54e0023..a19f1712d0d2 100644 --- a/server/workflow/workflow_server.go +++ b/server/workflow/workflow_server.go @@ -129,13 +129,21 @@ func (s *workflowServer) GetWorkflow(ctx context.Context, req *workflowpkg.Workf return wf, nil } -func containsWorkflow(s v1alpha1.WorkflowList, e v1alpha1.Workflow) bool { - for _, wf := range s.Items { - if wf.UID == e.UID { - return true - } +func mergeWithArchivedWorkflows(liveWfs v1alpha1.WorkflowList, archivedWfs v1alpha1.WorkflowList) v1alpha1.WorkflowList { + var finalWfs []v1alpha1.Workflow + var uidToWfs = map[types.UID][]v1alpha1.Workflow{} + for _, item := range liveWfs.Items { + uidToWfs[item.UID] = append(uidToWfs[item.UID], item) + } + for _, item := range archivedWfs.Items { + uidToWfs[item.UID] = append(uidToWfs[item.UID], item) + } + for _, v := range uidToWfs { + finalWfs = append(finalWfs, v[0]) } - return false + finalWfsList := v1alpha1.WorkflowList{Items: finalWfs} + sort.Sort(finalWfsList.Items) + return finalWfsList } func (s *workflowServer) ListWorkflows(ctx context.Context, req *workflowpkg.WorkflowListRequest) (*wfv1.WorkflowList, error) { @@ -159,11 +167,7 @@ func (s *workflowServer) ListWorkflows(ctx context.Context, req *workflowpkg.Wor log.Warnf("unable to list archived workflows:%v", err) } else { if archivedWfList != nil { - for _, item := range archivedWfList.Items { - if !containsWorkflow(*wfList, item) { - wfList.Items = append(wfList.Items, item) - } - } + mergeWithArchivedWorkflows(*wfList, *archivedWfList) } } @@ -184,9 +188,6 @@ func (s *workflowServer) ListWorkflows(ctx context.Context, req *workflowpkg.Wor } } - // we make no promises about the overall list sorting, we just sort each page - sort.Sort(wfList.Items) - res := &wfv1.WorkflowList{ListMeta: metav1.ListMeta{Continue: wfList.Continue, ResourceVersion: wfList.ResourceVersion}, Items: wfList.Items} newRes := &wfv1.WorkflowList{} if ok, err := cleaner.Clean(res, &newRes); err != nil { diff --git a/server/workflow/workflow_server_test.go b/server/workflow/workflow_server_test.go index 1d17bba26bca..b0781761ac62 100644 --- a/server/workflow/workflow_server_test.go +++ b/server/workflow/workflow_server_test.go @@ -648,6 +648,31 @@ func (t testWatchWorkflowServer) Send(*workflowpkg.WorkflowWatchEvent) error { panic("implement me") } +func TestMergeWithArchivedWorkflows(t *testing.T) { + liveWfList := v1alpha1.WorkflowList{ + Items: []v1alpha1.Workflow{ + {ObjectMeta: metav1.ObjectMeta{UID: "1"}}, + {ObjectMeta: metav1.ObjectMeta{UID: "2"}}, + }, + } + archivedWfList := v1alpha1.WorkflowList{ + Items: []v1alpha1.Workflow{ + {ObjectMeta: metav1.ObjectMeta{UID: "1"}}, + {ObjectMeta: metav1.ObjectMeta{UID: "3"}}, + {ObjectMeta: metav1.ObjectMeta{UID: "2"}}, + }, + } + assert.Equal(t, + mergeWithArchivedWorkflows(liveWfList, archivedWfList), + v1alpha1.WorkflowList{ + Items: []v1alpha1.Workflow{ + {ObjectMeta: metav1.ObjectMeta{UID: "3"}}, + {ObjectMeta: metav1.ObjectMeta{UID: "2"}}, + {ObjectMeta: metav1.ObjectMeta{UID: "1"}}, + }, + }) +} + func TestWatchWorkflows(t *testing.T) { server, ctx := getWorkflowServer() wf := &v1alpha1.Workflow{ diff --git a/ui/src/app/shared/workflow-operations-map.ts b/ui/src/app/shared/workflow-operations-map.tsx similarity index 52% rename from ui/src/app/shared/workflow-operations-map.ts rename to ui/src/app/shared/workflow-operations-map.tsx index 665b32600d8f..98cbb91ace03 100644 --- a/ui/src/app/shared/workflow-operations-map.ts +++ b/ui/src/app/shared/workflow-operations-map.tsx @@ -2,6 +2,9 @@ import {NodePhase, Workflow} from '../../models'; import {services} from './services'; import {WorkflowDeleteResponse} from './services/responses'; import {Utils} from './utils'; +import {useState} from "react"; +import * as React from "react"; +import {uiUrl} from "./base"; export type OperationDisabled = { [action in WorkflowOperationName]: boolean; @@ -22,6 +25,39 @@ export interface WorkflowOperations { [name: string]: WorkflowOperation; } +let globalDeleteArchived = false; + +const DeleteCheck = (props: {isWfInDB: boolean; isWfInCluster: boolean}) => { + // The local states are created intentionally so that the checkbox works as expected + const [da, sda] = useState(false); + if (props.isWfInDB && props.isWfInCluster) { + return ( + <> +

Are you sure you want to delete this workflow?

+
+ { + sda(!da); + globalDeleteArchived = !globalDeleteArchived; + }} + id='delete-check' + /> + +
+ + ); + } else { + return ( + <> +

Are you sure you want to delete this workflow?

+ + ); + } +}; + export const WorkflowOperationsMap: WorkflowOperations = { RETRY: { title: 'RETRY', @@ -66,6 +102,29 @@ export const WorkflowOperationsMap: WorkflowOperations = { title: 'DELETE', iconClassName: 'fa fa-trash', disabled: () => false, - action: (wf: Workflow) => services.workflows.delete(wf.metadata.name, wf.metadata.namespace) + action: (wf: Workflow) => { + popup + .confirm('Confirm', () => ) + .then(yes => { + if (yes) { + if (isWfInCluster) { + services.workflows + .delete(workflow.metadata.name, workflow.metadata.namespace) + .then(() => { + setIsWfInCluster(false); + }) + .catch(setError); + } + if (isWfInDB && (globalDeleteArchived || !isWfInCluster)) { + services.workflows + .deleteArchived(workflow.metadata.uid, workflow.metadata.namespace) + .then(() => { + setIsWfInDB(false); + }) + .catch(setError); + } + } + }); + } } }; From cb9ca5ce27bb54ff261d036e60152b1556c8f380 Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Sat, 17 Jun 2023 00:29:42 -0400 Subject: [PATCH 19/27] fix: Address feedback Signed-off-by: Yuan Tang --- server/workflow/workflow_server.go | 12 ++++-------- server/workflow/workflow_server_test.go | 17 ++++++++--------- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go index a19f1712d0d2..c88d716c125c 100644 --- a/server/workflow/workflow_server.go +++ b/server/workflow/workflow_server.go @@ -129,7 +129,7 @@ func (s *workflowServer) GetWorkflow(ctx context.Context, req *workflowpkg.Workf return wf, nil } -func mergeWithArchivedWorkflows(liveWfs v1alpha1.WorkflowList, archivedWfs v1alpha1.WorkflowList) v1alpha1.WorkflowList { +func mergeWithArchivedWorkflows(liveWfs v1alpha1.WorkflowList, archivedWfs v1alpha1.WorkflowList) *v1alpha1.WorkflowList { var finalWfs []v1alpha1.Workflow var uidToWfs = map[types.UID][]v1alpha1.Workflow{} for _, item := range liveWfs.Items { @@ -143,7 +143,7 @@ func mergeWithArchivedWorkflows(liveWfs v1alpha1.WorkflowList, archivedWfs v1alp } finalWfsList := v1alpha1.WorkflowList{Items: finalWfs} sort.Sort(finalWfsList.Items) - return finalWfsList + return &finalWfsList } func (s *workflowServer) ListWorkflows(ctx context.Context, req *workflowpkg.WorkflowListRequest) (*wfv1.WorkflowList, error) { @@ -163,12 +163,8 @@ func (s *workflowServer) ListWorkflows(ctx context.Context, req *workflowpkg.Wor NamePrefix: "", Namespace: req.Namespace, }) - if err != nil { - log.Warnf("unable to list archived workflows:%v", err) - } else { - if archivedWfList != nil { - mergeWithArchivedWorkflows(*wfList, *archivedWfList) - } + if err == nil && archivedWfList != nil { + wfList = mergeWithArchivedWorkflows(*wfList, *archivedWfList) } cleaner := fields.NewCleaner(req.Fields) diff --git a/server/workflow/workflow_server_test.go b/server/workflow/workflow_server_test.go index b0781761ac62..21b785ba51bb 100644 --- a/server/workflow/workflow_server_test.go +++ b/server/workflow/workflow_server_test.go @@ -662,15 +662,14 @@ func TestMergeWithArchivedWorkflows(t *testing.T) { {ObjectMeta: metav1.ObjectMeta{UID: "2"}}, }, } - assert.Equal(t, - mergeWithArchivedWorkflows(liveWfList, archivedWfList), - v1alpha1.WorkflowList{ - Items: []v1alpha1.Workflow{ - {ObjectMeta: metav1.ObjectMeta{UID: "3"}}, - {ObjectMeta: metav1.ObjectMeta{UID: "2"}}, - {ObjectMeta: metav1.ObjectMeta{UID: "1"}}, - }, - }) + expectedWfList := v1alpha1.WorkflowList{ + Items: []v1alpha1.Workflow{ + {ObjectMeta: metav1.ObjectMeta{UID: "3"}}, + {ObjectMeta: metav1.ObjectMeta{UID: "2"}}, + {ObjectMeta: metav1.ObjectMeta{UID: "1"}}, + }, + } + assert.Equal(t, mergeWithArchivedWorkflows(liveWfList, archivedWfList), &expectedWfList) } func TestWatchWorkflows(t *testing.T) { From a92363fd5109a078e3b40077ce30fcf5b811e4a1 Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Sat, 17 Jun 2023 21:11:57 -0400 Subject: [PATCH 20/27] fix: undo temporary changes Signed-off-by: Yuan Tang --- ui/src/app/shared/workflow-operations-map.tsx | 61 +------------------ 1 file changed, 1 insertion(+), 60 deletions(-) diff --git a/ui/src/app/shared/workflow-operations-map.tsx b/ui/src/app/shared/workflow-operations-map.tsx index 98cbb91ace03..665b32600d8f 100644 --- a/ui/src/app/shared/workflow-operations-map.tsx +++ b/ui/src/app/shared/workflow-operations-map.tsx @@ -2,9 +2,6 @@ import {NodePhase, Workflow} from '../../models'; import {services} from './services'; import {WorkflowDeleteResponse} from './services/responses'; import {Utils} from './utils'; -import {useState} from "react"; -import * as React from "react"; -import {uiUrl} from "./base"; export type OperationDisabled = { [action in WorkflowOperationName]: boolean; @@ -25,39 +22,6 @@ export interface WorkflowOperations { [name: string]: WorkflowOperation; } -let globalDeleteArchived = false; - -const DeleteCheck = (props: {isWfInDB: boolean; isWfInCluster: boolean}) => { - // The local states are created intentionally so that the checkbox works as expected - const [da, sda] = useState(false); - if (props.isWfInDB && props.isWfInCluster) { - return ( - <> -

Are you sure you want to delete this workflow?

-
- { - sda(!da); - globalDeleteArchived = !globalDeleteArchived; - }} - id='delete-check' - /> - -
- - ); - } else { - return ( - <> -

Are you sure you want to delete this workflow?

- - ); - } -}; - export const WorkflowOperationsMap: WorkflowOperations = { RETRY: { title: 'RETRY', @@ -102,29 +66,6 @@ export const WorkflowOperationsMap: WorkflowOperations = { title: 'DELETE', iconClassName: 'fa fa-trash', disabled: () => false, - action: (wf: Workflow) => { - popup - .confirm('Confirm', () => ) - .then(yes => { - if (yes) { - if (isWfInCluster) { - services.workflows - .delete(workflow.metadata.name, workflow.metadata.namespace) - .then(() => { - setIsWfInCluster(false); - }) - .catch(setError); - } - if (isWfInDB && (globalDeleteArchived || !isWfInCluster)) { - services.workflows - .deleteArchived(workflow.metadata.uid, workflow.metadata.namespace) - .then(() => { - setIsWfInDB(false); - }) - .catch(setError); - } - } - }); - } + action: (wf: Workflow) => services.workflows.delete(wf.metadata.name, wf.metadata.namespace) } }; From a4be657c66da41a1cd959e35d92200a29a25f7f6 Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Mon, 19 Jun 2023 12:47:25 -0400 Subject: [PATCH 21/27] fix: print error Signed-off-by: Yuan Tang --- server/workflow/workflow_server.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go index c88d716c125c..607cd7c177c8 100644 --- a/server/workflow/workflow_server.go +++ b/server/workflow/workflow_server.go @@ -163,8 +163,12 @@ func (s *workflowServer) ListWorkflows(ctx context.Context, req *workflowpkg.Wor NamePrefix: "", Namespace: req.Namespace, }) - if err == nil && archivedWfList != nil { - wfList = mergeWithArchivedWorkflows(*wfList, *archivedWfList) + if err != nil { + log.Warnf("unable to list archived workflows:%v", err) + } else { + if archivedWfList != nil { + wfList = mergeWithArchivedWorkflows(*wfList, *archivedWfList) + } } cleaner := fields.NewCleaner(req.Fields) From a4ed8ad6b7903eae64d76047bed19780b875ea6e Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Wed, 28 Jun 2023 21:35:41 -0400 Subject: [PATCH 22/27] fix: correct pagination and handle unmarshalling errors Signed-off-by: Yuan Tang --- persist/sqldb/workflow_archive.go | 4 +++- server/workflow/workflow_server.go | 16 ++++++++++++---- server/workflow/workflow_server_test.go | 3 ++- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/persist/sqldb/workflow_archive.go b/persist/sqldb/workflow_archive.go index 984d72fb0add..f4dda103c4ff 100644 --- a/persist/sqldb/workflow_archive.go +++ b/persist/sqldb/workflow_archive.go @@ -176,7 +176,9 @@ func (r *workflowArchive) ListWorkflows(namespace string, name string, namePrefi wf := wfs[i] err = json.Unmarshal([]byte(archivedWf.Workflow), &wf) if err != nil { - return nil, err + // We will filter out all the workflows with empty UIDs + wf.UID = "" + log.WithFields(log.Fields{"workflowUID": archivedWf.UID, "workflowName": archivedWf.Name}).Errorln("unable to unmarshal workflow from database") } wfs[i] = wf } diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go index 607cd7c177c8..042196995958 100644 --- a/server/workflow/workflow_server.go +++ b/server/workflow/workflow_server.go @@ -129,20 +129,28 @@ func (s *workflowServer) GetWorkflow(ctx context.Context, req *workflowpkg.Workf return wf, nil } -func mergeWithArchivedWorkflows(liveWfs v1alpha1.WorkflowList, archivedWfs v1alpha1.WorkflowList) *v1alpha1.WorkflowList { +func mergeWithArchivedWorkflows(liveWfs v1alpha1.WorkflowList, archivedWfs v1alpha1.WorkflowList, numWfsToKeep int) *v1alpha1.WorkflowList { var finalWfs []v1alpha1.Workflow var uidToWfs = map[types.UID][]v1alpha1.Workflow{} for _, item := range liveWfs.Items { uidToWfs[item.UID] = append(uidToWfs[item.UID], item) } for _, item := range archivedWfs.Items { - uidToWfs[item.UID] = append(uidToWfs[item.UID], item) + // The workflows with empty UIDs are the ones that we failed to unmarshall in `workflowArchive.ListWorkflows`. + if item.UID != "" { + uidToWfs[item.UID] = append(uidToWfs[item.UID], item) + } } + numWfs := 0 for _, v := range uidToWfs { - finalWfs = append(finalWfs, v[0]) + if numWfsToKeep < 0 || numWfs < numWfsToKeep { + finalWfs = append(finalWfs, v[0]) + numWfs += 1 + } } finalWfsList := v1alpha1.WorkflowList{Items: finalWfs} sort.Sort(finalWfsList.Items) + return &finalWfsList } @@ -167,7 +175,7 @@ func (s *workflowServer) ListWorkflows(ctx context.Context, req *workflowpkg.Wor log.Warnf("unable to list archived workflows:%v", err) } else { if archivedWfList != nil { - wfList = mergeWithArchivedWorkflows(*wfList, *archivedWfList) + wfList = mergeWithArchivedWorkflows(*wfList, *archivedWfList, int(listOption.Limit)) } } diff --git a/server/workflow/workflow_server_test.go b/server/workflow/workflow_server_test.go index 21b785ba51bb..43d062f0f0c1 100644 --- a/server/workflow/workflow_server_test.go +++ b/server/workflow/workflow_server_test.go @@ -669,7 +669,8 @@ func TestMergeWithArchivedWorkflows(t *testing.T) { {ObjectMeta: metav1.ObjectMeta{UID: "1"}}, }, } - assert.Equal(t, mergeWithArchivedWorkflows(liveWfList, archivedWfList), &expectedWfList) + assert.Len(t, mergeWithArchivedWorkflows(liveWfList, archivedWfList, -1).Items, len(expectedWfList.Items)) + assert.Len(t, mergeWithArchivedWorkflows(liveWfList, archivedWfList, 2).Items, 2) } func TestWatchWorkflows(t *testing.T) { From 84d778f8490b1ddd8ddd6d95ea8c72417a15201e Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Thu, 29 Jun 2023 00:39:22 -0400 Subject: [PATCH 23/27] fix: use 0 instead of -1 Signed-off-by: Yuan Tang --- server/workflow/workflow_server.go | 2 +- server/workflow/workflow_server_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go index 042196995958..ac8388dbe32a 100644 --- a/server/workflow/workflow_server.go +++ b/server/workflow/workflow_server.go @@ -143,7 +143,7 @@ func mergeWithArchivedWorkflows(liveWfs v1alpha1.WorkflowList, archivedWfs v1alp } numWfs := 0 for _, v := range uidToWfs { - if numWfsToKeep < 0 || numWfs < numWfsToKeep { + if numWfsToKeep == 0 || numWfs < numWfsToKeep { finalWfs = append(finalWfs, v[0]) numWfs += 1 } diff --git a/server/workflow/workflow_server_test.go b/server/workflow/workflow_server_test.go index 43d062f0f0c1..90b251386aef 100644 --- a/server/workflow/workflow_server_test.go +++ b/server/workflow/workflow_server_test.go @@ -669,7 +669,7 @@ func TestMergeWithArchivedWorkflows(t *testing.T) { {ObjectMeta: metav1.ObjectMeta{UID: "1"}}, }, } - assert.Len(t, mergeWithArchivedWorkflows(liveWfList, archivedWfList, -1).Items, len(expectedWfList.Items)) + assert.Len(t, mergeWithArchivedWorkflows(liveWfList, archivedWfList, 0).Items, len(expectedWfList.Items)) assert.Len(t, mergeWithArchivedWorkflows(liveWfList, archivedWfList, 2).Items, 2) } From 4e53ada39a4e585dc6da814f129fee341a98465b Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Thu, 29 Jun 2023 14:57:32 -0400 Subject: [PATCH 24/27] fix: Add missing list meta Signed-off-by: Yuan Tang --- server/workflow/workflow_server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go index ac8388dbe32a..99f3eab0e08b 100644 --- a/server/workflow/workflow_server.go +++ b/server/workflow/workflow_server.go @@ -148,7 +148,7 @@ func mergeWithArchivedWorkflows(liveWfs v1alpha1.WorkflowList, archivedWfs v1alp numWfs += 1 } } - finalWfsList := v1alpha1.WorkflowList{Items: finalWfs} + finalWfsList := v1alpha1.WorkflowList{Items: finalWfs, ListMeta: liveWfs.ListMeta} sort.Sort(finalWfsList.Items) return &finalWfsList From b6b15ec6296863d6b0b61ff01c4697154a159295 Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Thu, 29 Jun 2023 15:06:15 -0400 Subject: [PATCH 25/27] fix: simplify Signed-off-by: Yuan Tang --- persist/sqldb/workflow_archive.go | 11 +++++------ server/workflow/workflow_server.go | 5 +---- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/persist/sqldb/workflow_archive.go b/persist/sqldb/workflow_archive.go index f4dda103c4ff..15c780071b29 100644 --- a/persist/sqldb/workflow_archive.go +++ b/persist/sqldb/workflow_archive.go @@ -171,16 +171,15 @@ func (r *workflowArchive) ListWorkflows(namespace string, name string, namePrefi if err != nil { return nil, err } - wfs := make(wfv1.Workflows, len(archivedWfs)) - for i, archivedWf := range archivedWfs { - wf := wfs[i] + wfs := make(wfv1.Workflows, 0) + for _, archivedWf := range archivedWfs { + wf := wfv1.Workflow{} err = json.Unmarshal([]byte(archivedWf.Workflow), &wf) if err != nil { - // We will filter out all the workflows with empty UIDs - wf.UID = "" log.WithFields(log.Fields{"workflowUID": archivedWf.UID, "workflowName": archivedWf.Name}).Errorln("unable to unmarshal workflow from database") + } else { + wfs = append(wfs, wf) } - wfs[i] = wf } return wfs, nil } diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go index 99f3eab0e08b..b7540c579fc1 100644 --- a/server/workflow/workflow_server.go +++ b/server/workflow/workflow_server.go @@ -136,10 +136,7 @@ func mergeWithArchivedWorkflows(liveWfs v1alpha1.WorkflowList, archivedWfs v1alp uidToWfs[item.UID] = append(uidToWfs[item.UID], item) } for _, item := range archivedWfs.Items { - // The workflows with empty UIDs are the ones that we failed to unmarshall in `workflowArchive.ListWorkflows`. - if item.UID != "" { - uidToWfs[item.UID] = append(uidToWfs[item.UID], item) - } + uidToWfs[item.UID] = append(uidToWfs[item.UID], item) } numWfs := 0 for _, v := range uidToWfs { From f6df9090a98ce8858d9a0bb27891e2d8b4d6e34f Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Sun, 2 Jul 2023 00:36:47 -0400 Subject: [PATCH 26/27] fix: keep sorting Signed-off-by: Yuan Tang --- server/workflow/workflow_server.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go index b7540c579fc1..7ff909060052 100644 --- a/server/workflow/workflow_server.go +++ b/server/workflow/workflow_server.go @@ -146,8 +146,6 @@ func mergeWithArchivedWorkflows(liveWfs v1alpha1.WorkflowList, archivedWfs v1alp } } finalWfsList := v1alpha1.WorkflowList{Items: finalWfs, ListMeta: liveWfs.ListMeta} - sort.Sort(finalWfsList.Items) - return &finalWfsList } @@ -193,6 +191,9 @@ func (s *workflowServer) ListWorkflows(ctx context.Context, req *workflowpkg.Wor } } + // we make no promises about the overall list sorting, we just sort each page + sort.Sort(wfList.Items) + res := &wfv1.WorkflowList{ListMeta: metav1.ListMeta{Continue: wfList.Continue, ResourceVersion: wfList.ResourceVersion}, Items: wfList.Items} newRes := &wfv1.WorkflowList{} if ok, err := cleaner.Clean(res, &newRes); err != nil { From 2461b7c401169a7e27daf163f47b0c1387f24a8f Mon Sep 17 00:00:00 2001 From: Yuan Tang Date: Thu, 6 Jul 2023 23:48:11 -0400 Subject: [PATCH 27/27] fix: Consistent result and test correctness Signed-off-by: Yuan Tang --- server/workflow/workflow_server.go | 16 +++++++---- server/workflow/workflow_server_test.go | 36 ++++++++++--------------- 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go index 7ff909060052..dab707dc4b76 100644 --- a/server/workflow/workflow_server.go +++ b/server/workflow/workflow_server.go @@ -130,7 +130,7 @@ func (s *workflowServer) GetWorkflow(ctx context.Context, req *workflowpkg.Workf } func mergeWithArchivedWorkflows(liveWfs v1alpha1.WorkflowList, archivedWfs v1alpha1.WorkflowList, numWfsToKeep int) *v1alpha1.WorkflowList { - var finalWfs []v1alpha1.Workflow + var mergedWfs []v1alpha1.Workflow var uidToWfs = map[types.UID][]v1alpha1.Workflow{} for _, item := range liveWfs.Items { uidToWfs[item.UID] = append(uidToWfs[item.UID], item) @@ -138,15 +138,21 @@ func mergeWithArchivedWorkflows(liveWfs v1alpha1.WorkflowList, archivedWfs v1alp for _, item := range archivedWfs.Items { uidToWfs[item.UID] = append(uidToWfs[item.UID], item) } - numWfs := 0 + for _, v := range uidToWfs { + mergedWfs = append(mergedWfs, v[0]) + } + mergedWfsList := v1alpha1.WorkflowList{Items: mergedWfs, ListMeta: liveWfs.ListMeta} + sort.Sort(mergedWfsList.Items) + numWfs := 0 + var finalWfs []v1alpha1.Workflow + for _, item := range mergedWfsList.Items { if numWfsToKeep == 0 || numWfs < numWfsToKeep { - finalWfs = append(finalWfs, v[0]) + finalWfs = append(finalWfs, item) numWfs += 1 } } - finalWfsList := v1alpha1.WorkflowList{Items: finalWfs, ListMeta: liveWfs.ListMeta} - return &finalWfsList + return &v1alpha1.WorkflowList{Items: finalWfs, ListMeta: liveWfs.ListMeta} } func (s *workflowServer) ListWorkflows(ctx context.Context, req *workflowpkg.WorkflowListRequest) (*wfv1.WorkflowList, error) { diff --git a/server/workflow/workflow_server_test.go b/server/workflow/workflow_server_test.go index 90b251386aef..f747f8f42997 100644 --- a/server/workflow/workflow_server_test.go +++ b/server/workflow/workflow_server_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "testing" + "time" "github.com/go-jose/go-jose/v3/jwt" "github.com/stretchr/testify/assert" @@ -649,28 +650,19 @@ func (t testWatchWorkflowServer) Send(*workflowpkg.WorkflowWatchEvent) error { } func TestMergeWithArchivedWorkflows(t *testing.T) { - liveWfList := v1alpha1.WorkflowList{ - Items: []v1alpha1.Workflow{ - {ObjectMeta: metav1.ObjectMeta{UID: "1"}}, - {ObjectMeta: metav1.ObjectMeta{UID: "2"}}, - }, - } - archivedWfList := v1alpha1.WorkflowList{ - Items: []v1alpha1.Workflow{ - {ObjectMeta: metav1.ObjectMeta{UID: "1"}}, - {ObjectMeta: metav1.ObjectMeta{UID: "3"}}, - {ObjectMeta: metav1.ObjectMeta{UID: "2"}}, - }, - } - expectedWfList := v1alpha1.WorkflowList{ - Items: []v1alpha1.Workflow{ - {ObjectMeta: metav1.ObjectMeta{UID: "3"}}, - {ObjectMeta: metav1.ObjectMeta{UID: "2"}}, - {ObjectMeta: metav1.ObjectMeta{UID: "1"}}, - }, - } - assert.Len(t, mergeWithArchivedWorkflows(liveWfList, archivedWfList, 0).Items, len(expectedWfList.Items)) - assert.Len(t, mergeWithArchivedWorkflows(liveWfList, archivedWfList, 2).Items, 2) + timeNow := time.Now() + wf1 := v1alpha1.Workflow{ + ObjectMeta: metav1.ObjectMeta{UID: "1", CreationTimestamp: metav1.Time{Time: timeNow.Add(time.Second)}}} + wf2 := v1alpha1.Workflow{ + ObjectMeta: metav1.ObjectMeta{UID: "2", CreationTimestamp: metav1.Time{Time: timeNow.Add(2 * time.Second)}}} + wf3 := v1alpha1.Workflow{ + ObjectMeta: metav1.ObjectMeta{UID: "3", CreationTimestamp: metav1.Time{Time: timeNow.Add(3 * time.Second)}}} + liveWfList := v1alpha1.WorkflowList{Items: []v1alpha1.Workflow{wf1, wf2}} + archivedWfList := v1alpha1.WorkflowList{Items: []v1alpha1.Workflow{wf1, wf3, wf2}} + expectedWfList := v1alpha1.WorkflowList{Items: []v1alpha1.Workflow{wf3, wf2, wf1}} + expectedShortWfList := v1alpha1.WorkflowList{Items: []v1alpha1.Workflow{wf3, wf2}} + assert.Equal(t, expectedWfList.Items, mergeWithArchivedWorkflows(liveWfList, archivedWfList, 0).Items) + assert.Equal(t, expectedShortWfList.Items, mergeWithArchivedWorkflows(liveWfList, archivedWfList, 2).Items) } func TestWatchWorkflows(t *testing.T) {