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.

Sign up for GitHub

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

Jump to bottom

refactor(ui): workflow panel components from class to functional #11803

Merged
merged 10 commits into from
Sep 15, 2023
Merged

refactor(ui): workflow panel components from class to functional #11803

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
67abb6a
refactor(ui): workflow panel components from class to functional
tetora1053 Sep 12, 2023
9db7933
fix: remove a state just set but not used
tetora1053 Sep 12, 2023
3829568
refactor: from anonymous function to named function
tetora1053 Sep 13, 2023
6737c3b
refactor: from anonymous function to named function
tetora1053 Sep 13, 2023
9e1fd46
refactor: remove type annotations for primitives
tetora1053 Sep 13, 2023
48e214d
refactor: use async/await instead of chaining promise callbacks
tetora1053 Sep 13, 2023
eb59c26
refactor: use optional chanining instead of &&
tetora1053 Sep 13, 2023
6634c0a
refactor: memoize templateOptions in entrypoint select options
tetora1053 Sep 13, 2023
0e47525
refactor: hoist up static variable out of component
tetora1053 Sep 14, 2023
106027a
fix: variables memoization
tetora1053 Sep 15, 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
155 changes: 67 additions & 88 deletions ui/src/app/workflows/components/resubmit-workflow-panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {Checkbox} from 'argo-ui';
import * as React from 'react';
import React, {useState} from 'react';
import {Parameter, ResubmitOpts, Workflow} from '../../../models';
import {uiUrl} from '../../shared/base';
import {ErrorNotice} from '../../shared/components/error-notice';
Expand All @@ -12,103 +12,82 @@ interface Props {
isArchived: boolean;
}

