Bug 1918639: add missing trigger types and update the optional fields #7889

karthikjeeyar · 2021-01-21T09:30:17Z

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

Analysis / Root cause:
Event listeners has an optional triggerRef field which is not compatible in UI code causes the UI to crash

Solution Description:
Add the missing trigger types such as triggerRef and update the checks for optional fields.

Screen shots / Gifs for design review:
Eventlistener page:

Test setup:

Create an eventlistener with the yaml

apiVersion: triggers.tekton.dev/v1alpha1
kind: EventListener
metadata:
  name: vote-app
spec:
  serviceAccountName: pipeline
  triggers:
    - triggerRef: vote-app

Unit test cases:

EventListener Details
✓ should render eventlistener details page (25ms)
✓ should not render EventListenerTriggers section if the trigger contains binding ref (4ms)
✓ should not render EventListenerTriggers section if triggers contains triggerRef (3ms)

Browser conformance:

Chrome
Firefox
Safari
Edge

karthikjeeyar · 2021-01-21T09:31:45Z

/kind bug
/assign @andrewballantyne

openshift-ci-robot · 2021-01-21T09:42:32Z

@karthikjeeyar: This pull request references Bugzilla bug 1918639, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1918639: add missing trigger types and update the optional fields

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

andrewballantyne · 2021-01-21T16:24:50Z

This is a feature implementation. I discussed with @karthikjeeyar for us to focus on the fix here as a means not to crash the UI if they used the feature we don't support. Updating types are likely to stay as they are not user-facing and will help us not whitescreen.

openshift-ci-robot · 2021-01-22T12:54:06Z

@karthikjeeyar: This pull request references Bugzilla bug 1918639, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1918639: add missing trigger types and update the optional fields

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

karthikjeeyar · 2021-01-22T12:56:59Z

@andrewballantyne I've updated the code to handle the UI crashes, conditional rendering of the supported resource-links in the evenlistener details page.

...packages/pipelines-plugin/src/components/pipelines/detail-page-tabs/EventListenerDetails.tsx

andrewballantyne · 2021-01-25T21:21:12Z

...ackages/pipelines-plugin/src/components/pipelines/detail-page-tabs/EventListenerTriggers.tsx

