Merge pull request #2779 from jeff-phillips-18/pipeline-fixes

[RHOAIENG-6647] Fix to show all artifacts in pipeline topology view

frontend/package.json

@@ -63,7 +63,7 @@
     "@patternfly/react-styles": "^5.2.1",
     "@patternfly/react-table": "^5.2.1",
     "@patternfly/react-tokens": "^5.2.1",
-    "@patternfly/react-topology": "^5.3.0-prerelease.12",
+    "@patternfly/react-topology": "^5.4.0-prerelease.1",
     "@patternfly/react-virtualized-extension": "^5.0.0",
     "@types/classnames": "^2.3.1",
     "axios": "^1.6.4",

frontend/src/concepts/pipelines/topology/__tests__/usePipelineTaskTopology.spec.ts

@@ -0,0 +1,22 @@
+import { DEFAULT_TASK_NODE_TYPE } from '@patternfly/react-topology';
+import { testHook } from '~/__tests__/unit/testUtils/hooks';
+import { usePipelineTaskTopology } from '~/concepts/pipelines/topology';
+import { mockLargePipelineSpec } from '~/concepts/pipelines/topology/__tests__/mockPipelineSpec';
+import { ICON_TASK_NODE_TYPE } from '~/concepts/topology/utils';
+import { EXECUTION_TASK_NODE_TYPE } from '~/concepts/topology/const';
+
+describe('usePipelineTaskTopology', () => {
+  it('returns the correct number of nodes', () => {
+    const renderResult = testHook(usePipelineTaskTopology)(mockLargePipelineSpec);
+    const nodes = renderResult.result.current;
+
+    const tasks = nodes.filter((n) => n.type === DEFAULT_TASK_NODE_TYPE);
+    const groups = nodes.filter((n) => n.type === EXECUTION_TASK_NODE_TYPE);
+    const artifactNodes = nodes.filter((n) => n.type === ICON_TASK_NODE_TYPE);
+
+    expect(nodes).toHaveLength(86);
+    expect(tasks).toHaveLength(35);
+    expect(groups).toHaveLength(5);
+    expect(artifactNodes).toHaveLength(46);
+  });
+});

frontend/src/concepts/pipelines/topology/usePipelineTaskTopology.ts → frontend/src/concepts/pipelines/topology/usePipelineTaskTopology.tsx

