Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add eventVersion check for map tasks #484

Merged
merged 4 commits into from
May 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { keyBy } from 'lodash';
import { TaskExecutionPhase } from 'models/Execution/enums';
import { ExternalResource, LogsByPhase, NodeExecution } from 'models/Execution/types';
import { endNodeId, startNodeId } from 'models/Node/constants';
import { isMapTaskV1 } from 'models/Task/utils';
import { Workflow, WorkflowId } from 'models/Workflow/types';
import * as React from 'react';
import { useEffect, useMemo, useState } from 'react';
Expand All @@ -32,6 +33,16 @@ export const ExecutionWorkflowGraph: React.FC<ExecutionWorkflowGraphProps> = ({
const taskExecutions = useTaskExecutions(nodeExecution.id);
useTaskExecutionsRefresher(nodeExecution, taskExecutions);

const useNewMapTaskView = taskExecutions.value.every((taskExecution) => {
const {
closure: { taskType, metadata, eventVersion = 0 },
} = taskExecution;
return isMapTaskV1(
eventVersion,
metadata?.externalResources?.length ?? 0,
taskType ?? undefined,
);
});
const externalResources: ExternalResource[] = taskExecutions.value
.map((taskExecution) => taskExecution.closure.metadata?.externalResources)
.flat()
Expand All @@ -41,7 +52,7 @@ export const ExecutionWorkflowGraph: React.FC<ExecutionWorkflowGraphProps> = ({

return {
...nodeExecution,
...(logsByPhase.size > 0 && { logsByPhase }),
...(useNewMapTaskView && logsByPhase.size > 0 && { logsByPhase }),
};
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { noExecutionsFoundString } from 'common/constants';
import { NonIdealState } from 'components/common/NonIdealState';
import { WaitForData } from 'components/common/WaitForData';
import { MapTaskExecution, NodeExecution, TaskExecution } from 'models/Execution/types';
import { isMapTaskType } from 'models/Task/utils';
import { isMapTaskV1 } from 'models/Task/utils';
import { TaskExecutionPhase } from 'models/Execution/enums';
import { useTaskExecutions, useTaskExecutionsRefresher } from '../useTaskExecutions';
import { MapTaskExecutionsListItem } from './MapTaskExecutionListItem';
Expand Down Expand Up @@ -42,9 +42,14 @@ export const TaskExecutionsListContent: React.FC<{
return (
<>
{taskExecutions.map((taskExecution) => {
const taskType = taskExecution.closure.taskType ?? undefined;
const useNewMapTaskView =
isMapTaskType(taskType) && taskExecution.closure.metadata?.externalResources;
const {
closure: { taskType, metadata, eventVersion = 0 },
} = taskExecution;
const useNewMapTaskView = isMapTaskV1(
eventVersion,
metadata?.externalResources?.length ?? 0,
taskType ?? undefined,
);
return useNewMapTaskView ? (
<MapTaskExecutionsListItem
key={getUniqueTaskExecutionName(taskExecution)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ describe('WorkflowGraph', () => {
onPhaseSelectionChanged={jest.fn}
workflow={workflow}
nodeExecutionsById={nodeExecutionsById}
isDetailsTabClosed={true}
/>
</QueryClientProvider>,
);
Expand Down
29 changes: 28 additions & 1 deletion packages/zapp/console/src/models/Task/task.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { TaskType } from './constants';
import { isMapTaskType } from './utils';
import { isMapTaskType, isMapTaskV1 } from './utils';

describe('models/Task', () => {
it('isMapTaskType propely identifies mapped tasks', async () => {
Expand All @@ -14,4 +14,31 @@ describe('models/Task', () => {
// when regular task is provided - to be false
expect(isMapTaskType(TaskType.PYTHON)).toBeFalsy();
});

it('isMapTaskV1 propely identifies mapped task events above version 1', () => {
const eventVersionZero = 0;
const eventVersionN = 3;
const resourcesLengthZero = 0;
const resourcesLengthN = 5;
const mapTaskTypes = [TaskType.ARRAY, TaskType.ARRAY_AWS, TaskType.ARRAY_K8S];
const nonMapTaskType = TaskType.PYTHON;

describe('FALSE cases:', () => {
// when no task is provided
expect(isMapTaskV1(eventVersionN, resourcesLengthN)).toBeFalsy();
// when regular task is provided
expect(isMapTaskV1(eventVersionN, resourcesLengthN, nonMapTaskType)).toBeFalsy();
// when eventVersion < 1
expect(isMapTaskV1(eventVersionZero, resourcesLengthN, mapTaskTypes[0])).toBeFalsy();
// when externalResources array has length of 0
expect(isMapTaskV1(eventVersionN, resourcesLengthZero, mapTaskTypes[0])).toBeFalsy();
});

describe('TRUE cases:', () => {
// when mapped task is provided AND eventVersion >= 1 AND externalResources array length > 0
for (const task of mapTaskTypes) {
expect(isMapTaskV1(eventVersionN, resourcesLengthN, task)).toBeTruthy();
}
});
});
});
9 changes: 9 additions & 0 deletions packages/zapp/console/src/models/Task/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,12 @@ export function isMapTaskType(taskType?: string): boolean {
taskType === TaskType.ARRAY_K8S
);
}

/** Returns true if tasks schema is treated as a map task AND eventVersion >= 1 AND externalResources array length > 0 */
export function isMapTaskV1(
eventVersion: number,
externalResourcesLength: number,
taskType?: string,
): boolean {
return isMapTaskType(taskType) && eventVersion >= 1 && externalResourcesLength > 0;
}