@@ -24,7 +24,7 @@ const EventListenerTriggers: React.FC<EventListenerTriggersProps> = ({ namespace
      <dd>
        {triggers.map((trigger) => {
          const triggerTemplateKind = referenceForModel(TriggerTemplateModel);
-          const triggerTemplateName = trigger.template.name;
+          const triggerTemplateName = trigger.template?.name;


If you don't have a template name, I think we need to return null to just kill this node. No sense in trying to craft a broken block.

filtering triggers based on template?.name and returning null early on in the component, so that <dt>Tiggers <dt> is also not rendered.

I think there was a misunderstanding here.

You made this trigger.template?.name optional, but it will fail to render properly if the triggerTemplateName is undefined. So we have to do something about the iterations of this map that do NOT have a triggerTemplateName.

My recommendation is you return null and drop this iteration from rendering a broken link. I'm open to other suggestions, but we can't just leave it.

You made this trigger.template?.name optional, but it will fail to render properly if the triggerTemplateName is undefined. So we have to do something about the iterations of this map that do NOT have a triggerTemplateName.

I am filtering out the tirggers based on tempate.name on the top of this component now. so the map will only contain the set of triggers that consists of template.name.

...nes-plugin/src/components/pipelines/detail-page-tabs/__tests__/EventListenerDetails.spec.tsx

andrewballantyne · 2021-01-25T21:33:52Z

frontend/packages/pipelines-plugin/src/components/pipelines/resource-types/triggers.ts

  // Ref is used since Tekton Triggers 0.5 (part of OpenShift Pipeline Operator 1.1)
-  ref: string;
+  // Mutually exclusive with Name


deleted my comment (see comment history if you care to read it) There was type compilation issues I didn't see before, probably not worth the hassle.

Honestly we probably won't keep this for much longer. Perhaps part of the new parity support in 4.8 we remove the legacy type.

frontend/packages/pipelines-plugin/src/components/pipelines/resource-types/triggers.ts

frontend/packages/pipelines-plugin/src/components/pipelines/utils/triggers.ts

andrewballantyne · 2021-01-28T18:19:28Z

...ackages/pipelines-plugin/src/components/pipelines/detail-page-tabs/EventListenerTriggers.tsx

@@ -24,7 +24,7 @@ const EventListenerTriggers: React.FC<EventListenerTriggersProps> = ({ namespace
      <dd>
        {triggers.map((trigger) => {
          const triggerTemplateKind = referenceForModel(TriggerTemplateModel);
-          const triggerTemplateName = trigger.template.name;
+          const triggerTemplateName = trigger.template?.name;


I think there was a misunderstanding here.

You made this trigger.template?.name optional, but it will fail to render properly if the triggerTemplateName is undefined. So we have to do something about the iterations of this map that do NOT have a triggerTemplateName.

My recommendation is you return null and drop this iteration from rendering a broken link. I'm open to other suggestions, but we can't just leave it.

jerolimov · 2021-01-29T12:05:08Z

/retest

jerolimov · 2021-01-29T12:07:43Z

/cc

karthikjeeyar · 2021-01-29T13:03:16Z

You made this trigger.template?.name optional, but it will fail to render properly if the triggerTemplateName is undefined. So we have to do something about the iterations of this map that do NOT have a triggerTemplateName.

@andrewballantyne I removed optional chaining as I have checks in the top of the component to early return null, if the template.name is not available in the triggers.

andrewballantyne

Thanks @karthikjeeyar Looks good 👍

andrewballantyne · 2021-01-29T15:50:47Z

/retest

spadgett · 2021-01-29T17:13:58Z

/override ci/prow/ceph-storage-plugin

The ceph-storage-plugin job should only have run on PRs with changes to frontend/packages/ceph-storage-plugin. It failed due to an unrelated CI config change. Overriding status on this PR.

openshift-ci-robot · 2021-01-29T17:14:21Z

@spadgett: Overrode contexts on behalf of spadgett: ci/prow/ceph-storage-plugin

In response to this:

/override ci/prow/ceph-storage-plugin

The ceph-storage-plugin job should only have run on PRs with changes to frontend/packages/ceph-storage-plugin. It failed due to an unrelated CI config change. Overriding status on this PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

andrewballantyne · 2021-01-29T17:45:47Z

/retest

spadgett · 2021-01-29T17:54:42Z

/override ci/prow/ceph-storage-plugin

openshift-ci-robot · 2021-01-29T17:55:01Z

@spadgett: Overrode contexts on behalf of spadgett: ci/prow/ceph-storage-plugin

In response to this:

/override ci/prow/ceph-storage-plugin

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

...packages/pipelines-plugin/src/components/pipelines/detail-page-tabs/EventListenerDetails.tsx

invincibleJai · 2021-02-01T10:37:56Z

/lgtm

Verified locally, works as expected

openshift-ci-robot · 2021-02-01T10:38:14Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/pipelines-plugin/OWNERS~~ [andrewballantyne,invincibleJai]

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

openshift-bot · 2021-02-01T13:04:42Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-02-01T13:30:44Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2021-02-01T19:04:38Z

@karthikjeeyar: All pull requests linked via external trackers have merged:

openshift/console#7889

Bugzilla bug 1918639 has been moved to the MODIFIED state.

In response to this:

Bug 1918639: add missing trigger types and update the optional fields

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

andrewballantyne · 2021-02-01T19:13:44Z

/cherry-pick release-4.6

openshift-cherrypick-robot · 2021-02-01T19:14:23Z

@andrewballantyne: #7889 failed to apply on top of branch "release-4.6":

Applying: add missing trigger types and update the optional fields
Using index info to reconstruct a base tree...
A	frontend/packages/pipelines-plugin/src/components/pipelines/detail-page-tabs/EventListenerDetails.tsx
A	frontend/packages/pipelines-plugin/src/components/pipelines/detail-page-tabs/EventListenerTriggers.tsx
A	frontend/packages/pipelines-plugin/src/components/pipelines/resource-types/triggers.ts
A	frontend/packages/pipelines-plugin/src/components/pipelines/utils/triggers.ts
Falling back to patching base and 3-way merge...
Auto-merging frontend/packages/dev-console/src/components/pipelines/utils/triggers.ts
CONFLICT (content): Merge conflict in frontend/packages/dev-console/src/components/pipelines/utils/triggers.ts
Auto-merging frontend/packages/dev-console/src/components/pipelines/detail-page-tabs/EventListenerTriggers.tsx
CONFLICT (content): Merge conflict in frontend/packages/dev-console/src/components/pipelines/detail-page-tabs/EventListenerTriggers.tsx
Auto-merging frontend/packages/dev-console/src/components/pipelines/detail-page-tabs/EventListenerDetails.tsx
CONFLICT (content): Merge conflict in frontend/packages/dev-console/src/components/pipelines/detail-page-tabs/EventListenerDetails.tsx
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 add missing trigger types and update the optional fields
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot requested review from nemesis09 and rohitkrai03 January 21, 2021 09:30

openshift-ci-robot assigned andrewballantyne Jan 21, 2021

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 21, 2021

karthikjeeyar changed the title ~~add missing trigger types and update the optional fields~~ Bug 1918639: add missing trigger types and update the optional fields Jan 21, 2021

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jan 21, 2021

karthikjeeyar force-pushed the fix-trigger-ref branch from 64651a1 to 9e0d44b Compare January 21, 2021 09:43

karthikjeeyar force-pushed the fix-trigger-ref branch 2 times, most recently from 186df13 to e69a9e3 Compare January 22, 2021 12:47

karthikjeeyar force-pushed the fix-trigger-ref branch from e69a9e3 to c7d5558 Compare January 22, 2021 13:16

andrewballantyne suggested changes Jan 25, 2021

View reviewed changes

karthikjeeyar force-pushed the fix-trigger-ref branch from c7d5558 to af682bd Compare January 28, 2021 16:10

andrewballantyne suggested changes Jan 28, 2021

View reviewed changes

openshift-ci-robot requested a review from jerolimov January 29, 2021 12:07

karthikjeeyar force-pushed the fix-trigger-ref branch from af682bd to b861c72 Compare January 29, 2021 12:58

andrewballantyne approved these changes Jan 29, 2021

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 29, 2021

invincibleJai reviewed Feb 1, 2021

View reviewed changes

...packages/pipelines-plugin/src/components/pipelines/detail-page-tabs/EventListenerDetails.tsx Outdated Show resolved Hide resolved

add missing trigger types and update the optional fields

e923fc6

karthikjeeyar force-pushed the fix-trigger-ref branch from b861c72 to e923fc6 Compare February 1, 2021 10:37

openshift-ci-robot assigned invincibleJai Feb 1, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2021

openshift-merge-robot merged commit 886ea30 into openshift:master Feb 1, 2021

karthikjeeyar mentioned this pull request Feb 2, 2021

Bug 1923939: add missing trigger types and update the optional fields - 4.6 #8015

Merged

spadgett added this to the v4.7 milestone Feb 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1918639: add missing trigger types and update the optional fields #7889

Bug 1918639: add missing trigger types and update the optional fields #7889

karthikjeeyar commented Jan 21, 2021 •

edited

karthikjeeyar commented Jan 21, 2021

openshift-ci-robot commented Jan 21, 2021

andrewballantyne commented Jan 21, 2021

openshift-ci-robot commented Jan 22, 2021

karthikjeeyar commented Jan 22, 2021 •

edited

andrewballantyne Jan 25, 2021

karthikjeeyar Jan 28, 2021 •

edited

andrewballantyne Jan 28, 2021

karthikjeeyar Jan 29, 2021

andrewballantyne Jan 25, 2021 •

edited

andrewballantyne Jan 28, 2021

jerolimov commented Jan 29, 2021

jerolimov commented Jan 29, 2021

karthikjeeyar commented Jan 29, 2021 •

edited

andrewballantyne left a comment

andrewballantyne commented Jan 29, 2021

spadgett commented Jan 29, 2021

openshift-ci-robot commented Jan 29, 2021

andrewballantyne commented Jan 29, 2021

spadgett commented Jan 29, 2021

openshift-ci-robot commented Jan 29, 2021

invincibleJai commented Feb 1, 2021

openshift-ci-robot commented Feb 1, 2021

openshift-bot commented Feb 1, 2021

openshift-bot commented Feb 1, 2021

openshift-ci-robot commented Feb 1, 2021

andrewballantyne commented Feb 1, 2021

openshift-cherrypick-robot commented Feb 1, 2021

Bug 1918639: add missing trigger types and update the optional fields #7889

Bug 1918639: add missing trigger types and update the optional fields #7889

Conversation

karthikjeeyar commented Jan 21, 2021 • edited

karthikjeeyar commented Jan 21, 2021

openshift-ci-robot commented Jan 21, 2021

andrewballantyne commented Jan 21, 2021

openshift-ci-robot commented Jan 22, 2021

karthikjeeyar commented Jan 22, 2021 • edited

andrewballantyne Jan 25, 2021

Choose a reason for hiding this comment

karthikjeeyar Jan 28, 2021 • edited

Choose a reason for hiding this comment

andrewballantyne Jan 28, 2021

Choose a reason for hiding this comment

karthikjeeyar Jan 29, 2021

Choose a reason for hiding this comment

andrewballantyne Jan 25, 2021 • edited

Choose a reason for hiding this comment

andrewballantyne Jan 28, 2021

Choose a reason for hiding this comment

jerolimov commented Jan 29, 2021

jerolimov commented Jan 29, 2021

karthikjeeyar commented Jan 29, 2021 • edited

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne commented Jan 29, 2021

spadgett commented Jan 29, 2021

openshift-ci-robot commented Jan 29, 2021

andrewballantyne commented Jan 29, 2021

spadgett commented Jan 29, 2021

openshift-ci-robot commented Jan 29, 2021

invincibleJai commented Feb 1, 2021

openshift-ci-robot commented Feb 1, 2021

openshift-bot commented Feb 1, 2021

openshift-bot commented Feb 1, 2021

openshift-ci-robot commented Feb 1, 2021

andrewballantyne commented Feb 1, 2021

openshift-cherrypick-robot commented Feb 1, 2021

karthikjeeyar commented Jan 21, 2021 •

edited

karthikjeeyar commented Jan 22, 2021 •

edited

karthikjeeyar Jan 28, 2021 •

edited

andrewballantyne Jan 25, 2021 •

edited

karthikjeeyar commented Jan 29, 2021 •

edited