fix: gracefully handle invalid CronWorkflows and simplify logic. (cherry-pick #14197)

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

@@ -7,7 +7,7 @@ import {WorkflowParametersEditor} from '../shared/components/editors/workflow-pa
 import {ObjectEditor} from '../shared/components/object-editor';
 import type {Lang} from '../shared/components/object-parser';
 import {CronWorkflow} from '../shared/models';
-import {CronWorkflowSpecEditor} from './cron-workflow-spec-editior';
+import {CronWorkflowSpecEditor} from './cron-workflow-spec-editor';
 import {CronWorkflowStatusViewer} from './cron-workflow-status-viewer';
 
 export function CronWorkflowEditor({

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

@@ -169,28 +169,20 @@ export function CronWorkflowList({match, location, history}: RouteComponentProps
                                         <div className='columns small-1'>{w.metadata.namespace}</div>
                                         <div className='columns small-1'>{w.spec.timezone}</div>
                                         <div className='columns small-1'>
-                                            {w.spec.schedule
-                                                ? w.spec.schedule
-                                                : w.spec.schedules.map(schedule => (
-                                                      <>
-                                                          {schedule}
-                                                          <br />
-                                                      </>
-                                                  ))}
+                                            {w.spec.schedules.map(schedule => (
+                                                <>
+                                                    {schedule}
+                                                    <br />
+                                                </>
+                                            ))}
                                         </div>
                                         <div className='columns small-2'>
-                                            {w.spec.schedule ? (
-                                                <PrettySchedule schedule={w.spec.schedule} />
-                                            ) : (
+                                            {w.spec.schedules.map(schedule => (
                                                 <>
-                                                    {w.spec.schedules.map(schedule => (
-                                                        <>
-                                                            <PrettySchedule schedule={schedule} />
-                                                            <br />
-                                                        </>
-                                                    ))}
+                                                    <PrettySchedule schedule={schedule} />
+                                                    <br />
                                                 </>
-                                            )}
+                                            ))}
                                         </div>
                                         <div className='columns small-2'>
                                             <Timestamp date={w.metadata.creationTimestamp} displayISOFormat={storedDisplayISOFormatCreation} />
@@ -223,10 +215,6 @@ export function CronWorkflowList({match, location, history}: RouteComponentProps
 }
 
 function getSpecNextScheduledTime(spec: CronWorkflowSpec): Date {
-    if (spec.schedule) {
-        return getNextScheduledTime(spec.schedule, spec.timezone);
-    }
-
     let out: Date;
     spec.schedules.forEach(schedule => {
         const next = getNextScheduledTime(schedule, spec.timezone);

ui/src/cron-workflows/cron-workflow-spec-editior.tsx → ui/src/cron-workflows/cron-workflow-spec-editor.tsx

@@ -14,22 +14,12 @@ export function CronWorkflowSpecEditor({onChange, spec}: {spec: CronWorkflowSpec
                 <div className='row white-box__details-row'>
                     <div className='columns small-3'>Schedules</div>
                     <div className='columns small-9'>
-                        {(spec.schedule ?? '') != '' ? (
+                        {spec.schedules.map((schedule, index) => (
                             <>
-                                <TextInput value={spec.schedule} onChange={schedule => onChange({...spec, schedule})} />
-                                <ScheduleValidator schedule={spec.schedule} />
+                                <TextInput value={schedule} onChange={newSchedule => onChange({...spec, schedules: updateScheduleAtIndex(spec.schedules, index, newSchedule)})} />
+                                <ScheduleValidator schedule={schedule} />
                             </>
-                        ) : (
-                            spec.schedules.map((schedule, index) => (
-                                <>
-                                    <TextInput
-                                        value={schedule}
-                                        onChange={newSchedule => onChange({...spec, schedules: updateScheduleAtIndex(spec.schedules, index, newSchedule)})}
-                                    />
-                                    <ScheduleValidator schedule={schedule} />
-                                </>
-                            ))
-                        )}
+                        ))}
                     </div>
                 </div>
                 <div className='row white-box__details-row'>

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

@@ -19,22 +19,12 @@ export function CronWorkflowStatusViewer({spec, status}: {spec: CronWorkflowSpec
                     {title: 'Active', value: status.active ? getCronWorkflowActiveWorkflowList(status.active) : <i>No Workflows Active</i>},
                     {
                         title: 'Schedules',
-                        value: (
+                        value: spec.schedules.map(schedule => (
                             <>
-                                {(spec.schedule ?? '') != '' ? (
-                                    <>
-                                        <code>{spec.schedule}</code> <PrettySchedule schedule={spec.schedule} />
-                                    </>
-                                ) : (
-                                    spec.schedules.map(schedule => (
-                                        <>
-                                            <code>{schedule}</code> <PrettySchedule schedule={schedule} />
-                                            <br />
-                                        </>
-                                    ))
-                                )}
+                                <code>{schedule}</code> <PrettySchedule schedule={schedule} />
+                                <br />
                             </>
-                        )
+                        ))
                     },
                     {title: 'Last Scheduled Time', value: <Timestamp date={status.lastScheduledTime} timestampKey={TIMESTAMP_KEYS.CRON_WORKFLOW_STATUS_LAST_SCHEDULED} />},
                     {title: 'Conditions', value: <ConditionsPanel conditions={status.conditions} />}

ui/src/shared/examples.ts

@@ -84,7 +84,7 @@ export const exampleCronWorkflow = (namespace: string): CronWorkflow => ({
     },
     spec: {
         workflowMetadata: {labels},
-        schedule: '* * * * *',
+        schedules: ['* * * * *'],
         workflowSpec: {
             entrypoint,
             arguments: argumentz,

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

@@ -15,7 +15,6 @@ export type ConcurrencyPolicy = 'Allow' | 'Forbid' | 'Replace';
 export interface CronWorkflowSpec {
     workflowSpec: WorkflowSpec;
     workflowMetadata?: kubernetes.ObjectMeta;
-    schedule: string;
     schedules?: string[];
     concurrencyPolicy?: ConcurrencyPolicy;
     suspend?: boolean;

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

@@ -0,0 +1,113 @@
+import {exampleCronWorkflow} from '../examples';
+import {CronWorkflowService} from './cron-workflow-service';
+import requests from './requests';
+
+jest.mock('./requests');
+
+describe('cron workflow service', () => {
+    describe('create', () => {
+        test('with valid CronWorkflow', async () => {
+            const cronWf = exampleCronWorkflow('ns');
+            const request = {send: jest.fn().mockResolvedValue({body: cronWf})};
+            jest.spyOn(requests, 'post').mockReturnValue(request as any);
+
+            const result = await CronWorkflowService.create(cronWf, cronWf.metadata.namespace);
+
+            expect(result).toStrictEqual(cronWf);
+            expect(requests.post).toHaveBeenCalledWith('api/v1/cron-workflows/ns');
+        });
+    });
+
+    describe('list', () => {
+        test('with no results', async () => {
+            jest.spyOn(requests, 'get').mockResolvedValue({body: {}} as any);
+
+            const result = await CronWorkflowService.list('ns');
+
+            expect(result).toStrictEqual([]);
+            expect(requests.get).toHaveBeenCalledWith('api/v1/cron-workflows/ns?');
+        });
+
+        test('with multiple results', async () => {
+            const items = [exampleCronWorkflow('ns'), exampleCronWorkflow('ns')];
+            jest.spyOn(requests, 'get').mockResolvedValue({body: {items}} as any);
+
+            const result = await CronWorkflowService.list('ns', ['foo', 'bar']);
+
+            expect(result).toStrictEqual(items);
+            expect(requests.get).toHaveBeenCalledWith('api/v1/cron-workflows/ns?listOptions.labelSelector=foo,bar');
+        });
+    });
+
+    describe('get', () => {
+        test('with valid CronWorkflow', async () => {
+            const cronWf = exampleCronWorkflow('ns');
+            jest.spyOn(requests, 'get').mockResolvedValue({body: cronWf} as any);
+
+            const result = await CronWorkflowService.get(cronWf.metadata.name, 'ns');
+
+            expect(result).toStrictEqual(cronWf);
+            expect(requests.get).toHaveBeenCalledWith(`api/v1/cron-workflows/ns/${cronWf.metadata.name}`);
+        });
+
+        test('with old CronWorkflow using "schedule"', async () => {
+            const cronWf = exampleCronWorkflow('otherns') as any;
+            cronWf.spec.schedule = '* * * * *';
+            delete cronWf.spec.schedules;
+            jest.spyOn(requests, 'get').mockResolvedValue({body: cronWf} as any);
+
+            const result = await CronWorkflowService.get(cronWf.metadata.name, 'otherns');
+
+            expect(result.spec.schedules).toEqual(['* * * * *']);
+            expect(requests.get).toHaveBeenCalledWith(`api/v1/cron-workflows/otherns/${cronWf.metadata.name}`);
+        });
+
+        test('with invalid CronWorkflow missing "schedules"', async () => {
+            const cronWf = exampleCronWorkflow('otherns');
+            delete cronWf.spec.schedules;
+            jest.spyOn(requests, 'get').mockResolvedValue({body: cronWf} as any);
+
+            const result = await CronWorkflowService.get(cronWf.metadata.name, 'otherns');
+
+            expect(result.spec.schedules).toEqual([]);
+            expect(requests.get).toHaveBeenCalledWith(`api/v1/cron-workflows/otherns/${cronWf.metadata.name}`);
+        });
+    });
+
+    describe('update', () => {
+        test('with valid CronWorkflow', async () => {
+            const cronWf = exampleCronWorkflow('ns');
+            const request = {send: jest.fn().mockResolvedValue({body: cronWf})};
+            jest.spyOn(requests, 'put').mockReturnValue(request as any);
+
+            const result = await CronWorkflowService.update(cronWf, cronWf.metadata.name, cronWf.metadata.namespace);
+
+            expect(result).toStrictEqual(cronWf);
+            expect(requests.put).toHaveBeenCalledWith(`api/v1/cron-workflows/ns/${cronWf.metadata.name}`);
+        });
+    });
+
+    describe('suspend', () => {
+        test('with valid CronWorkflow', async () => {
+            const cronWf = exampleCronWorkflow('ns');
+            jest.spyOn(requests, 'put').mockResolvedValue({body: cronWf} as any);
+
+            const result = await CronWorkflowService.suspend(cronWf.metadata.name, 'ns');
+
+            expect(result).toStrictEqual(cronWf);
+            expect(requests.put).toHaveBeenCalledWith(`api/v1/cron-workflows/ns/${cronWf.metadata.name}/suspend`);
+        });
+    });
+
+    describe('resume', () => {
+        test('with valid CronWorkflow', async () => {
+            const cronWf = exampleCronWorkflow('ns');
+            jest.spyOn(requests, 'put').mockResolvedValue({body: cronWf} as any);
+
+            const result = await CronWorkflowService.resume(cronWf.metadata.name, 'ns');
+
+            expect(result).toStrictEqual(cronWf);
+            expect(requests.put).toHaveBeenCalledWith(`api/v1/cron-workflows/ns/${cronWf.metadata.name}/resume`);
+        });
+    });
+});

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

@@ -2,41 +2,55 @@ import {CronWorkflow, CronWorkflowList} from '../models';
 import requests from './requests';
 import {queryParams} from './utils';
 
+// Handle CronWorkflows using the deprecated "schedule" field by automatically
+// migrating them to use "schedules".
+// Also, gracefully handle invalid CronWorkflows that are missing both
+// "schedule" and "schedules".
+function normalizeSchedules(cronWorkflow: any): CronWorkflow {
+    cronWorkflow.spec.schedules ??= [];
+    // TODO: Delete this once we drop support for "schedule"
+    if ((cronWorkflow.spec.schedule ?? '') != '') {
+        cronWorkflow.spec.schedules.push(cronWorkflow.spec.schedule);
+        delete cronWorkflow.spec.schedule;
+    }
+    return cronWorkflow as CronWorkflow;
+}
+
 export const CronWorkflowService = {
     create(cronWorkflow: CronWorkflow, namespace: string) {
         return requests
             .post(`api/v1/cron-workflows/${namespace}`)
             .send({cronWorkflow})
-            .then(res => res.body as CronWorkflow);
+            .then(res => normalizeSchedules(res.body));
     },
 
     list(namespace: string, labels: string[] = []) {
         return requests
             .get(`api/v1/cron-workflows/${namespace}?${queryParams({labels}).join('&')}`)
             .then(res => res.body as CronWorkflowList)
-            .then(list => list.items || []);
+            .then(list => (list.items || []).map(normalizeSchedules));
     },
 
     get(name: string, namespace: string) {
-        return requests.get(`api/v1/cron-workflows/${namespace}/${name}`).then(res => res.body as CronWorkflow);
+        return requests.get(`api/v1/cron-workflows/${namespace}/${name}`).then(res => normalizeSchedules(res.body));
     },
 
     update(cronWorkflow: CronWorkflow, name: string, namespace: string) {
         return requests
             .put(`api/v1/cron-workflows/${namespace}/${name}`)
             .send({cronWorkflow})
-            .then(res => res.body as CronWorkflow);
+            .then(res => normalizeSchedules(res.body));
     },
 
     delete(name: string, namespace: string) {
         return requests.delete(`api/v1/cron-workflows/${namespace}/${name}`);
     },
 
     suspend(name: string, namespace: string) {
-        return requests.put(`api/v1/cron-workflows/${namespace}/${name}/suspend`).then(res => res.body as CronWorkflow);
+        return requests.put(`api/v1/cron-workflows/${namespace}/${name}/suspend`).then(res => normalizeSchedules(res.body));
     },
 
     resume(name: string, namespace: string) {
-        return requests.put(`api/v1/cron-workflows/${namespace}/${name}/resume`).then(res => res.body as CronWorkflow);
+        return requests.put(`api/v1/cron-workflows/${namespace}/${name}/resume`).then(res => normalizeSchedules(res.body));
     }
 };