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(ui): Add drawer with more details for each workflow in Workflow List #3151

Merged
merged 35 commits into from Jun 4, 2020
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
8b20eda
Add labels column to workflows list
rbreeze Jun 1, 2020
736cd00
feat(ui): add ability to filter workflows by label by clicking on a l…
rbreeze Jun 1, 2020
6af5d5d
Refactor labels column into its own component. Add show / hide button…
rbreeze Jun 1, 2020
0b4cf34
fix(ui): linting issue with FontAwesome tags not being self closing
rbreeze Jun 1, 2020
5001871
Fix no new line at EOF issue
rbreeze Jun 1, 2020
e88cd10
Linted
rbreeze Jun 2, 2020
b61ca97
Remove text-decoration from SHOW / HIDE buttons and remove async / aw…
rbreeze Jun 2, 2020
58e61f4
Merge branch 'master' of github.com:argoproj/argo into label-filtering
rbreeze Jun 2, 2020
9a7b5dc
Resize workflows list columns for better visibility.
rbreeze Jun 2, 2020
a7a7c02
Change variable name of workflow to match convention
rbreeze Jun 2, 2020
376d96f
Linted
rbreeze Jun 2, 2020
7186409
Refactor workflows list by separating out rows into an independent co…
rbreeze Jun 2, 2020
27db356
Merge branch 'master' of github.com:argoproj/argo into label-filtering
rbreeze Jun 2, 2020
85e115a
Style labels to emphasize which row they belong to
rbreeze Jun 2, 2020
c0b630f
Fix colors of label drawer for better contrast
rbreeze Jun 2, 2020
1a5031f
feat(ui): Add more details to workflow labels drawer: message, resour…
rbreeze Jun 3, 2020
d85d192
Linted
rbreeze Jun 3, 2020
45eab5d
Resolve merge conflicts
rbreeze Jun 3, 2020
b24ee90
Fix format of labels
rbreeze Jun 3, 2020
251f89f
Linted
rbreeze Jun 3, 2020
7ec9961
Add lazy loading for workflows list drawer
rbreeze Jun 3, 2020
59c21b0
Merge branch 'master' of github.com:argoproj/argo into info-drawer
rbreeze Jun 3, 2020
ae73519
fix(server): Allow for field selection for workflow-event endpoint. F…
rbreeze Jun 3, 2020
084f923
Fix lazy loading referencing prop instead of state
rbreeze Jun 3, 2020
1017094
Codegen
simster7 Jun 4, 2020
161c3cb
Merge branch 'master' into HEAD
simster7 Jun 4, 2020
42155c3
Revert go.mod and go.sum changes
simster7 Jun 4, 2020
f209bdb
Swagger
simster7 Jun 4, 2020
e572625
Reorder display of info in drawer, small refactor of Workflows Row co…
rbreeze Jun 4, 2020
6c686d8
Merge branch 'master' of github.com:argoproj/argo into info-drawer
rbreeze Jun 4, 2020
7d9cbe0
Remove changes from #3165
rbreeze Jun 4, 2020
0231220
Reformat drawer to better display workflow conditions
rbreeze Jun 4, 2020
2720473
Revert generated file to older version
rbreeze Jun 4, 2020
dce8c0d
Change how conditions are displayed in workflow drawer
rbreeze Jun 4, 2020
57fb2d3
Lint
simster7 Jun 4, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/openapi-spec/swagger.json
Expand Up @@ -754,6 +754,11 @@
"description": "The continue option should be set when retrieving more results from the server. Since this value is\nserver defined, clients may only use the continue value from a previous query result with identical\nquery parameters (except for the value of continue) and the server may reject a continue value it\ndoes not recognize. If the specified continue value is no longer valid whether due to expiration\n(generally five to fifteen minutes) or a configuration change on the server, the server will\nrespond with a 410 ResourceExpired error together with a continue token. If the client needs a\nconsistent list, it must restart their list without the continue field. Otherwise, the client may\nsend another list request with the token received with the 410 error, the server will respond with\na list starting from the next key, but from the latest snapshot, which is inconsistent from the\nprevious list results - objects that are created, modified, or deleted after the first list request\nwill be included in the response, as long as their keys are after the \"next key\".\n\nThis field is not supported when watch is true. Clients may start a watch from the last\nresourceVersion value returned by the server and not miss any modifications.",
"name": "listOptions.continue",
"in": "query"
},
{
"type": "string",
"name": "fields",
"in": "query"
}
],
"responses": {
Expand Down
212 changes: 131 additions & 81 deletions pkg/apiclient/workflow/workflow.pb.go

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions pkg/apiclient/workflow/workflow.proto
Expand Up @@ -89,6 +89,7 @@ message WorkflowDeleteResponse {
message WatchWorkflowsRequest {
string namespace = 1;
k8s.io.apimachinery.pkg.apis.meta.v1.ListOptions listOptions = 2;
string fields = 3;
Copy link
Member

Choose a reason for hiding this comment

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

Likewise here. Shouldn't this be done in #3165 instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was due to a gap in my understanding of git - my commits from the #3165 branch were added to this branch. I was trying to debug this enhancement with the changes I made in #3165 without considering that accordingly those changes would be added to this branch.

To resolve this, should I simply delete the lines in question and recommit?

Copy link
Member

Choose a reason for hiding this comment

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

No worries. Yes, I would simply delete these lines and make sure that they are reverted exactly to what they were before by making sure they didn't change with git diff master

}

message WorkflowWatchEvent {
Expand Down
6 changes: 6 additions & 0 deletions pkg/apiclient/workflow/workflow.swagger.json
Expand Up @@ -98,6 +98,12 @@
"in": "query",
"required": false,
"type": "string"
},
{
"name": "fields",
"in": "query",
"required": false,
"type": "string"
}
],
"tags": [
Expand Down
6 changes: 5 additions & 1 deletion ui/src/app/shared/services/workflows-service.ts
Expand Up @@ -32,7 +32,11 @@ export class WorkflowsService {
}

public watch(filter: {namespace?: string; name?: string; phases?: Array<string>; labels?: Array<string>}): Observable<models.kubernetes.WatchEvent<Workflow>> {
const url = `api/v1/workflow-events/${filter.namespace || ''}?${this.queryParams(filter).join('&')}`;
const params = this.queryParams(filter);
params.push(
`fields=result.object.metadata.name,result.object.metadata.namespace,result.object.status.finishedAt,result.object.status.startedAt,result.object.status.phase`
);
const url = `api/v1/workflow-events/${filter.namespace || ''}?${params.join('&')}`;
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be fixing the workflow-events endpoint issue. Shouldn't this be done in #3165 instead?


return requests.loadEventSource(url, true).map(data => JSON.parse(data).result as models.kubernetes.WatchEvent<Workflow>);
}
Expand Down
123 changes: 123 additions & 0 deletions ui/src/app/workflows/components/workflow-drawer/workflow-drawer.scss
@@ -0,0 +1,123 @@
@import 'node_modules/argo-ui/src/styles/config';

.workflow-drawer {
height: 100%;
font-size: .8125em;
background-color: $argo-color-gray-1;
margin-top: -8px;
padding: 1.5em 15px;
border-radius: 0 0 3px 3px;

&__section {
padding: 1em 0;
box-sizing: border-box;
border-top: 2px solid $argo-color-gray-3;

&:first-child {
padding-top: 0;
}

&:last-child {
padding-bottom: 0;
}

&:only-child {
padding: 0;
}
}

&__message {

&--label {
margin-bottom: 1em;
}

&--content {
font-weight: 600;
}

}

&__labels {
border-top: none;
&--list {
margin-top: 0.5em;
}
}

.tag {
line-height: normal;
background-color: $argo-color-gray-3;
border-radius: 3px;
height: auto;
margin: 0.5em 0;
margin-right: 10px;
display: flex;
overflow: hidden;
cursor: pointer;

&:hover {
.key {
color: $argo-color-teal-5;
}
.value {
color: $argo-color-gray-3;
}
}

.key, .value {
padding: 0.5em 8px;
}

.key {
color: $argo-color-gray-6;
}

.value {
color: white;
background-color: $argo-color-teal-5;
}
}

&__resourcesDuration {
display: flex;
align-items: center;

$gutter: 2em;

&--label {
margin-left: 0.5em;
}

&--value {
font-weight: 600;
}

&--container {
padding-left: $gutter / 2;
margin-left: $gutter / 2;
border-left: 1px solid $argo-color-gray-3;
}

}

&__conditions {
display: flex;
margin-top: 0.5em;

.tag {
cursor: default;
&:hover {

.key {
color: $argo-color-gray-6;
}

.value {
color: white;
}

}
}
}
}
@@ -0,0 +1,75 @@
import * as React from 'react';
import * as models from '../../../../models';

import {WorkflowLabels} from '../workflow-labels/workflow-labels';

require('./workflow-drawer.scss');

interface WorkflowDrawerProps {
workflow: models.Workflow;
onChange: (key: string) => void;
}

export class WorkflowDrawer extends React.Component<WorkflowDrawerProps, {}> {
Copy link
Member

Choose a reason for hiding this comment

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

I would change the ordering things are displayed here to be

  1. Message
  2. Conditions
  3. Resource Durations
  4. Labels

public render() {
const wf = this.props.workflow;
return (
<div className='workflow-drawer'>
<div className='workflow-drawer__section workflow-drawer__labels'>
<div className='workflow-drawer__title'>LABELS</div>
<div className='workflow-drawer__labels--list'>
<WorkflowLabels
workflow={wf}
onChange={key => {
this.props.onChange(key);
}}
/>
</div>
</div>
{!wf.status.message ? null : (
<div className='workflow-drawer__section workflow-drawer__message'>
<div className='workflow-drawer__title workflow-drawer__message--label'>MESSAGE</div>
<div className='workflow-drawer__message--content'>{wf.status.message}</div>
</div>
)}
{!wf.status.resourcesDuration ? null : (
<div className='workflow-drawer__section'>
<div className='workflow-drawer__resourcesDuration'>
<div className='workflow-drawer__title'>
RESOURCES DURATION&nbsp;
<a href='https://github.com/argoproj/argo/blob/master/docs/resource-duration.md' target='_blank'>
<i className='fas fa-info-circle' />
</a>
</div>
<div className='workflow-drawer__resourcesDuration--container'>
<div>
<span className='workflow-drawer__resourcesDuration--value'>{wf.status.resourcesDuration.cpu} sec</span>
<span className='workflow-drawer__resourcesDuration--label'>(*1 CPU)</span>
</div>
<div>
<span className='workflow-drawer__resourcesDuration--value'>{wf.status.resourcesDuration.memory} sec</span>
<span className='workflow-drawer__resourcesDuration--label'>(*100Mi Memory)</span>
</div>
</div>
</div>
</div>
)}
{!wf.status.conditions ? null : (
<div className='workflow-drawer__section'>
<div className='workflow-drawer__title'>CONDITIONS</div>
<div className='workflow-drawer__conditions'>
{wf.status.conditions.map(condition => {
return (
<div className='tag' key={`${wf.metadata.namespace}-${wf.metadata.name}-${condition.type}-${condition.status}`}>
<div className='key'>{condition.type}</div>
<div className='value'>{condition.status}</div>
Copy link
Member

Choose a reason for hiding this comment

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

With Conditions, the Message field is often more important than the status field. In fact, we only show the status field at all only if the message field is empty. Furthermore, these message fields can be quite long... take a look at this example
image

So I'm not sure the tag class that we use to display "labels" would work here. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see. With context, I agree - when I first saw that example, I thought that the SpecWarning portion was not itself a condition, but a constant warning displayed for every workflow.

</div>
);
})}
</div>
</div>
)}
</div>
);
}
}
Expand Up @@ -5,12 +5,9 @@
flex-wrap: wrap;
align-items: center;
height: 100%;
padding: 0.25em 0;
$gutter: 0.5em;
font-size: .8125em;
background-color: $argo-color-gray-1;
margin-top: -8px;
padding: 0.5em 15px;
border-radius: 0 0 3px 3px;

.tag {
Expand Down
Expand Up @@ -265,7 +265,7 @@ export class WorkflowsList extends BasePage<RouteComponentProps<any>, State> {
<div className='columns small-2'>STARTED</div>
<div className='columns small-2'>FINISHED</div>
<div className='columns small-1'>DURATION</div>
<div className='columns small-1'>LABELS</div>
<div className='columns small-1'>DETAILS</div>
</div>
{this.state.workflows.map(wf => {
return (
Expand Down
42 changes: 26 additions & 16 deletions ui/src/app/workflows/components/workflows-row/workflows-row.tsx
Expand Up @@ -6,20 +6,25 @@ import {uiUrl} from '../../../shared/base';
import {PhaseIcon} from '../../../shared/components/phase-icon';
import {Timestamp} from '../../../shared/components/timestamp';
import {formatDuration, wfDuration} from '../../../shared/duration';
import {WorkflowLabels} from '../workflow-labels/workflow-labels';
import {services} from '../../../shared/services';
import {WorkflowDrawer} from '../workflow-drawer/workflow-drawer';

interface WorkflowsRowProps {
workflow: models.Workflow;
onChange: (key: string) => void;
}
export class WorkflowsRow extends React.Component<WorkflowsRowProps, {hideLabels: boolean}> {

export class WorkflowsRow extends React.Component<WorkflowsRowProps, {hideDrawer: boolean; workflow: models.Workflow}> {
Copy link
Member

Choose a reason for hiding this comment

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

Can we make {hideDrawer: boolean; workflow: models.Workflow} a format type like we do with props?

interface WorkflowRowState {
...
}

constructor(props: WorkflowsRowProps) {
super(props);
this.state = {hideLabels: true};
this.state = {
workflow: this.props.workflow,
hideDrawer: true
};
}

public render() {
const wf = this.props.workflow;
const wf = this.state.workflow;
return (
<div className='workflows-list__row-container'>
<Link className='row argo-table-list__row' to={uiUrl(`workflows/${wf.metadata.namespace}/${wf.metadata.name}`)}>
Expand All @@ -42,10 +47,11 @@ export class WorkflowsRow extends React.Component<WorkflowsRowProps, {hideLabels
<div
onClick={e => {
e.preventDefault();
this.setState({hideLabels: !this.state.hideLabels});
this.fetchWorkflow();
this.setState({hideDrawer: !this.state.hideDrawer});
}}
className={`workflows-row__action workflows-row__action--${this.state.hideLabels ? 'show' : 'hide'}`}>
{this.state.hideLabels ? (
className={`workflows-row__action workflows-row__action--${this.state.hideDrawer ? 'show' : 'hide'}`}>
{this.state.hideDrawer ? (
<span>
SHOW <i className='fas fa-caret-down' />{' '}
</span>
Expand All @@ -58,19 +64,23 @@ export class WorkflowsRow extends React.Component<WorkflowsRowProps, {hideLabels
</div>
</div>
</Link>
{this.state.hideLabels ? (
{this.state.hideDrawer ? (
<span />
) : (
<div>
<WorkflowLabels
workflow={wf}
onChange={key => {
this.props.onChange(key);
}}
/>
</div>
<WorkflowDrawer
workflow={wf}
onChange={key => {
this.props.onChange(key);
}}
/>
)}
</div>
);
}

private fetchWorkflow(): void {
Copy link
Member

Choose a reason for hiding this comment

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

Nice. I would call this fetchFullWorkflow to emphasize that we have now is only a partial Workflow.

services.workflows.get(this.props.workflow.metadata.namespace, this.props.workflow.metadata.name).then(wf => {
this.setState({workflow: wf});
});
}
}