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

Add interruptible override to launch forms #chore #417

Merged
merged 9 commits into from
May 3, 2022
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
"@commitlint/cli": "^8.3.5",
"@commitlint/config-conventional": "^8.3.4",
"@date-io/moment": "1.3.9",
"@flyteorg/flyteidl": "0.24.11",
"@flyteorg/flyteidl": "1.1.0",
"@material-ui/core": "^4.0.0",
"@material-ui/icons": "^4.0.0",
"@material-ui/pickers": "^3.2.2",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { Execution } from 'models/Execution/types';
import * as React from 'react';
import { getLaunchPlan } from 'models/Launch/api';
import { LaunchPlanSpec } from 'models/Launch/types';
import { dashedValueString } from 'common/constants';
import { ExecutionMetadataLabels } from './constants';

const useStyles = makeStyles((theme: Theme) => {
Expand Down Expand Up @@ -39,6 +40,7 @@ export const ExecutionMetadataExtra: React.FC<{
maxParallelism,
rawOutputDataConfig,
securityContext,
interruptible,
} = execution.spec;

const [launchPlanSpec, setLaunchPlanSpec] = React.useState<Partial<LaunchPlanSpec>>({});
Expand Down Expand Up @@ -67,6 +69,10 @@ export const ExecutionMetadataExtra: React.FC<{
label: ExecutionMetadataLabels.parallelism,
value: maxParallelism,
},
{
label: ExecutionMetadataLabels.interruptible,
value: interruptible ? (interruptible.value ? 'true' : 'false') : dashedValueString,
},
];

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ function useRelaunchWorkflowFormState({ execution }: RelaunchExecutionFormProps)
annotations,
authRole,
securityContext,
interruptible,
},
} = execution;

Expand All @@ -59,6 +60,7 @@ function useRelaunchWorkflowFormState({ execution }: RelaunchExecutionFormProps)
annotations,
authRole,
securityContext,
interruptible,
};
},
},
Expand All @@ -74,7 +76,7 @@ function useRelaunchTaskFormState({ execution }: RelaunchExecutionFormProps) {
defaultValue: {} as TaskInitialLaunchParameters,
doFetch: async (execution) => {
const {
spec: { authRole, launchPlan: taskId },
spec: { authRole, launchPlan: taskId, interruptible },
} = execution;
const task = await apiContext.getTask(taskId);
const inputDefinitions = getTaskInputs(task);
Expand All @@ -85,7 +87,7 @@ function useRelaunchTaskFormState({ execution }: RelaunchExecutionFormProps) {
},
apiContext,
);
return { authRole, values, taskId };
return { authRole, values, taskId, interruptible };
},
},
execution,
Expand Down
1 change: 1 addition & 0 deletions src/components/Executions/ExecutionDetails/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export enum ExecutionMetadataLabels {
rawOutputPrefix = 'Raw Output Prefix',
parallelism = 'Parallelism',
securityContextDefault = 'default',
interruptible = 'Interruptible override',
}

export const tabs = {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { render } from '@testing-library/react';
import { dashedValueString } from 'common/constants';
import { Protobuf } from 'flyteidl';
import { Execution, WorkflowExecutionIdentifier } from 'models/Execution/types';
import { createMockExecution } from 'models/__mocks__/executionsData';
import * as React from 'react';
Expand All @@ -11,6 +12,7 @@ import { ExecutionMetadata } from '../ExecutionMetadata';
const clusterTestId = `metadata-${ExecutionMetadataLabels.cluster}`;
const startTimeTestId = `metadata-${ExecutionMetadataLabels.time}`;
const durationTestId = `metadata-${ExecutionMetadataLabels.duration}`;
const interruptibleTestId = `metadata-${ExecutionMetadataLabels.interruptible}`;

jest.mock('models/Launch/api', () => ({
getLaunchPlan: jest.fn(() => Promise.resolve({ spec: {} })),
Expand Down Expand Up @@ -75,4 +77,22 @@ describe('ExecutionMetadata', () => {
Routes.ExecutionDetails.makeUrl(referenceExecution),
);
});

it('shows true if execution was marked as interruptible', () => {
execution.spec.interruptible = Protobuf.BoolValue.create({ value: true });
const { getByTestId } = renderMetadata();
expect(getByTestId(interruptibleTestId)).toHaveTextContent('true');
});

it('shows false if execution was not marked as interruptible', () => {
execution.spec.interruptible = Protobuf.BoolValue.create({ value: false });
const { getByTestId } = renderMetadata();
expect(getByTestId(interruptibleTestId)).toHaveTextContent('false');
});

it('shows dashes if no interruptible value is found in execution spec', () => {
delete execution.spec.interruptible;
const { getByTestId } = renderMetadata();
expect(getByTestId(interruptibleTestId)).toHaveTextContent(dashedValueString);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
} from 'components/Launch/LaunchForm/utils';
import { mockSimpleVariables } from 'components/Launch/LaunchForm/__mocks__/mockInputs';
import { primitiveLiteral } from 'components/Launch/LaunchForm/__mocks__/utils';
import { Admin } from 'flyteidl';
import { Admin, Protobuf } from 'flyteidl';
import { LiteralMap, ResourceType, Variable } from 'models/Common/types';
import { getExecutionData } from 'models/Execution/api';
import { Execution, ExecutionData } from 'models/Execution/types';
Expand Down Expand Up @@ -155,6 +155,39 @@ describe('RelaunchExecutionForm', () => {
}),
});
});