interface State {
overrideParameters: boolean;
workflowParameters: Parameter[];
memoized: boolean;
error?: Error;
isSubmitting: boolean;
}
export function ResubmitWorkflowPanel(props: Props) {
const [overrideParameters, setOverrideParameters] = useState(false);
const [workflowParameters, setWorkflowParameters] = useState<Parameter[]>(JSON.parse(JSON.stringify(props.workflow.spec.arguments.parameters || [])));
const [memoized, setMemoized] = useState(false);
const [error, setError] = useState<Error>();
const [isSubmitting, setIsSubmitting] = useState(false);

export class ResubmitWorkflowPanel extends React.Component<Props, State> {
constructor(props: any) {
super(props);
const state: State = {
workflowParameters: JSON.parse(JSON.stringify(this.props.workflow.spec.arguments.parameters || [])),
memoized: false,
isSubmitting: false,
overrideParameters: false
async function submit() {
setIsSubmitting(true);
const parameters: ResubmitOpts['parameters'] = overrideParameters
? [...workflowParameters.filter(p => Utils.getValueFromParameter(p) !== undefined).map(p => p.name + '=' + Utils.getValueFromParameter(p))]
: [];
const opts: ResubmitOpts = {
parameters,
memoized
};
this.state = state;

try {
const submitted = props.isArchived
? await services.workflows.resubmitArchived(props.workflow.metadata.uid, props.workflow.metadata.namespace, opts)
: await services.workflows.resubmit(props.workflow.metadata.name, props.workflow.metadata.namespace, opts);
document.location.href = uiUrl(`workflows/${submitted.metadata.namespace}/${submitted.metadata.name}`);
Copy link
Contributor

@agilgur5 agilgur5 Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this did not change, but shouldn't this use react-router navigation instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I think the current pattern is to use the History API with history.push. We are on an older version of react-router though (I'm working on upgrading it -- have to finish removing BasePage first), so I don't think the useHistory hook is available. Other components use history from props currently

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 just researching how to write in current react router version.
I'll try!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tryed passing history in props and replaced 'document.location.href' with 'history.push', but it didn't work as I expected :(

props.history.push(`workflows/${submitted.metadata.namespace}/${submitted.metadata.name}`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the scope of the changes is likely to be larger than initially expected, it might be good to reconsider the approach after your react-router version up I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it eventually update itself to in progress?

no, the icon does not change at all from this image
Screen Shot 2023-09-14 at 22 33 50

before the modification, the icon had changed yellow -> blue -> green

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. Since this is on the same page (just different Workflow), I wonder if maybe the page is not quite unloading correctly and still has the old state. The effects seem to have proper disposers and dependencies, so I don't see anything strange off the top of my head.

At this point, especially since it potentially involves a whole other page's logic, I think we should punt on the navigation improvements for now. Appreciate your efforts and investigation here! Hopefully that will help the next person to tackle this too (which could be yourself 😉 ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the retry component, the state appears to be changing as expected, so the difference in implementation between retry and resubmit may be a clue, though I haven't been able to found out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I figure it out, I'll make a PR🔥

Copy link
Contributor

Choose a reason for hiding this comment

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

so the difference in implementation between retry and resubmit may be a clue

So a retry does not create a new Workflow, it just reruns failed steps. A resubmit creates a wholly new Workflow, effectively a clone of the old one.

So for a retry, the URL does not change. For a resubmit it does.
For the retry, the existing ListWatch might be able to pick up the change automatically. For a resubmit, an entirely new request is needed for the new Workflow.

Maybe those details may help you debug?
I already knew that though (I wrote a bit of the documentation on retry vs resubmit in #11625) and couldn't figure it out, though I didn't stare too hard

} catch (err) {
setError(err);
setIsSubmitting(false);
}
}

public render() {
return (
<>
<h4>Resubmit Workflow</h4>
<h5>
{this.props.workflow.metadata.namespace}/{this.props.workflow.metadata.name}
</h5>
{this.state.error && <ErrorNotice error={this.state.error} />}
<div className='white-box'>
<div key='override-parameters' style={{marginBottom: 25}}>
<label>Override Parameters</label>
<div className='columns small-9'>
<Checkbox checked={this.state.overrideParameters} onChange={overrideParameters => this.setState({overrideParameters})} />
</div>
return (
<>
<h4>Resubmit Workflow</h4>
<h5>
{props.workflow.metadata.namespace}/{props.workflow.metadata.name}
</h5>
{error && <ErrorNotice error={error} />}
<div className='white-box'>
<div key='override-parameters' style={{marginBottom: 25}}>
<label>Override Parameters</label>
<div className='columns small-9'>
<Checkbox checked={overrideParameters} onChange={setOverrideParameters} />
</div>
</div>

{this.state.overrideParameters && (
<div key='parameters' style={{marginBottom: 25}}>
<label>Parameters</label>
{this.state.workflowParameters.length > 0 && (
<ParametersInput parameters={this.state.workflowParameters} onChange={workflowParameters => this.setState({workflowParameters})} />
)}
{this.state.workflowParameters.length === 0 ? (
<>
<br />
<label>No parameters</label>
</>
) : (
<></>
)}
</div>
)}

<div key='memoized' style={{marginBottom: 25}}>
<label>Memoized</label>
<div className='columns small-9'>
<Checkbox checked={this.state.memoized} onChange={memoized => this.setState({memoized})} />
</div>
{overrideParameters && (
<div key='parameters' style={{marginBottom: 25}}>
<label>Parameters</label>
{workflowParameters.length > 0 && <ParametersInput parameters={workflowParameters} onChange={setWorkflowParameters} />}
{workflowParameters.length === 0 && (
<>
<br />
<label>No parameters</label>
</>
)}
</div>
)}

{this.state.overrideParameters && this.state.memoized && (
<div key='warning-override-with-memoized'>
<i className='fa fa-exclamation-triangle' style={{color: '#f4c030'}} />
Overriding parameters on memoized submitted workflows may have unexpected results.
</div>
)}

<div key='resubmit'>
<button onClick={() => this.submit()} className='argo-button argo-button--base' disabled={this.state.isSubmitting}>
<i className='fa fa-plus' /> {this.state.isSubmitting ? 'Loading...' : 'Resubmit'}
</button>
<div key='memoized' style={{marginBottom: 25}}>
<label>Memoized</label>
<div className='columns small-9'>
<Checkbox checked={memoized} onChange={setMemoized} />
</div>
</div>
</>
);
}

private submit() {
this.setState({isSubmitting: true});
const parameters: ResubmitOpts['parameters'] = this.state.overrideParameters
? [...this.state.workflowParameters.filter(p => Utils.getValueFromParameter(p) !== undefined).map(p => p.name + '=' + Utils.getValueFromParameter(p))]
: [];
const opts: ResubmitOpts = {
parameters,
memoized: this.state.memoized
};
{overrideParameters && memoized && (
<div key='warning-override-with-memoized'>
<i className='fa fa-exclamation-triangle' style={{color: '#f4c030'}} />
Overriding parameters on memoized submitted workflows may have unexpected results.
</div>
)}

if (!this.props.isArchived) {
services.workflows
.resubmit(this.props.workflow.metadata.name, this.props.workflow.metadata.namespace, opts)
.then((submitted: Workflow) => (document.location.href = uiUrl(`workflows/${submitted.metadata.namespace}/${submitted.metadata.name}`)))
.catch(error => this.setState({error, isSubmitting: false}));
} else {
services.workflows
.resubmitArchived(this.props.workflow.metadata.uid, this.props.workflow.metadata.namespace, opts)
.then((submitted: Workflow) => (document.location.href = uiUrl(`workflows/${submitted.metadata.namespace}/${submitted.metadata.name}`)))
.catch(error => this.setState({error, isSubmitting: false}));
}
}
<div key='resubmit'>
<button onClick={submit} className='argo-button argo-button--base' disabled={isSubmitting}>
<i className='fa fa-plus' /> {isSubmitting ? 'Loading...' : 'Resubmit'}
</button>
</div>
</div>
</>
);
}
172 changes: 77 additions & 95 deletions ui/src/app/workflows/components/retry-workflow-panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {Checkbox} from 'argo-ui';
import * as React from 'react';
import React, {useState} from 'react';
import {Parameter, RetryOpts, Workflow} from '../../../models';
import {uiUrl} from '../../shared/base';
import {ErrorNotice} from '../../shared/components/error-notice';
Expand All @@ -12,112 +12,94 @@ interface Props {
isArchived: boolean;
}

interface State {
overrideParameters: boolean;
restartSuccessful: boolean;
workflowParameters: Parameter[];
nodeFieldSelector: string;
error?: Error;
isSubmitting: boolean;
}
export function RetryWorkflowPanel(props: Props) {
const [overrideParameters, setOverrideParameters] = useState(false);
const [restartSuccessful, setRestartSuccessful] = useState(false);
const [workflowParameters, setWorkflowParameters] = useState<Parameter[]>(JSON.parse(JSON.stringify(props.workflow.spec.arguments.parameters || [])));
const [nodeFieldSelector, setNodeFieldSelector] = useState('');
const [error, setError] = useState<Error>();
const [isSubmitting, setIsSubmitting] = useState(false);

export class RetryWorkflowPanel extends React.Component<Props, State> {
constructor(props: any) {
super(props);
const state: State = {
workflowParameters: JSON.parse(JSON.stringify(this.props.workflow.spec.arguments.parameters || [])),
isSubmitting: false,
overrideParameters: false,
nodeFieldSelector: '',
restartSuccessful: false
async function submit() {
setIsSubmitting(true);
const parameters: RetryOpts['parameters'] = overrideParameters
? [...workflowParameters.filter(p => Utils.getValueFromParameter(p) !== undefined).map(p => p.name + '=' + Utils.getValueFromParameter(p))]
: [];
const opts: RetryOpts = {
parameters,
restartSuccessful,
nodeFieldSelector
};
this.state = state;

try {
const submitted = props.isArchived
? await services.workflows.retryArchived(props.workflow.metadata.uid, props.workflow.metadata.namespace, opts)
: await services.workflows.retry(props.workflow.metadata.name, props.workflow.metadata.namespace, opts);
document.location.href = uiUrl(`workflows/${submitted.metadata.namespace}/${submitted.metadata.name}`);
} catch (err) {
setError(err);
setIsSubmitting(false);
}
}

public render() {
return (
<>
<h4>Retry Workflow</h4>
<h5>
{this.props.workflow.metadata.namespace}/{this.props.workflow.metadata.name}
</h5>
return (
<>
<h4>Retry Workflow</h4>
<h5>
{props.workflow.metadata.namespace}/{props.workflow.metadata.name}
</h5>

{this.state.error && <ErrorNotice error={this.state.error} />}
<div className='white-box'>
<div key='override-parameters' style={{marginBottom: 25}}>
<label>Override Parameters</label>
<div className='columns small-9'>
<Checkbox checked={this.state.overrideParameters} onChange={overrideParameters => this.setState({overrideParameters})} />
</div>
{error && <ErrorNotice error={error} />}
<div className='white-box'>
{/* Override Parameters */}
<div key='override-parameters' style={{marginBottom: 25}}>
<label>Override Parameters</label>
<div className='columns small-9'>
<Checkbox checked={overrideParameters} onChange={setOverrideParameters} />
</div>
</div>

{this.state.overrideParameters && (
<div key='parameters' style={{marginBottom: 25}}>
<label>Parameters</label>
{this.state.workflowParameters.length > 0 && (
<ParametersInput parameters={this.state.workflowParameters} onChange={workflowParameters => this.setState({workflowParameters})} />
)}
{this.state.workflowParameters.length === 0 ? (
<>
<br />
<label>No parameters</label>
</>
) : (
<></>
)}
</div>
)}
{overrideParameters && (
<div key='parameters' style={{marginBottom: 25}}>
<label>Parameters</label>
{workflowParameters.length > 0 ? (
<ParametersInput parameters={workflowParameters} onChange={setWorkflowParameters} />
) : (
<>
<br />
<label>No parameters</label>
</>
)}
</div>
)}

<div key='restart-successful' style={{marginBottom: 25}}>
<label>Restart Successful</label>
<div className='columns small-9'>
<Checkbox checked={this.state.restartSuccessful} onChange={restartSuccessful => this.setState({restartSuccessful})} />
</div>
{/* Restart Successful */}
<div key='restart-successful' style={{marginBottom: 25}}>
<label>Restart Successful</label>
<div className='columns small-9'>
<Checkbox checked={restartSuccessful} onChange={setRestartSuccessful} />
</div>
</div>

{this.state.restartSuccessful && (
<div key='node-field-selector' style={{marginBottom: 25}}>
<label>
Node Field Selector to restart nodes. <a href='https://argoproj.github.io/argo-workflows/node-field-selector/'>See document</a>.
</label>
{restartSuccessful && (
<div key='node-field-selector' style={{marginBottom: 25}}>
<label>
Node Field Selector to restart nodes. <a href='https://argoproj.github.io/argo-workflows/node-field-selector/'>See document</a>.
</label>

<div className='columns small-9'>
<textarea className='argo-field' value={this.state.nodeFieldSelector} onChange={e => this.setState({nodeFieldSelector: e.target.value})} />
</div>
<div className='columns small-9'>
<textarea className='argo-field' value={nodeFieldSelector} onChange={e => setNodeFieldSelector(e.target.value)} />
</div>
)}

<div key='retry'>
<button onClick={() => this.submit()} className='argo-button argo-button--base' disabled={this.state.isSubmitting}>
<i className='fa fa-plus' /> {this.state.isSubmitting ? 'Loading...' : 'Retry'}
</button>
</div>
</div>
</>
);
}
)}

private submit() {
this.setState({isSubmitting: true});
const parameters: RetryOpts['parameters'] = this.state.overrideParameters
? [...this.state.workflowParameters.filter(p => Utils.getValueFromParameter(p) !== undefined).map(p => p.name + '=' + Utils.getValueFromParameter(p))]
: [];
const opts: RetryOpts = {
parameters,
restartSuccessful: this.state.restartSuccessful,
nodeFieldSelector: this.state.nodeFieldSelector
};

if (!this.props.isArchived) {
services.workflows
.retry(this.props.workflow.metadata.name, this.props.workflow.metadata.namespace, opts)
.then((submitted: Workflow) => (document.location.href = uiUrl(`workflows/${submitted.metadata.namespace}/${submitted.metadata.name}`)))
.catch(error => this.setState({error, isSubmitting: false}));
} else {
services.workflows
.retryArchived(this.props.workflow.metadata.uid, this.props.workflow.metadata.namespace, opts)
.then((submitted: Workflow) => (document.location.href = uiUrl(`workflows/${submitted.metadata.namespace}/${submitted.metadata.name}`)))
.catch(error => this.setState({error, isSubmitting: false}));
}
}
{/* Retry button */}
<div key='retry'>
<button onClick={submit} className='argo-button argo-button--base' disabled={isSubmitting}>
<i className='fa fa-plus' /> {isSubmitting ? 'Loading...' : 'Retry'}
</button>
</div>
</div>
</>
);
}
Loading