Add pipeline trigger resource pages and related links in details pages #5005

karthikjeeyar · 2020-04-09T21:44:20Z

Fixes:- https://issues.redhat.com/browse/ODC-3427

Analysis / Root cause:
User should be able to traverse between the trigger resources quickly from the details page.

Solution Description:
Added custom pages for the trigger resources and added the links of the related resources.

screenshot:

Pipeline Details

Trigger Template Details

Eventlistener-details

Unit test:

Browser conformance:

Chrome
Firefox
Safari
Edge

cc: @serenamarie125 @andrewballantyne

karthikjeeyar · 2020-04-09T21:48:59Z

/kind feature

karthikjeeyar · 2020-04-09T21:49:19Z

/assign @andrewballantyne

andrewballantyne · 2020-04-13T07:24:51Z

...end/packages/dev-console/src/components/pipelineruns/detail-page-tabs/PipelineRunDetails.tsx

@@ -31,6 +37,22 @@ export const PipelineRunDetails: React.FC<PipelineRunDetailsProps> = ({ obj: pip
            </dd>
          </dl>
          <br />
+          <dl>
+            <dt>Triggered by</dt>


Oops, you overstepped your story. Please undo changes here. They'll not be adequate to cover the need. See my PR if you're curious how rough it was lol. There is a need to get the username and apply it to the PipelineRun (can't just use the current user at time of read).

FWIW, this is the story that covers the Pipeline Run / Trigged By content: https://issues.redhat.com/browse/ODC-3323

andrewballantyne · 2020-04-13T07:25:28Z

...v-console/src/components/pipelineruns/detail-page-tabs/__tests__/PipelineRunDetails.spec.tsx

@@ -57,4 +75,28 @@ describe('PipelineRun details page', () => {
      .filter({ kind: referenceForModel(PipelineResourceModel) });
    expect(resources).toHaveLength(0);
  });
+
+  it('should return username when labels are not present in the pipeline run object', () => {
+    expect(wrapper.find('[data-test-id="triggered-by"]').text()).toBe('kube:admin');


Naturally undo all of these as well.

andrewballantyne · 2020-04-13T08:01:20Z

...tend/packages/dev-console/src/components/pipelines/detail-page-tabs/EventListenerDetails.tsx

+  const { template, bindings } =
+    eventListener?.spec?.triggers?.reduce(
+      (res, trigger) => {
+        res.template.push(trigger.template.name);
+        trigger.bindings.map((b) => res.bindings.push(b.name));
+        return res;
+      },
+      { ...resources },
+    ) || resources;


Suggested change

const { template, bindings } =

eventListener?.spec?.triggers?.reduce(

(res, trigger) => {

res.template.push(trigger.template.name);

trigger.bindings.map((b) => res.bindings.push(b.name));

return res;

},

{ ...resources },

) || resources;

const { template, bindings } =

(eventListener.spec.triggers || []).reduce(

(acc, trigger) => ({

bindings: [...acc.bindings, trigger.bindings],

template: [...acc.template, trigger.template],

}),

{ template: [], bindings: [] },

);

Rule of thumb is not to mutate outside resources during a .map or .reduce as they are intended to be as pure as possible. It's just easier for others to rationalize them out if you keep to these guidelines.

Also, EventListenerKind has spec.triggers as being a required path. So we either have an invalid type or unnecessary null-checks.

One last thing is that .reduce will return the default if the array is empty so this structure allows you not to need to do an extra ||.

updated it.

andrewballantyne · 2020-04-13T08:07:16Z

...tend/packages/dev-console/src/components/pipelines/detail-page-tabs/EventListenerDetails.tsx

+import ResourceLinkList from '../resource-overview/ResourceLinkList';
+
+export interface EventListenerDetailsProps {
+  obj: K8sResourceKind;


Suggested change

obj: K8sResourceKind;

obj: EventListenerKind;

andrewballantyne · 2020-04-13T08:09:33Z

frontend/packages/dev-console/src/components/pipelines/resource-overview/ResourceLinkList.tsx

+import { ResourceLink } from '@console/internal/components/utils';
+import './ResourceLinkList.scss';
+
+type ResourceLinkListProps = {


Maybe we change the API of this to make it easier to call.

Model, "links" & namespace is all we really need.

In ResourceLink component kind determines which badge to use, so it is also required.

Not sure I understand what the concern is... you can compute the values with the ModelKind:

// calling file <ResourceLinkList model={TriggerBindingModel} ... /> // ResourceLinkList const title = model.labelPlural; const kind = referenceForModel(model);

Updated the api to make it easier to call

andrewballantyne · 2020-04-13T08:12:45Z

...s/dev-console/src/components/pipelines/detail-page-tabs/pipeline-details/PipelineDetails.tsx

        </div>
-      )}
+        {pipeline.spec && pipeline.spec.tasks && (


🤦 Oh right... Pipeline spec is poorly typed. I appreciate you updating this to match types - but spec is never not there to have a proper Pipeline and since 0.9 or so of the Operator, you have to have at least 1 task.

Maybe we just update the type of the Pipeline kind and remove this check.

andrewballantyne · 2020-04-13T08:22:32Z

frontend/packages/dev-console/src/components/pipelines/resource-overview/ResourceLinkList.tsx

+            <React.Fragment key={name}>
+              <dd>
+                <ResourceLink kind={kind} name={name} namespace={namespace} title={name} inline />
+              </dd>
+            </React.Fragment>


Suggested change

<React.Fragment key={name}>

<dd>

<ResourceLink kind={kind} name={name} namespace={namespace} title={name} inline />

</dd>

</React.Fragment>

<dd key={name}>

<ResourceLink kind={kind} name={name} namespace={namespace} title={name} inline />

</dd>

Fragments are there to support multiple "root" children. Recommend you read the Fragments page on the React docs as this is not the first time I have mentioned a fragment issue.

I was not sure why added this 🤦 , clearly we dont need this, will remove it.

karthikjeeyar · 2020-04-13T12:51:04Z

/retest

serenamarie125

Few questions for you!

Does this PR cover adding trigger information on the Pipeline Run details page? I don't see a screenshot of that.
When Trigger Templates are shown, I think they should have the associated URL below them, at least that was the design. I know that piece of information is optional, but just checking
On the Pipeline details page, can we move the Trigger Templates above the Tasks : https://docs.google.com/document/d/11vShZz7TX7b8YUyTG5zBMZXFnQmPEJ8nqUapCL70yyk/edit#heading=h.woem8ztgbvh5

karthikjeeyar · 2020-04-13T15:01:56Z

@serenamarie125
PLR links are covered in this PR #5011

I have moved the trigger template above the Tasks section.

serenamarie125

thanks for the rework & explanations @karthikjeeyar LGTM! Approved by UX

andrewballantyne · 2020-04-13T17:35:50Z

...end/packages/dev-console/src/components/pipelineruns/detail-page-tabs/PipelineRunDetails.tsx

@@ -8,7 +8,6 @@ import { PipelineModel, PipelineResourceModel } from '../../../models';
 export interface PipelineRunDetailsProps {
  obj: PipelineRun;
 }
-


Net zero changes please.

andrewballantyne · 2020-04-13T17:35:55Z

...v-console/src/components/pipelineruns/detail-page-tabs/__tests__/PipelineRunDetails.spec.tsx

@@ -10,7 +10,6 @@ import PipelineRunVisualization from '../PipelineRunVisualization';

 const mockPipelineRun =
  pipelineTestData[PipelineExampleNames.SIMPLE_PIPELINE].pipelineRuns[DataState.SUCCESS];
-


Net zero changes please.

andrewballantyne · 2020-04-13T17:38:26Z

frontend/packages/dev-console/src/components/pipelines/resource-overview/ResourceLinkList.tsx

+import { ResourceLink } from '@console/internal/components/utils';
+import './ResourceLinkList.scss';
+
+type ResourceLinkListProps = {


Not sure I understand what the concern is... you can compute the values with the ModelKind:

// calling file <ResourceLinkList model={TriggerBindingModel} ... /> // ResourceLinkList const title = model.labelPlural; const kind = referenceForModel(model);

andrewballantyne · 2020-04-13T17:49:12Z

...tend/packages/dev-console/src/components/pipelines/detail-page-tabs/EventListenerDetails.tsx

+const EventListenerDetails: React.FC<EventListenerDetailsProps> = ({ obj: eventListener }) => {
+  const { template, bindings } = (eventListener?.spec?.triggers || []).reduce(
+    (acc, trigger) => ({
+      bindings: [...acc.bindings, ...trigger.bindings.map((binding) => binding.name)],


Oh right, my example neglected that it was an array. Hmmm... the array of objects is there because bindings can have two types ClusterTriggerBinding and TriggerBinding... can you add this to the tech debt ticket? I think we need to swing back once we get an operator update as ClusterTriggerBinding doesn't exist on 0.10.8 😞

Task to update binding logic for 10.0.8 https://issues.redhat.com/browse/ODC-3557

...s/dev-console/src/components/pipelines/detail-page-tabs/pipeline-details/PipelineDetails.tsx

frontend/packages/dev-console/src/utils/pipeline-augment.ts

andrewballantyne · 2020-04-13T17:55:30Z

Few questions for you!

When Trigger Templates are shown, I think they should have the associated URL below them, at least that was the design. I know that piece of information is optional, but just checking

Oh crud! I forgot about this on my first review pass. Please add this @karthikjeeyar - the URL is awkward to get so we'll have to write another hook for this.

I'll see what I can write for this while you're asleep :)

andrewballantyne

Please focus on the spec and if it makes it optional. We need to start aligning our Kind definitions and our Code. It becomes unruly having to guess every time you write new code seeing optionals literally everywhere.

frontend/packages/dev-console/src/components/pipelines/utils/triggers.ts

andrewballantyne · 2020-04-14T18:57:29Z

...ages/dev-console/src/components/pipelines/resource-overview/TriggerTemplateResourceLink.scss

+.odc-trigger-template-list {
+  margin-bottom: var(--pf-global--spacer--lg);
+  &__url {
+    margin-left: calc(10 * var(--pf-global--BorderWidth--lg));


lol, what? I know we request for pf variables... but this is a bit of malicious compliance lol. I'll have to look at what you're trying to do lol.

This is to align the Url with the TT name

Ah indeed... we'll need to iterate on this solution heh... I'd prefer a px number over "10 border widths" lol.

This var(--pf-global--BorderWidth--lg) is in px, so calc(10* 3px) is resulting in 30px.

Change this to a pixel value. Using a variable incorrectly is not a good idea.

...ckages/dev-console/src/components/pipelines/resource-overview/TriggerTemplateResouceLink.tsx

frontend/packages/dev-console/src/components/pipelines/resource-overview/ResourceLinkList.tsx

...end/packages/dev-console/src/components/pipelineruns/detail-page-tabs/PipelineRunDetails.tsx

...kages/dev-console/src/components/pipelines/resource-overview/TriggerTemplateResourceLink.tsx

andrewballantyne · 2020-04-16T13:31:03Z

...ages/dev-console/src/components/pipelines/resource-overview/TriggerTemplateResourceLink.scss

+.odc-trigger-template-list {
+  margin-bottom: var(--pf-global--spacer--lg);
+  &__url {
+    margin-left: calc(10 * var(--pf-global--BorderWidth--lg));


Change this to a pixel value. Using a variable incorrectly is not a good idea.

…page

andrewballantyne

/lgtm

openshift-ci-robot · 2020-04-16T14:21:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, karthikjeeyar, serenamarie125

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/packages/dev-console/OWNERS~~ [andrewballantyne]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the component/dev-console Related to dev-console label Apr 9, 2020

openshift-ci-robot requested review from abhinandan13jan and vikram-raj April 9, 2020 21:44

openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 9, 2020

openshift-ci-robot assigned andrewballantyne Apr 9, 2020

karthikjeeyar force-pushed the trigger-pages branch from 5b3844f to 88a3b46 Compare April 10, 2020 07:06

andrewballantyne suggested changes Apr 13, 2020

View reviewed changes

karthikjeeyar force-pushed the trigger-pages branch 2 times, most recently from 6c2768c to 82834f2 Compare April 13, 2020 11:57

serenamarie125 suggested changes Apr 13, 2020

View reviewed changes

serenamarie125 approved these changes Apr 13, 2020

View reviewed changes

andrewballantyne reviewed Apr 13, 2020

View reviewed changes

andrewballantyne suggested changes Apr 13, 2020

View reviewed changes

andrewballantyne suggested changes Apr 14, 2020

View reviewed changes

karthikjeeyar force-pushed the trigger-pages branch 2 times, most recently from 03031f8 to 8e3cbf8 Compare April 16, 2020 11:35

andrewballantyne suggested changes Apr 16, 2020

View reviewed changes

Add pipeline trigger resource pages and related links in the details …

e4a999b

…page

karthikjeeyar force-pushed the trigger-pages branch from 8e3cbf8 to e4a999b Compare April 16, 2020 14:11

andrewballantyne approved these changes Apr 16, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 16, 2020

openshift-merge-robot merged commit 08c9ebf into openshift:master Apr 16, 2020

spadgett added this to the v4.5 milestone Apr 20, 2020

		@@ -10,7 +10,6 @@ import PipelineRunVisualization from '../PipelineRunVisualization';

		const mockPipelineRun =
		pipelineTestData[PipelineExampleNames.SIMPLE_PIPELINE].pipelineRuns[DataState.SUCCESS];

Add pipeline trigger resource pages and related links in details pages #5005

Add pipeline trigger resource pages and related links in details pages #5005

Conversation

karthikjeeyar commented Apr 9, 2020 • edited

karthikjeeyar commented Apr 9, 2020

karthikjeeyar commented Apr 9, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikjeeyar commented Apr 13, 2020

serenamarie125 left a comment

Choose a reason for hiding this comment

karthikjeeyar commented Apr 13, 2020

serenamarie125 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

andrewballantyne commented Apr 13, 2020

andrewballantyne 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

andrewballantyne left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 16, 2020

karthikjeeyar commented Apr 9, 2020 •

edited