feat(ui): Add drawer with more details for each workflow in Workflow List #3151

rbreeze · 2020-06-03T00:10:44Z

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
I've signed the CLA.
I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
My builds are green. Try syncing with master if they are not.
My organization is added to USERS.md.

…abel in the new labels column

… to take up less real estate.

…ait from labels column

…mponent to keep track of show/hide state. Reformat column widths for readability and change format of labels

…ces duration, and conditions

alexec · 2020-06-03T00:26:38Z

be very selective in the amount of data you query yes? there maybe >2k workflow listed

rbreeze · 2020-06-03T00:34:36Z

@alexec Is the data I've added so far too much? I discussed this a bit with @simster7 and those were the fields he suggested but I can remove some if needed. I also believe the drawer doesn't get rendered until a user clicks the SHOW button

simster7 · 2020-06-03T00:41:29Z

be very selective in the amount of data you query yes? there maybe >2k workflow listed

I believe the data has already been transmitted to the UI at this point, it's simply being surfaced now. Also, it might be a bit hard to see but the drawer is only shown – and therefore only rendered by React – if a user decides to display it.

rbreeze · 2020-06-03T15:58:42Z

I believe the data has already been transmitted to the UI at this point, it's simply being surfaced now.

Yeah, each workflow object is already there in its entirety, I'm just displaying more of its properties in the drawer.

alexec

believe the data has already been transmitted to the UI at this point,

We should not be transmitting the data of EVERY workflow that is loaded into this page. Only top line items. Can I confirm that if you open the drawer, then you are only loading the workflow at that point?

simster7 · 2020-06-03T16:45:44Z

We should not be transmitting the data of EVERY workflow that is loaded into this page. Only top line items. Can I confirm that if you open the drawer, then you are only loading the workflow at that point?

Sorry, not sure I understand what your concern is here. Could you help clarify?

If you're concerned about sending more information over-the-wire, then this PR does not send any new information that was not previously sent before. As of right now, we send Workflow objects in their entirety to the endpoint used by the timeline page. This PR simply exposes more of the information that is already being sent. If you're proposing we strip Workflow objects sent through this endpoint down to only the necessary fields to display them, then that would be out of scope of this PR.

If you're concerned about the UI having performance issues when loading or displaying the information that it has received, then React renders the information intelligently and will only do so if the user clicks the "Show" button.

alexec · 2020-06-03T17:32:06Z

@simster7 I'm confused - the screen shot in the description is of the workflow list page - but you're talking about the timeline page - I don't know what page that is - the only thing it might be is the timeline tab fo the workflow page?

Can you confirm which page makes these changes?

alexec · 2020-06-03T17:38:20Z

Looking at the changes, this is the workflow list page. This page should not be listing all workflow details - only those relevant to the list. This is to reduce network I/O for long lists (1000+ workflows).

This should be achieved by line 26 in workflow-service.ts:

fields=metadata,items.metadata.name,items.metadata.namespace,items.status.phase,items.status.finishedAt,items.status.startedAt

If that is actually returning more information, then we have a bug that this PR has revealed. However, what I expect is happening is that events that come from /workflow-events contain complete workflows. If that is the case, this solution will probably not work for large numbers of workflows - as you won't get that data and so they'll - missing the data you're rendering - I don't know how the changes handle that - but it could crash the UI.

If I'm correct, then can you make sure you're only getting the specific minimal data you need for this? Even adding labels to WorkflowService.list might be a lot.

Or, maybe remove the drawer if that data is missing.

rbreeze · 2020-06-03T17:54:39Z

I'm having trouble understanding what you mean by events coming from /workflow-events; I can't find any reference to it in workflows-list.tsx, is there data being passed from workflow-events that I'm missing? Either way, it does seem that there is extra data coming from the api call at line 26 in workflow-service.ts that wasn't requested, namely in the status portion. However I believe that the labels are included in the metadata portion of the request though so it makes sense that they'd be there.

The way the changes handle missing data is by checking if each parameter exists in the workflow object, and if they don't it just doesn't render that portion - so I think in the case of missing data it would be ok.

In case this helps clarify anything, I've included a screenshot of a workflow object that's part of the workflows state in the WorkflowsList React component. This is what I used as a reference when constructing the drawer, and I didn't request any additional data from the API but rather only used what was there.

alexec · 2020-06-03T19:33:31Z

You're correct, we're talking about the same page. I was using the term "timeline"

I guess it is call the timeline then!

I suggest lazy-loading the workflow using WorkflowService.get - that way you'll have all the data you need without any problems.

I do like the idea of casually inspecting workflow like this. I think people will find it really useful.

rbreeze · 2020-06-03T19:43:35Z

great, sounds good! I'll start working on that. Do you think the lazy loading requires its own PR or should I include it in this one?

rbreeze · 2020-06-03T20:11:16Z

Upon inspection of the data returned directly from that API call, I'm finding that the response directly from the server (inspected with the Network dev tools) is exactly as expected without the additional data. However, somewhere between the server's response and the returned value of the services.workflows.list call, the extra data is being added somehow.

I'm thinking there are two places this could happen:

On line 27 of workflows-service.ts where the response is cast as a WorkflowList return requests.get(...).then(res => res.body as WorkflowList); This seems unlikely since res.body doesn't contain the data that's appearing, and at most this typecast would just assign null to the values that were missing.
The subscription is somehow filling in the gaps. However, I'm not sure how this would occur either as before the subscription is unsubscribed before the API call and only resubscribed after the spot where the data is appearing (the console.log statement below).

In workflows-list.tsx:

if (this.subscription) {
    this.subscription.unsubscribe();
}
...
if (!this.state.initialized) {
    workflowList = services.info.getInfo().then(info => {
        ...
        this.setState({initialized: true, managedNamespace: !!info.managedNamespace});
        return services.workflows.list(newNamespace, selectedPhases, selectedLabels, pagination);
    });
} else {
    ...
    workflowList = services.workflows.list(newNamespace, selectedPhases, selectedLabels, pagination);
}

workflowList
.then(wfList => {
    console.log(wfList)
    ...
}).then(() => {
    this.subscription = services.workflows
        .watch({namespace: newNamespace, phases: selectedPhases, labels: selectedLabels})
        .map(workflowChange => {
             ...

rbreeze · 2020-06-03T20:21:28Z

This console.log statement in workflows-service.ts also has the extra data in it. Yet, inspecting the actual network transaction shows that the server response doesn't have the extra data.

return requests.get(`api/v1/workflows/${namespace}?${params.join('&')}`).then(res => {
    console.log(res.body)
    return res.body as WorkflowList
});

Server response, from network inspector tab (the response we expect / want):

Output of console.log statement from above:

There are no additional requests to that endpoint, and while I suspect that the websocket is probably the culprit, I can't figure out how it would be injecting more data so soon after the return of that API call.

simster7 · 2020-06-03T20:36:46Z

great, sounds good! I'll start working on that. Do you think the lazy loading requires its own PR or should I include it in this one?

I would do the lazy loading in this PR.

You seem to have gotten a great start in debugging the Workflow object issue. Would you like to own it? If so, I'd recommend trying to fix it in a separate PR. Also, if you need help feel free to ping me

rbreeze · 2020-06-03T20:40:18Z

I can definitely debug the Workflow object issue, I'll open an issue and PR this afternoon. Thanks!

…ixes argoproj#3163

simster7

Looking good so far! First round of comments

simster7 · 2020-06-04T16:30:28Z

ui/src/app/shared/services/workflows-service.ts

+        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('&')}`;


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

simster7 · 2020-06-04T16:33:12Z

ui/src/app/workflows/components/workflow-drawer/workflow-drawer.tsx

+                                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>


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

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

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.

simster7 · 2020-06-04T16:34:06Z

ui/src/app/workflows/components/workflows-row/workflows-row.tsx

                )}
            </div>
        );
    }
+
+    private fetchWorkflow(): void {


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

simster7 · 2020-06-04T16:34:46Z

ui/src/app/workflows/components/workflows-row/workflows-row.tsx


 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}> {


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

interface WorkflowRowState { ... }

simster7 · 2020-06-04T16:35:49Z

ui/src/app/workflows/components/workflow-drawer/workflow-drawer.tsx

+    onChange: (key: string) => void;
+}
+
+export class WorkflowDrawer extends React.Component<WorkflowDrawerProps, {}> {


I would change the ordering things are displayed here to be

Message

Conditions

Resource Durations

Labels

simster7 · 2020-06-04T16:36:24Z

pkg/apiclient/workflow/workflow.proto

@@ -89,6 +89,7 @@ message WorkflowDeleteResponse {
 message WatchWorkflowsRequest {
    string namespace = 1;
    k8s.io.apimachinery.pkg.apis.meta.v1.ListOptions listOptions = 2;
+    string fields = 3;


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

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?

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

…mponent state

simster7

Almost there, just one more comment

simster7 · 2020-06-04T18:49:03Z

pkg/apiclient/workflow/workflow.pb.go

@@ -806,6 +806,7 @@ var xxx_messageInfo_WorkflowDeleteResponse proto.InternalMessageInfo
 type WatchWorkflowsRequest struct {
 	Namespace            string          `protobuf:"bytes,1,opt,name=namespace,proto3" json:"namespace,omitempty"`
 	ListOptions          *v1.ListOptions `protobuf:"bytes,2,opt,name=listOptions,proto3" json:"listOptions,omitempty"`
+	Fields               string          `protobuf:"bytes,3,opt,name=fields,proto3" json:"fields,omitempty"`


Still need to get rid of changes in this file. Doing a git checkout master pkg/apiclient/workflow/workflow.pb.go should suffice

simster7

Excellent work!

sonarcloud · 2020-06-04T21:44:29Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
No Duplication information

Concerns addressed by lazy-loading full Workflow infomration

rbreeze added 18 commits June 1, 2020 10:34

Add labels column to workflows list

8b20eda

feat(ui): add ability to filter workflows by label by clicking on a l…

736cd00

…abel in the new labels column

Refactor labels column into its own component. Add show / hide button…

6af5d5d

… to take up less real estate.

fix(ui): linting issue with FontAwesome tags not being self closing

0b4cf34

Fix no new line at EOF issue

5001871

Linted

e88cd10

Remove text-decoration from SHOW / HIDE buttons and remove async / aw…

b61ca97

…ait from labels column

Merge branch 'master' of github.com:argoproj/argo into label-filtering

58e61f4

Resize workflows list columns for better visibility.

9a7b5dc

Change variable name of workflow to match convention

a7a7c02

Linted

376d96f

Refactor workflows list by separating out rows into an independent co…

7186409

…mponent to keep track of show/hide state. Reformat column widths for readability and change format of labels

Merge branch 'master' of github.com:argoproj/argo into label-filtering

27db356

Style labels to emphasize which row they belong to

85e115a

Fix colors of label drawer for better contrast

c0b630f

feat(ui): Add more details to workflow labels drawer: message, resour…

1a5031f

…ces duration, and conditions

Linted

d85d192

Resolve merge conflicts

45eab5d

rbreeze added 2 commits June 2, 2020 17:29

Fix format of labels

b24ee90

Linted

251f89f

simster7 self-assigned this Jun 3, 2020

alexec previously requested changes Jun 3, 2020

View reviewed changes

rbreeze mentioned this pull request Jun 3, 2020

bug(ui): Workflow List component receiving too much information from server #3163

Closed

4 tasks

rbreeze added 4 commits June 3, 2020 14:10

Add lazy loading for workflows list drawer

7ec9961

Merge branch 'master' of github.com:argoproj/argo into info-drawer

59c21b0

fix(server): Allow for field selection for workflow-event endpoint. F…

ae73519

…ixes argoproj#3163

Fix lazy loading referencing prop instead of state

084f923

rbreeze requested a review from jessesuen as a code owner June 3, 2020 23:52

simster7 added 4 commits June 4, 2020 09:19

Codegen

1017094

Merge branch 'master' into HEAD

161c3cb

Revert go.mod and go.sum changes

42155c3

Swagger

f209bdb

simster7 suggested changes Jun 4, 2020

View reviewed changes

rbreeze added 4 commits June 4, 2020 09:50

Reorder display of info in drawer, small refactor of Workflows Row co…

e572625

…mponent state

Merge branch 'master' of github.com:argoproj/argo into info-drawer

6c686d8

Remove changes from argoproj#3165

7d9cbe0

Reformat drawer to better display workflow conditions

0231220

simster7 reviewed Jun 4, 2020

View reviewed changes

rbreeze and others added 3 commits June 4, 2020 12:44

Revert generated file to older version

2720473

Change how conditions are displayed in workflow drawer

dce8c0d

Lint

57fb2d3

simster7 approved these changes Jun 4, 2020

View reviewed changes

simster7 merged commit a130d48 into argoproj:master Jun 4, 2020

rbreeze deleted the info-drawer branch June 4, 2020 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): Add drawer with more details for each workflow in Workflow List #3151

feat(ui): Add drawer with more details for each workflow in Workflow List #3151

rbreeze commented Jun 3, 2020

alexec commented Jun 3, 2020

rbreeze commented Jun 3, 2020

simster7 commented Jun 3, 2020

rbreeze commented Jun 3, 2020

alexec left a comment

simster7 commented Jun 3, 2020

alexec commented Jun 3, 2020

alexec commented Jun 3, 2020

rbreeze commented Jun 3, 2020

alexec commented Jun 3, 2020

rbreeze commented Jun 3, 2020

rbreeze commented Jun 3, 2020 •

edited

rbreeze commented Jun 3, 2020

simster7 commented Jun 3, 2020

rbreeze commented Jun 3, 2020

simster7 left a comment

simster7 Jun 4, 2020

simster7 Jun 4, 2020

rbreeze Jun 4, 2020

simster7 Jun 4, 2020

simster7 Jun 4, 2020

simster7 Jun 4, 2020

simster7 Jun 4, 2020

rbreeze Jun 4, 2020

simster7 Jun 4, 2020

simster7 left a comment

simster7 Jun 4, 2020

simster7 left a comment

sonarcloud bot commented Jun 4, 2020

feat(ui): Add drawer with more details for each workflow in Workflow List #3151

feat(ui): Add drawer with more details for each workflow in Workflow List #3151

Conversation

rbreeze commented Jun 3, 2020

alexec commented Jun 3, 2020

rbreeze commented Jun 3, 2020

simster7 commented Jun 3, 2020

rbreeze commented Jun 3, 2020

alexec left a comment

Choose a reason for hiding this comment

simster7 commented Jun 3, 2020

alexec commented Jun 3, 2020

alexec commented Jun 3, 2020

rbreeze commented Jun 3, 2020

alexec commented Jun 3, 2020

rbreeze commented Jun 3, 2020

rbreeze commented Jun 3, 2020 • edited

rbreeze commented Jun 3, 2020

simster7 commented Jun 3, 2020

rbreeze commented Jun 3, 2020

simster7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simster7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simster7 left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 4, 2020

rbreeze commented Jun 3, 2020 •

edited