@@ -1,3 +1,4 @@
+import * as React from 'react';
 import {
   PipelineComponentsKF,
   PipelineExecutorsKF,
@@ -23,9 +24,11 @@ import {
 } from './parseUtils';
 import { PipelineTask, PipelineTaskRunStatus } from './pipelineTaskTypes';
 
-const EMPTY_STATE: PipelineNodeModelExpanded[] = [];
+const idForTaskArtifact = (groupId: string | undefined, artifactId: string) =>
+  groupId ? `${groupId}-ARTIFACT-${artifactId}` : artifactId;
 
-const getNodeArtifacts = (
+const getTaskArtifacts = (
+  groupId: string | undefined,
   taskId: string,
   status: PipelineTaskRunStatus | undefined,
   componentRef: string,
@@ -42,14 +45,14 @@ const getNodeArtifacts = (
       const { artifactId } =
         artifactNodeData?.find((a) => artifactKey === a.outputArtifactKey) ?? {};
 
-      // if no node needs it as an input, we don't really need a well known id
-      const id = artifactId ?? artifactKey;
+      // if no node needs it as an input, we don't really need a well known id, prepend taskId to ensure uniqueness
+      const id = idForTaskArtifact(groupId, artifactId ?? artifactKey);
 
       const artifactPipelineTask: PipelineTask = {
         type: 'artifact',
         name: label,
         inputs: {
-          artifacts: [{ label: id, type: composeArtifactType(data) }],
+          artifacts: [{ label, type: composeArtifactType(data) }],
         },
       };
 
@@ -68,7 +71,8 @@ const getNodeArtifacts = (
   return artifactNodes;
 };
 
-const getNestedNodes = (
+const getNodesForTasks = (
+  groupId: string | undefined,
   spec: PipelineSpecVariable,
   items: Record<string, TaskKF>,
   components: PipelineComponentsKF,
@@ -89,6 +93,7 @@ const getNestedNodes = (
     const status =
       parseRuntimeInfoFromExecutions(taskId, executions) ||
       parseRuntimeInfoFromRunDetails(taskId, runDetails);
+    const runStatus = translateStatusForNode(status?.state);
 
     const runAfter: string[] = details.dependentTasks ?? [];
     const hasSubTask =
@@ -108,7 +113,8 @@ const getNestedNodes = (
       volumeMounts: parseVolumeMounts(spec.platform_spec, executorLabel),
     };
 
-    const artifactNodes = getNodeArtifacts(
+    const artifactNodes = getTaskArtifacts(
+      groupId,
       taskId,
       status,
       componentRef,
@@ -120,33 +126,45 @@ const getNestedNodes = (
       children.push(...artifactNodes.map((n) => n.id));
     }
 
+    if (details.dependentTasks) {
+      // This task's runAfters may need artifact relationships -- find those artifactIds
+      runAfter.push(
+        ...details.dependentTasks
+          .map((dependantTaskId) => {
+            const art = taskArtifactMap[dependantTaskId];
+            return art ? art.map((v) => idForTaskArtifact(groupId, v.artifactId)) : null;
+          })
+          .filter((v): v is string[] => !!v)
+          .flat(),
+      );
+    }
+
     if (hasSubTask && subTasks) {
-      const [nestedNodes, nestedChildren] = getNestedNodes(
+      const subTasksArtifactMap = parseTasksForArtifactRelationship(subTasks);
+
+      const [nestedNodes, taskChildren] = getNodesForTasks(
+        taskId,
         spec,
         subTasks,
         components,
         executors,
         componentArtifactMap,
-        taskArtifactMap,
+        subTasksArtifactMap,
         runDetails,
         executions,
       );
-      const newChildren = nestedChildren.filter((child) => !nodes.find((n) => n.id === child));
-      const newNodes = nestedNodes.filter((node) => !nodes.find((n) => n.id === node.id));
 
       const itemNode = createGroupNode(
         taskId,
         taskName,
         pipelineTask,
         runAfter,
-        translateStatusForNode(status?.state),
-        newChildren,
+        runStatus,
+        taskChildren,
       );
-      nodes.push(itemNode, ...newNodes);
+      nodes.push(itemNode, ...nestedNodes);
     } else {
-      nodes.push(
-        createNode(taskId, taskName, pipelineTask, runAfter, translateStatusForNode(status?.state)),
-      );
+      nodes.push(createNode(taskId, taskName, pipelineTask, runAfter, runStatus));
     }
     children.push(taskId);
   });
@@ -158,104 +176,39 @@ export const usePipelineTaskTopology = (
   spec?: PipelineSpecVariable,
   runDetails?: RunDetailsKF,
   executions?: Execution[] | null,
-): PipelineNodeModelExpanded[] => {
-  if (!spec) {
-    return EMPTY_STATE;
-  }
-  const pipelineSpec = spec.pipeline_spec ?? spec;
-
-  const {
-    components,
-    deploymentSpec: { executors },
-    root: {
-      dag: { tasks },
-    },
-  } = pipelineSpec;
-
-  const componentArtifactMap = parseComponentsForArtifactRelationship(components);
-  const taskArtifactMap = parseTasksForArtifactRelationship(tasks);
-
-  return Object.entries(tasks).reduce<PipelineNodeModelExpanded[]>((acc, [taskId, taskValue]) => {
-    const taskName = taskValue.taskInfo.name;
-
-    const componentRef = taskValue.componentRef.name;
-    const component = components[componentRef];
-    const isGroupNode = !!component?.dag;
-    const groupTasks = component?.dag?.tasks;
-
-    const executorLabel = component?.executorLabel;
-    const executor = executorLabel ? executors[executorLabel] : undefined;
-
-    const status =
-      parseRuntimeInfoFromExecutions(taskId, executions) ||
-      parseRuntimeInfoFromRunDetails(taskId, runDetails);
-
-    const nodes: PipelineNodeModelExpanded[] = [];
-    const runAfter: string[] = taskValue.dependentTasks ?? [];
-
-    const artifactNodes = getNodeArtifacts(
-      taskId,
-      status,
-      componentRef,
+): PipelineNodeModelExpanded[] =>
+  React.useMemo(() => {
+    if (!spec) {
+      return [];
+    }
+    const pipelineSpec = spec.pipeline_spec ?? spec;
+
+    const {
+      components,
+      deploymentSpec: { executors },
+      root: {
+        dag: { tasks },
+      },
+    } = pipelineSpec;
+
+    const componentArtifactMap = parseComponentsForArtifactRelationship(components);
+    const taskArtifactMap = parseTasksForArtifactRelationship(tasks);
+
+    const nodes = getNodesForTasks(
+      'root',
+      spec,
+      tasks,
+      components,
+      executors,
       componentArtifactMap,
       taskArtifactMap,
-    );
-    if (artifactNodes.length) {
-      nodes.push(...artifactNodes);
-    }
-
-    if (taskValue.dependentTasks) {
-      // This task's runAfters may need artifact relationships -- find those artifactIds
-      runAfter.push(
-        ...taskValue.dependentTasks
-          .map((dependantTaskId) => {
-            const art = taskArtifactMap[dependantTaskId];
-            return art ? art.map((v) => v.artifactId) : null;
-          })
-          .filter((v): v is string[] => !!v)
-          .flat(),
-      );
-    }
-
-    const pipelineTask: PipelineTask = {
-      type: isGroupNode ? 'groupTask' : 'task',
-      name: taskName,
-      steps: executor ? [executor.container] : undefined,
-      inputs: parseInputOutput(component?.inputDefinitions),
-      outputs: parseInputOutput(component?.outputDefinitions),
-      status,
-      volumeMounts: parseVolumeMounts(spec.platform_spec, executorLabel),
-    };
-
-    // This task's rendering information
-    if (isGroupNode && groupTasks) {
-      const [nestedNodes, children] = getNestedNodes(
-        spec,
-        groupTasks,
-        components,
-        executors,
-        componentArtifactMap,
-        taskArtifactMap,
-        runDetails,
-        executions,
-      );
-      const newChildren = children.filter((child) => !nodes.find((n) => n.id === child));
-      const newNodes = nestedNodes.filter((node) => !nodes.find((n) => n.id === node.id));
-      const itemNode = createGroupNode(
-        taskId,
-        taskName,
-        pipelineTask,
-        runAfter,
-        translateStatusForNode(status?.state),
-        newChildren,
-      );
-      nodes.push(itemNode, ...newNodes);
-    } else {
-      nodes.push(
-        createNode(taskId, taskName, pipelineTask, runAfter, translateStatusForNode(status?.state)),
-      );
-    }
-
-    return [...acc, ...nodes];
-  }, []);
-};
+      runDetails,
+      executions,
+    )[0];
+
+    // Since we have artifacts that are input only that do not get created, remove any dependencies on them
+    return nodes.map((n) => ({
+      ...n,
+      runAfterTasks: n.runAfterTasks?.filter((t) => nodes.find((nextNode) => nextNode.id === t)),
+    }));
+  }, [executions, runDetails, spec]);

frontend/src/concepts/topology/PipelineDefaultTaskGroup.tsx

@@ -65,6 +65,7 @@ const DefaultTaskGroupInner: React.FunctionComponent<PipelinesDefaultGroupInnerP
         hideDetailsAtMedium
         showStatusState
         scaleNode={hover && detailsLevel !== ScaleDetailsLevel.high}
+        showLabel={detailsLevel === ScaleDetailsLevel.high}
         status={element.getData()?.runStatus}
         hiddenDetailsShownStatuses={[
           RunStatus.Succeeded,

frontend/src/concepts/topology/PipelineVisualizationSurface.tsx

@@ -9,7 +9,7 @@ import {
   TopologyView,
   useVisualizationController,
   VisualizationSurface,
-  getSpacerNodes,
+  addSpacerNodes,
 } from '@patternfly/react-topology';
 import {
   EmptyState,
@@ -40,23 +40,7 @@ const PipelineVisualizationSurface: React.FC<PipelineVisualizationSurfaceProps>
       return node;
     });
 
-    const spacerNodes = getSpacerNodes(updateNodes);
-
-    // find the parent of each spacer node
-    spacerNodes.forEach((spacerNode) => {
-      const nodeIds = spacerNode.id.split('|');
-      if (nodeIds[0]) {
-        const parent = updateNodes.find((n) => n.children?.includes(nodeIds[0]));
-        if (parent) {
-          parent.children?.push(spacerNode.id);
-        }
-      }
-    });
-
-    // Dagre likes the root nodes to be first in the order
-    const renderNodes = [...spacerNodes, ...updateNodes].sort(
-      (a, b) => (a.runAfterTasks?.length ?? 0) - (b.runAfterTasks?.length ?? 0),
-    );
+    const renderNodes = addSpacerNodes(updateNodes);
 
     // TODO: We can have a weird edge issue if the node is off by a few pixels vertically from the center
     const edges = getEdgesFromNodes(renderNodes);