Skip to content

Commit fbda985

Browse files
MasonMMasonM
authored
fix: gracefully handle invalid CronWorkflows and simplify logic. (cherry-pick #14197) (#14419)
Signed-off-by: Mason Malone <[email protected]>
1 parent fbea019 commit fbda985

File tree

8 files changed

+153
-59
lines changed

8 files changed

+153
-59
lines changed

‎ui/src/cron-workflows/cron-workflow-editor.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import {WorkflowParametersEditor} from '../shared/components/editors/workflow-pa
77
import {ObjectEditor} from '../shared/components/object-editor';
88
import type {Lang} from '../shared/components/object-parser';
99
import {CronWorkflow} from '../shared/models';
10-
import {CronWorkflowSpecEditor} from './cron-workflow-spec-editior';
10+
import {CronWorkflowSpecEditor} from './cron-workflow-spec-editor';
1111
import {CronWorkflowStatusViewer} from './cron-workflow-status-viewer';
1212

1313
export function CronWorkflowEditor({

‎ui/src/cron-workflows/cron-workflow-list.tsx

Lines changed: 10 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -169,28 +169,20 @@ export function CronWorkflowList({match, location, history}: RouteComponentProps
169169
<div className='columns small-1'>{w.metadata.namespace}</div>
170170
<div className='columns small-1'>{w.spec.timezone}</div>
171171
<div className='columns small-1'>
172-
{w.spec.schedule
173-
? w.spec.schedule
174-
: w.spec.schedules.map(schedule => (
175-
<>
176-
{schedule}
177-
<br />
178-
</>
179-
))}
172+
{w.spec.schedules.map(schedule => (
173+
<>
174+
{schedule}
175+
<br />
176+
</>
177+
))}
180178
</div>
181179
<div className='columns small-2'>
182-
{w.spec.schedule ? (
183-
<PrettySchedule schedule={w.spec.schedule} />
184-
) : (
180+
{w.spec.schedules.map(schedule => (
185181
<>
186-
{w.spec.schedules.map(schedule => (
187-
<>
188-
<PrettySchedule schedule={schedule} />
189-
<br />
190-
</>
191-
))}
182+
<PrettySchedule schedule={schedule} />
183+
<br />
192184
</>
193-
)}
185+
))}
194186
</div>
195187
<div className='columns small-2'>
196188
<Timestamp date={w.metadata.creationTimestamp} displayISOFormat={storedDisplayISOFormatCreation} />
@@ -223,10 +215,6 @@ export function CronWorkflowList({match, location, history}: RouteComponentProps
223215
}
224216

225217
function getSpecNextScheduledTime(spec: CronWorkflowSpec): Date {
226-
if (spec.schedule) {
227-
return getNextScheduledTime(spec.schedule, spec.timezone);
228-
}
229-
230218
let out: Date;
231219
spec.schedules.forEach(schedule => {
232220
const next = getNextScheduledTime(schedule, spec.timezone);

‎ui/src/cron-workflows/cron-workflow-spec-editior.tsx renamed to ‎ui/src/cron-workflows/cron-workflow-spec-editor.tsx

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -14,22 +14,12 @@ export function CronWorkflowSpecEditor({onChange, spec}: {spec: CronWorkflowSpec
1414
<div className='row white-box__details-row'>
1515
<div className='columns small-3'>Schedules</div>
1616
<div className='columns small-9'>
17-
{(spec.schedule ?? '') != '' ? (
17+
{spec.schedules.map((schedule, index) => (
1818
<>
19-
<TextInput value={spec.schedule} onChange={schedule => onChange({...spec, schedule})} />
20-
<ScheduleValidator schedule={spec.schedule} />
19+
<TextInput value={schedule} onChange={newSchedule => onChange({...spec, schedules: updateScheduleAtIndex(spec.schedules, index, newSchedule)})} />
20+
<ScheduleValidator schedule={schedule} />
2121
</>
22-
) : (
23-
spec.schedules.map((schedule, index) => (
24-
<>
25-
<TextInput
26-
value={schedule}
27-
onChange={newSchedule => onChange({...spec, schedules: updateScheduleAtIndex(spec.schedules, index, newSchedule)})}
28-
/>
29-
<ScheduleValidator schedule={schedule} />
30-
</>
31-
))
32-
)}
22+
))}
3323
</div>
3424
</div>
3525
<div className='row white-box__details-row'>

‎ui/src/cron-workflows/cron-workflow-status-viewer.tsx

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,22 +19,12 @@ export function CronWorkflowStatusViewer({spec, status}: {spec: CronWorkflowSpec
1919
{title: 'Active', value: status.active ? getCronWorkflowActiveWorkflowList(status.active) : <i>No Workflows Active</i>},
2020
{
2121
title: 'Schedules',
22-
value: (
22+
value: spec.schedules.map(schedule => (
2323
<>
24-
{(spec.schedule ?? '') != '' ? (
25-
<>
26-
<code>{spec.schedule}</code> <PrettySchedule schedule={spec.schedule} />
27-
</>
28-
) : (
29-
spec.schedules.map(schedule => (
30-
<>
31-
<code>{schedule}</code> <PrettySchedule schedule={schedule} />
32-
<br />
33-
</>
34-
))
35-
)}
24+
<code>{schedule}</code> <PrettySchedule schedule={schedule} />
25+
<br />
3626
</>
37-
)
27+
))
3828
},
3929
{title: 'Last Scheduled Time', value: <Timestamp date={status.lastScheduledTime} timestampKey={TIMESTAMP_KEYS.CRON_WORKFLOW_STATUS_LAST_SCHEDULED} />},
4030
{title: 'Conditions', value: <ConditionsPanel conditions={status.conditions} />}

‎ui/src/shared/examples.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ export const exampleCronWorkflow = (namespace: string): CronWorkflow => ({
8484
},
8585
spec: {
8686
workflowMetadata: {labels},
87-
schedule: '* * * * *',
87+
schedules: ['* * * * *'],
8888
workflowSpec: {
8989
entrypoint,
9090
arguments: argumentz,

‎ui/src/shared/models/cron-workflows.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ export type ConcurrencyPolicy = 'Allow' | 'Forbid' | 'Replace';
1515
export interface CronWorkflowSpec {
1616
workflowSpec: WorkflowSpec;
1717
workflowMetadata?: kubernetes.ObjectMeta;
18-
schedule: string;
1918
schedules?: string[];
2019
concurrencyPolicy?: ConcurrencyPolicy;
2120
suspend?: boolean;

‎ui/src/shared/services/cron-workflow-service.test.ts

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
import {exampleCronWorkflow} from '../examples';
2+
import {CronWorkflowService} from './cron-workflow-service';
3+
import requests from './requests';
4+
5+
jest.mock('./requests');
6+
7+
describe('cron workflow service', () => {
8+
describe('create', () => {
9+
test('with valid CronWorkflow', async () => {
10+
const cronWf = exampleCronWorkflow('ns');
11+
const request = {send: jest.fn().mockResolvedValue({body: cronWf})};
12+
jest.spyOn(requests, 'post').mockReturnValue(request as any);
13+
14+
const result = await CronWorkflowService.create(cronWf, cronWf.metadata.namespace);
15+
16+
expect(result).toStrictEqual(cronWf);
17+
expect(requests.post).toHaveBeenCalledWith('api/v1/cron-workflows/ns');
18+
});
19+
});
20+
21+
describe('list', () => {
22+
test('with no results', async () => {
23+
jest.spyOn(requests, 'get').mockResolvedValue({body: {}} as any);
24+
25+
const result = await CronWorkflowService.list('ns');
26+
27+
expect(result).toStrictEqual([]);
28+
expect(requests.get).toHaveBeenCalledWith('api/v1/cron-workflows/ns?');
29+
});
30+
31+
test('with multiple results', async () => {
32+
const items = [exampleCronWorkflow('ns'), exampleCronWorkflow('ns')];
33+
jest.spyOn(requests, 'get').mockResolvedValue({body: {items}} as any);
34+
35+
const result = await CronWorkflowService.list('ns', ['foo', 'bar']);
36+
37+
expect(result).toStrictEqual(items);
38+
expect(requests.get).toHaveBeenCalledWith('api/v1/cron-workflows/ns?listOptions.labelSelector=foo,bar');
39+
});
40+
});
41+
42+
describe('get', () => {
43+
test('with valid CronWorkflow', async () => {
44+
const cronWf = exampleCronWorkflow('ns');
45+
jest.spyOn(requests, 'get').mockResolvedValue({body: cronWf} as any);
46+
47+
const result = await CronWorkflowService.get(cronWf.metadata.name, 'ns');
48+
49+
expect(result).toStrictEqual(cronWf);
50+
expect(requests.get).toHaveBeenCalledWith(`api/v1/cron-workflows/ns/${cronWf.metadata.name}`);
51+
});
52+
53+
test('with old CronWorkflow using "schedule"', async () => {
54+
const cronWf = exampleCronWorkflow('otherns') as any;
55+
cronWf.spec.schedule = '* * * * *';
56+
delete cronWf.spec.schedules;
57+
jest.spyOn(requests, 'get').mockResolvedValue({body: cronWf} as any);
58+
59+
const result = await CronWorkflowService.get(cronWf.metadata.name, 'otherns');
60+
61+
expect(result.spec.schedules).toEqual(['* * * * *']);
62+
expect(requests.get).toHaveBeenCalledWith(`api/v1/cron-workflows/otherns/${cronWf.metadata.name}`);
63+
});
64+
65+
test('with invalid CronWorkflow missing "schedules"', async () => {
66+
const cronWf = exampleCronWorkflow('otherns');
67+
delete cronWf.spec.schedules;
68+
jest.spyOn(requests, 'get').mockResolvedValue({body: cronWf} as any);
69+
70+
const result = await CronWorkflowService.get(cronWf.metadata.name, 'otherns');
71+
72+
expect(result.spec.schedules).toEqual([]);
73+
expect(requests.get).toHaveBeenCalledWith(`api/v1/cron-workflows/otherns/${cronWf.metadata.name}`);
74+
});
75+
});
76+
77+
describe('update', () => {
78+
test('with valid CronWorkflow', async () => {
79+
const cronWf = exampleCronWorkflow('ns');
80+
const request = {send: jest.fn().mockResolvedValue({body: cronWf})};
81+
jest.spyOn(requests, 'put').mockReturnValue(request as any);
82+
83+
const result = await CronWorkflowService.update(cronWf, cronWf.metadata.name, cronWf.metadata.namespace);
84+
85+
expect(result).toStrictEqual(cronWf);
86+
expect(requests.put).toHaveBeenCalledWith(`api/v1/cron-workflows/ns/${cronWf.metadata.name}`);
87+
});
88+
});
89+
90+
describe('suspend', () => {
91+
test('with valid CronWorkflow', async () => {
92+
const cronWf = exampleCronWorkflow('ns');
93+
jest.spyOn(requests, 'put').mockResolvedValue({body: cronWf} as any);
94+
95+
const result = await CronWorkflowService.suspend(cronWf.metadata.name, 'ns');
96+
97+
expect(result).toStrictEqual(cronWf);
98+
expect(requests.put).toHaveBeenCalledWith(`api/v1/cron-workflows/ns/${cronWf.metadata.name}/suspend`);
99+
});
100+
});
101+
102+
describe('resume', () => {
103+
test('with valid CronWorkflow', async () => {
104+
const cronWf = exampleCronWorkflow('ns');
105+
jest.spyOn(requests, 'put').mockResolvedValue({body: cronWf} as any);
106+
107+
const result = await CronWorkflowService.resume(cronWf.metadata.name, 'ns');
108+
109+
expect(result).toStrictEqual(cronWf);
110+
expect(requests.put).toHaveBeenCalledWith(`api/v1/cron-workflows/ns/${cronWf.metadata.name}/resume`);
111+
});
112+
});
113+
});

‎ui/src/shared/services/cron-workflow-service.ts

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,41 +2,55 @@ import {CronWorkflow, CronWorkflowList} from '../models';
22
import requests from './requests';
33
import {queryParams} from './utils';
44

5+
// Handle CronWorkflows using the deprecated "schedule" field by automatically
6+
// migrating them to use "schedules".
7+
// Also, gracefully handle invalid CronWorkflows that are missing both
8+
// "schedule" and "schedules".
9+
function normalizeSchedules(cronWorkflow: any): CronWorkflow {
10+
cronWorkflow.spec.schedules ??= [];
11+
// TODO: Delete this once we drop support for "schedule"
12+
if ((cronWorkflow.spec.schedule ?? '') != '') {
13+
cronWorkflow.spec.schedules.push(cronWorkflow.spec.schedule);
14+
delete cronWorkflow.spec.schedule;
15+
}
16+
return cronWorkflow as CronWorkflow;
17+
}
18+
519
export const CronWorkflowService = {
620
create(cronWorkflow: CronWorkflow, namespace: string) {
721
return requests
822
.post(`api/v1/cron-workflows/${namespace}`)
923
.send({cronWorkflow})
10-
.then(res => res.body as CronWorkflow);
24+
.then(res => normalizeSchedules(res.body));
1125
},
1226

1327
list(namespace: string, labels: string[] = []) {
1428
return requests
1529
.get(`api/v1/cron-workflows/${namespace}?${queryParams({labels}).join('&')}`)
1630
.then(res => res.body as CronWorkflowList)
17-
.then(list => list.items || []);
31+
.then(list => (list.items || []).map(normalizeSchedules));
1832
},
1933

2034
get(name: string, namespace: string) {
21-
return requests.get(`api/v1/cron-workflows/${namespace}/${name}`).then(res => res.body as CronWorkflow);
35+
return requests.get(`api/v1/cron-workflows/${namespace}/${name}`).then(res => normalizeSchedules(res.body));
2236
},
2337

2438
update(cronWorkflow: CronWorkflow, name: string, namespace: string) {
2539
return requests
2640
.put(`api/v1/cron-workflows/${namespace}/${name}`)
2741
.send({cronWorkflow})
28-
.then(res => res.body as CronWorkflow);
42+
.then(res => normalizeSchedules(res.body));
2943
},
3044

3145
delete(name: string, namespace: string) {
3246
return requests.delete(`api/v1/cron-workflows/${namespace}/${name}`);
3347
},
3448

3549
suspend(name: string, namespace: string) {
36-
return requests.put(`api/v1/cron-workflows/${namespace}/${name}/suspend`).then(res => res.body as CronWorkflow);
50+
return requests.put(`api/v1/cron-workflows/${namespace}/${name}/suspend`).then(res => normalizeSchedules(res.body));
3751
},
3852

3953
resume(name: string, namespace: string) {
40-
return requests.put(`api/v1/cron-workflows/${namespace}/${name}/resume`).then(res => res.body as CronWorkflow);
54+
return requests.put(`api/v1/cron-workflows/${namespace}/${name}/resume`).then(res => normalizeSchedules(res.body));
4155
}
4256
};

0 commit comments

Comments
 (0)