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

feat: show history about completed runs in each cron workflow #11790

Closed
wants to merge 22 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
2a11703
docs: Add description about img in artifact-visualization.md
juijeong8324 Sep 3, 2023
95e1128
docs: modify img to image in artifact-visualization.md
juijeong8324 Sep 3, 2023
8d5be3a
docs: modify docs/artifact-visualization.md
juijeong8324 Sep 4, 2023
086bb9b
docs: add line break
GeunSam2 Sep 3, 2023
2748ce9
docs: replace inline html to duble space
GeunSam2 Sep 3, 2023
1d3e875
Merge pull request #1 from ArgoWorkflows-OSS/fix/docs-lint-error-for-…
juijeong8324 Sep 4, 2023
5c993a6
Merge branch 'argoproj:master' into master
juijeong8324 Sep 5, 2023
6732b5f
Merge branch 'argoproj:master' into master
juijeong8324 Sep 5, 2023
ab52986
docs : Add image file at docs/assets
juijeong8324 Sep 5, 2023
666fc25
docs: modify demo image artifact-visualization.md
juijeong8324 Sep 5, 2023
503be03
Merge branch 'argoproj:master' into master
juijeong8324 Sep 6, 2023
120d1fc
Merge branch 'argoproj:master' into master
juijeong8324 Sep 9, 2023
e70b3cf
feat: show history about completed runs in each cron workflow
juijeong8324 Sep 9, 2023
69d925e
Merge branch 'argoproj:master' into master
juijeong8324 Sep 10, 2023
8be5e72
Merge branch 'argoproj:master' into master
juijeong8324 Sep 12, 2023
789e437
design : Add cron-workflows.details.sass
juijeong8324 Sep 12, 2023
7c32559
feat : Filter in the request to pass cron-workflow
juijeong8324 Sep 12, 2023
a752691
Merge branch 'argoproj:master' into master
yunwoo-yu Sep 13, 2023
8a3b7df
Merge branch 'master' into master
yunwoo-yu Sep 13, 2023
6679692
Merge pull request #1 from juijeong8324/master
yunwoo-yu Sep 13, 2023
bb6b8e0
Revert "feat/design : Apply feedback about PR #11790"
yunwoo-yu Sep 13, 2023
0341a92
Merge pull request #2 from yunwoo-yu/revert-1-master
yunwoo-yu Sep 13, 2023
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 @@ -2,16 +2,21 @@ import {NotificationType, Page, SlidingPanel} from 'argo-ui';
import * as React from 'react';
import {useContext, useEffect, useState} from 'react';
import {RouteComponentProps} from 'react-router';
import {CronWorkflow, Link} from '../../../../models';
import * as models from '../../../../models';
import {CronWorkflow, Link, Workflow} from '../../../../models';
import {uiUrl} from '../../../shared/base';
import {ErrorNotice} from '../../../shared/components/error-notice';
import {Loading} from '../../../shared/components/loading';
import {useCollectEvent} from '../../../shared/components/use-collect-event';
import {Context} from '../../../shared/context';
import {historyUrl} from '../../../shared/history';
import {Pagination, parseLimit} from '../../../shared/pagination';
import {services} from '../../../shared/services';
import {useQueryParams} from '../../../shared/use-query-params';
import * as Actions from '../../../shared/workflow-operations-map';
import {WidgetGallery} from '../../../widgets/widget-gallery';
import {allBatchActionsEnabled, WorkflowListRenderOptions} from '../../../workflows/components/workflows-list/workflows-list';
import {WorkflowsRow} from '../../../workflows/components/workflows-row/workflows-row';
import {CronWorkflowEditor} from '../cron-workflow-editor';