it('should not set interruptible value if not provided', async () => {
delete execution.spec.interruptible;
const { getByText } = renderForm();
await waitFor(() => expect(getByText(mockContentString)));
checkLaunchFormProps({
initialParameters: expect.objectContaining({
interruptible: undefined,
}),
});
});

it('should have correct interruptible value if override is enabled', async () => {
execution.spec.interruptible = Protobuf.BoolValue.create({ value: true });
const { getByText } = renderForm();
await waitFor(() => expect(getByText(mockContentString)));
checkLaunchFormProps({
initialParameters: expect.objectContaining({
interruptible: Protobuf.BoolValue.create({ value: true }),
}),
});
});

it('should have correct interruptible value if override is disabled', async () => {
execution.spec.interruptible = Protobuf.BoolValue.create({ value: false });
const { getByText } = renderForm();
await waitFor(() => expect(getByText(mockContentString)));
checkLaunchFormProps({
initialParameters: expect.objectContaining({
interruptible: Protobuf.BoolValue.create({ value: false }),
}),
});
});
});

describe('Launch form with full inputs', () => {
Expand Down Expand Up @@ -256,5 +289,38 @@ describe('RelaunchExecutionForm', () => {
}),
});
});

it('should not set interruptible value if not provided', async () => {
delete execution.spec.interruptible;
const { getByText } = renderForm();
await waitFor(() => expect(getByText(mockContentString)));
checkLaunchFormProps({
initialParameters: expect.objectContaining({
interruptible: undefined,
}),
});
});

it('should have correct interruptible value if override is enabled', async () => {
execution.spec.interruptible = Protobuf.BoolValue.create({ value: true });
const { getByText } = renderForm();
await waitFor(() => expect(getByText(mockContentString)));
checkLaunchFormProps({
initialParameters: expect.objectContaining({
interruptible: Protobuf.BoolValue.create({ value: true }),
}),
});
});

it('should have correct interruptible value if override is disabled', async () => {
execution.spec.interruptible = Protobuf.BoolValue.create({ value: false });
const { getByText } = renderForm();
await waitFor(() => expect(getByText(mockContentString)));
checkLaunchFormProps({
initialParameters: expect.objectContaining({
interruptible: Protobuf.BoolValue.create({ value: false }),
}),
});
});
});
});
112 changes: 112 additions & 0 deletions src/components/Launch/LaunchForm/LaunchInterruptibleInput.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
import { makeStyles, Theme, Typography } from '@material-ui/core';
import FormControlLabel from '@material-ui/core/FormControlLabel';
import Checkbox from '@material-ui/core/Checkbox';
import * as React from 'react';
import { Protobuf } from 'flyteidl';
import { useStyles } from './styles';
import { LaunchInterruptibleInputRef } from './types';
import { formStrings } from './constants';

export const useInterruptibleStyles = makeStyles((theme: Theme) => ({
labelIndeterminate: {
color: theme.palette.grey[500],
},
}));

const isValueValid = (value: any) => {
return value !== undefined && value !== null;
};

interface LaunchInterruptibleInputProps {
initialValue?: Protobuf.IBoolValue | null;
}