require('../../../workflows/components/workflow-details/workflow-details.scss');
Expand All @@ -25,6 +30,12 @@ export const CronWorkflowDetails = ({match, location, history}: RouteComponentPr
const [name] = useState(match.params.name);
const [sidePanel, setSidePanel] = useState(queryParams.get('sidePanel'));
const [tab, setTab] = useState(queryParams.get('tab'));
const [workflows, setWorkflows] = useState<Workflow[]>([]);
const [{selectedPhases, selectedLabels, paginationLimit}, setSelectedData] = useState<WorkflowListRenderOptions>(JSON.parse(localStorage.getItem('ListOptions/options')));
Copy link
Member

@agilgur5 agilgur5 Sep 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are no phases or labels here. those are from the sidebar filter in the Workflows List which is not present here.

pagination does not seem to be present either. there seems to be some logic here, but no UI elements for pagination.

also the localStorage call would not apply here as it is specific to the Workflows list page

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Thanks I was wondering this, would it be a good idea to put a selection or pagination in the detail cron page?

Copy link
Member

@agilgur5 agilgur5 Sep 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think filtering is needed right now. pagination would be nice to have, but for a first-pass MVP, I think we can leave out pagination as well.

You may have not noticed, but the main overall Cron Workflows page itself actually doesn't implement pagination yet 😅 : #10846

const [, setBatchActionDisabled] = useState<Actions.OperationDisabled>();
const [selectedWorkflows, setSelectedWorkflows] = useState(new Map<string, models.Workflow>());
const [columns, setColumns] = useState<models.Column[]>([]);
const [pagination] = useState<Pagination>({offset: queryParams.get('offset'), limit: parseLimit(queryParams.get('limit') || `${paginationLimit}` || '50')});

const [cronWorkflow, setCronWorkflow] = useState<CronWorkflow>();
const [edited, setEdited] = useState(false);
Expand Down Expand Up @@ -62,6 +73,16 @@ export const CronWorkflowDetails = ({match, location, history}: RouteComponentPr

useEffect(() => setEdited(true), [cronWorkflow]);

useEffect(() => {
services.workflows.list(namespace, selectedPhases, selectedLabels, pagination).then(res => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we can filter in the request to not retrieve unnecessary workflows, although that might not be possible

Copy link
Member

@agilgur5 agilgur5 Sep 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is possible (and is already done in the Workflows List filter for Cron Workflows).
you can pass in a label with the Cron Workflow name: workflows.argoproj.io/cron-workflow=<name-of-cron-workflow>

That is the only label that should be passed as there is otherwise no label filter here (that is similarly only in the sidebar on the Workflows List page)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also should use async/await per #11740

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Thanks for the feedback, would it be ok to change to async/await and get the data via label names like on the workflows List page?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can replace selectedLabels (the 3rd argument to list) with the cron-workflow label from above

setWorkflows(res.items.filter(el => el.metadata.name.substring(0, el.metadata.name.lastIndexOf('-')) === name));
});

services.info.getInfo().then(info => {
setColumns(info.columns);
});
}, []);

useCollectEvent('openedCronWorkflowDetails');

const suspendButton =
Expand Down Expand Up @@ -183,6 +204,17 @@ export const CronWorkflowDetails = ({match, location, history}: RouteComponentPr

return items;
};
const updateCurrentlySelectedAndBatchActions = (newSelectedWorkflows: Map<string, Workflow>): void => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the checkbox is unused here. No batch actions were added to this page, so it's not useable currently

Copy link
Contributor Author

@yunwoo-yu yunwoo-yu Sep 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The workflows-row component has fixed selection and checkboxes. What do you think about creating a new cron-workflows-row component?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So ideally we would factor out a workflows-row without checkboxes and then use that in both places.
In particular, this history view you've created could also be useful for Workflow Templates as well, so making it reusable would be useful.

That being said, that requires a bit of refactoring. As a first-pass, we could just make the checkboxes no-op for now and refactor later -- that was why I commented on the function itself and not the UI elements.
I'll leave that decision up to you for if you want to try to tackle a refactor or just leave the checkboxes non-functional for now.

const actions: any = Actions.WorkflowOperationsMap;
const nowDisabled: any = {...allBatchActionsEnabled};
for (const action of Object.keys(nowDisabled)) {
for (const wf of Array.from(newSelectedWorkflows.values())) {
nowDisabled[action] = nowDisabled[action] || actions[action].disabled(wf);
}
}
setBatchActionDisabled(nowDisabled);
setSelectedWorkflows(new Map<string, models.Workflow>(newSelectedWorkflows));
};

return (
<Page
Expand All @@ -207,6 +239,84 @@ export const CronWorkflowDetails = ({match, location, history}: RouteComponentPr
<SlidingPanel isShown={!!sidePanel} onClose={() => setSidePanel(null)}>
{sidePanel === 'share' && <WidgetGallery namespace={namespace} label={'workflows.argoproj.io/cron-workflow=' + name} />}
</SlidingPanel>
<div className='argo-table-list'>
<div className='row argo-table-list__head'>
<div className='columns small-1 workflows-list__status'>
<input
type='checkbox'
className='workflows-list__status--checkbox'
checked={workflows.length === selectedWorkflows.size}
onClick={e => {
e.stopPropagation();
}}
onChange={e => {
if (workflows.length === selectedWorkflows.size) {
// All workflows are selected, deselect them all
updateCurrentlySelectedAndBatchActions(new Map<string, models.Workflow>());
} else {
// Not all workflows are selected, select them all
const currentlySelected: Map<string, Workflow> = selectedWorkflows;
workflows.forEach(wf => {
if (!currentlySelected.has(wf.metadata.uid)) {
currentlySelected.set(wf.metadata.uid, wf);
}
});
updateCurrentlySelectedAndBatchActions(currentlySelected);
}
}}
/>
</div>
<div className='row small-11'>
<div className='columns small-2'>NAME</div>
<div className='columns small-1'>NAMESPACE</div>
<div className='columns small-1'>STARTED</div>
<div className='columns small-1'>FINISHED</div>
<div className='columns small-1'>DURATION</div>
<div className='columns small-1'>PROGRESS</div>
<div className='columns small-2'>MESSAGE</div>
<div className='columns small-1'>DETAILS</div>
<div className='columns small-1'>ARCHIVED</div>
{(columns || []).map(col => {
return (
<div className='columns small-1' key={col.key}>
{col.name}
</div>
);
})}
</div>
</div>
{workflows.map(wf => {
return (
<WorkflowsRow
workflow={wf}
key={wf.metadata.uid}
checked={selectedWorkflows.has(wf.metadata.uid)}
columns={columns}
onChange={key => {
const value = `${key}=${wf.metadata?.labels[key]}`;
let newTags: string[] = [];
if (selectedLabels.indexOf(value) === -1) {
newTags = selectedLabels.concat(value);
setSelectedData(prev => ({...prev, selectedLabels: newTags}));
}
}}
select={subWf => {
const wfUID = subWf.metadata.uid;
if (!wfUID) {
return;
}
const currentlySelected: Map<string, Workflow> = selectedWorkflows;
if (!currentlySelected.has(wfUID)) {
currentlySelected.set(wfUID, subWf);
} else {
currentlySelected.delete(wfUID);
}
updateCurrentlySelectedAndBatchActions(currentlySelected);
}}
/>
);
})}
</div>
</>
</Page>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,13 @@ interface State {
columns: models.Column[];
}

interface WorkflowListRenderOptions {
export interface WorkflowListRenderOptions {
paginationLimit: number;
selectedPhases: WorkflowPhase[];
selectedLabels: string[];
}

const allBatchActionsEnabled: Actions.OperationDisabled = {
export const allBatchActionsEnabled: Actions.OperationDisabled = {
RETRY: false,
RESUBMIT: false,
SUSPEND: false,
Expand Down