export const LaunchInterruptibleInputImpl: React.ForwardRefRenderFunction<
LaunchInterruptibleInputRef,
LaunchInterruptibleInputProps
> = (props, ref) => {
// interruptible stores the override to enable/disable the setting for an execution
const [interruptible, setInterruptible] = React.useState(false);
// indeterminate tracks whether the interruptible flag is unspecified/indeterminate (true) or an override has been selected (false)
const [indeterminate, setIndeterminate] = React.useState(true);

React.useEffect(() => {
if (isValueValid(props.initialValue) && isValueValid(props.initialValue!.value)) {
setInterruptible(() => props.initialValue!.value!);
setIndeterminate(() => false);
} else {
setInterruptible(() => false);
setIndeterminate(() => true);
}
}, [props.initialValue?.value]);

const handleInputChange = React.useCallback(() => {
if (indeterminate) {
setInterruptible(() => true);
setIndeterminate(() => false);
} else if (interruptible) {
setInterruptible(() => false);
setIndeterminate(() => false);
} else {
setInterruptible(() => false);
setIndeterminate(() => true);
}
}, [interruptible, indeterminate]);

React.useImperativeHandle(
ref,
() => ({
getValue: () => {
if (indeterminate) {
return null;
}

return Protobuf.BoolValue.create({ value: interruptible });
},
validate: () => true,
}),
[interruptible, indeterminate],
);

const styles = useStyles();
const interruptibleStyles = useInterruptibleStyles();

const getInterruptibleLabel = () => {
if (indeterminate) {
return (
<Typography
className={interruptibleStyles.labelIndeterminate}
>{`${formStrings.interruptible} (no override)`}</Typography>
);
} else if (interruptible) {
return <Typography>{`${formStrings.interruptible} (enabled)`}</Typography>;
}
return <Typography>{`${formStrings.interruptible} (disabled)`}</Typography>;
};

return (
<section>
<header className={styles.sectionHeader}>
<Typography variant="h6">Override interruptible flag</Typography>
<Typography variant="body2">
Overrides the interruptible flag of a workflow for a single execution, allowing it to be
forced on or off. If no value was selected, the workflow's default will be used.
</Typography>
</header>
<section title={formStrings.interruptible}>
<FormControlLabel
control={
<Checkbox
checked={interruptible}
indeterminate={indeterminate}
onChange={handleInputChange}
/>
}
label={getInterruptibleLabel()}
/>
</section>
</section>
);
};

export const LaunchInterruptibleInput = React.forwardRef(LaunchInterruptibleInputImpl);
15 changes: 13 additions & 2 deletions src/components/Launch/LaunchForm/LaunchTaskForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { LaunchFormHeader } from './LaunchFormHeader';
import { LaunchFormInputs } from './LaunchFormInputs';
import { LaunchState } from './launchMachine';
import { LaunchRoleInput } from './LaunchRoleInput';
import { LaunchInterruptibleInput } from './LaunchInterruptibleInput';
import { SearchableSelector } from './SearchableSelector';
import { useStyles } from './styles';
import { BaseInterpretedLaunchState, BaseLaunchService, LaunchTaskFormProps } from './types';
Expand All @@ -15,8 +16,14 @@ import { isEnterInputsState } from './utils';

/** Renders the form for initiating a Launch request based on a Task */
export const LaunchTaskForm: React.FC<LaunchTaskFormProps> = (props) => {
const { formInputsRef, roleInputRef, state, service, taskSourceSelectorState } =
useLaunchTaskFormState(props);
const {
formInputsRef,
roleInputRef,
interruptibleInputRef,
state,
service,
taskSourceSelectorState,
} = useLaunchTaskFormState(props);
const styles = useStyles();
const baseState = state as BaseInterpretedLaunchState;
const baseService = service as BaseLaunchService;
Expand Down Expand Up @@ -63,6 +70,10 @@ export const LaunchTaskForm: React.FC<LaunchTaskFormProps> = (props) => {
/>
) : null}
<LaunchFormInputs key={formKey} ref={formInputsRef} state={baseState} variant="task" />
<LaunchInterruptibleInput
initialValue={state.context.interruptible}
ref={interruptibleInputRef}
/>
</DialogContent>
<LaunchFormActions state={baseState} service={baseService} onClose={props.onClose} />
</>
Expand Down
6 changes: 6 additions & 0 deletions src/components/Launch/LaunchForm/LaunchWorkflowForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@ import { useLaunchWorkflowFormState } from './useLaunchWorkflowFormState';
import { isEnterInputsState } from './utils';
import { LaunchRoleInput } from './LaunchRoleInput';
import { LaunchFormAdvancedInputs } from './LaunchFormAdvancedInputs';
import { LaunchInterruptibleInput } from './LaunchInterruptibleInput';

/** Renders the form for initiating a Launch request based on a Workflow */
export const LaunchWorkflowForm: React.FC<LaunchWorkflowFormProps> = (props) => {
const {
formInputsRef,
roleInputRef,
advancedOptionsRef,
interruptibleInputRef,
state,
service,
workflowSourceSelectorState,
Expand Down Expand Up @@ -105,6 +107,10 @@ export const LaunchWorkflowForm: React.FC<LaunchWorkflowFormProps> = (props) =>
/>
) : null}
<LaunchFormAdvancedInputs ref={advancedOptionsRef} state={state} />
<LaunchInterruptibleInput
initialValue={state.context.interruptible}
ref={interruptibleInputRef}
/>
</AccordionDetails>
</Accordion>
</DialogContent>
Expand Down
1 change: 1 addition & 0 deletions src/components/Launch/LaunchForm/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export const formStrings = {
title: 'Create New Execution',
workflowVersion: 'Workflow Version',
launchPlan: 'Launch Plan',
interruptible: 'Interruptible',
};

export const AuthRoleStrings: { [k in AuthRoleTypes]: AuthRoleMeta } = {
Expand Down
Loading