Bug 1878700: Fix Add Pipeline Trigger for Pipeline Operator 1.1 #6612

jerolimov · 2020-09-14T11:38:12Z

Fixes:
https://issues.redhat.com/browse/ODC-4496
https://bugzilla.redhat.com/show_bug.cgi?id=1878700

Analysis / Root cause:
With the OpenShift Pipeline Operator 1.1 a new version of Pipeline Trigger CRDs was released. The new version was 0.6.

In 0.5.0 (we didn't see this version in the Operator) they deprecated {{spec.triggers[].bindings[].name}}.

In 0.6.0 (TP2 - 1.1 of the Operator) it has been removed in favour of deprecated {{spec.triggers[].bindings[].ref}}.

So we need to change the EventListener generation (source code) in a way we support OpenShift Pipeline Operator 1.0 and 1.1.

Solution Description:
The code checks the installed OpenShift Pipeline Operator version. If it finds the version 0.x or 1.0.x it uses the old EventListener format, and the new one format otherwise (also in the case the Operator version check fails).

Screen shots / Gifs for design review:
No visual change here.

Unit test coverage report:
Add new tests for the new getPipelineOperatorVersion method.

 PASS  packages/dev-console/src/components/pipelines/__tests__/pipeline-operator.spec.ts
  getPipelineOperatorVersion
    ✓ should fetch the ClusterServiceVersion from the api (1ms)
    ✓ should return the active ClusterServiceVersion if multiple returns (1ms)
    ✓ should fetch the latest (highest) ClusterServiceVersion from the api
    ✓ should return null if there is no ClusterServiceVersion available (1ms)
    ✓ should return null if there is no matching ClusterServiceVersion available

Test setup:

Add the OpenShift Pipeline Operator in the Version 1.0 and 1.1.
Create a new Pipeline
Add a trigger

Browser conformance:

Chrome
Firefox
Safari
Edge

openshift-ci-robot · 2020-09-14T11:38:22Z

@jerolimov: This pull request references Bugzilla bug 1878700, 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.6.0) matches configured target release for branch (4.6.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 1878700: Fix Pipeline Triggers for Pipeline Operator 1.1

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 · 2020-09-14T11:40:32Z

@jerolimov: This pull request references Bugzilla bug 1878700, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.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 1878700: Fix Pipeline Triggers for Pipeline Operator 1.1

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.

jerolimov · 2020-09-14T12:01:15Z

/retest

jerolimov · 2020-09-14T12:02:45Z

/retest

jerolimov · 2020-09-14T13:59:37Z

/retest

andrewballantyne · 2020-09-14T15:58:02Z

@jerolimov You have TS errors, I think it's causing the whole CI stack to fail:

ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/src/components/pipelines/utils/triggers.ts
(171,19): Property 'name' does not exist on type 'EventListenerKindBindingReference'.
ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/src/components/pipelines/utils/triggers.ts
(199,20): Property 'name' does not exist on type 'EventListenerKindBindingReference'.

jerolimov · 2020-09-14T18:27:01Z

You have TS errors, I think it's causing the whole CI stack to fail:

@andrewballantyne Thanks for that hint. :) Fixed the TS error. This fixes also an issue that the ClusterTriggerBinding name (or now ref) was not shown on the EventListener detail page. We talked about this issue already.

Previously it does not show the name for Pipeline Triggers which was created with 1.1.

I noticed that Pipeline Triggers which was created with 1.0 does not run in 1.1. But this is true for all Pipeline objects I think. The pipelines itself also doesn't work after the update. Since everything is Tech Preview and Alpha here I think this is "okayish"

jerolimov · 2020-09-14T22:44:37Z

/retest

jerolimov · 2020-09-15T07:56:53Z

/retest

jerolimov · 2020-09-15T12:50:53Z

/kind bug

rottencandy · 2020-09-15T16:07:52Z

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

+  ref: string;
+  // We also support older operators, so need to show & save the old field as well.
+  // https://github.com/tektoncd/triggers/pull/603/files
+  // https://github.com/tektoncd/triggers/releases/tag/v0.5.0 and
+  // https://github.com/tektoncd/triggers/releases/tag/v0.6.0
+  /** @deprecated use ref instead */
+  name?: string;


Wondering if we might be able to use keyof here, or would it become too verbose? Especially since name is deprecated.

Don't know what the solution looks like then. But the usage in triggers.ts works fine with name and at least VScode shows these deprecated hint. But I'm open to see and try an example here. /me open the TS keyof definition :)

jerolimov · 2020-09-15T16:36:07Z

The current solution uses the Tekton CRD version labels. Unfortunately these are not readable for all users / developers. Will change the solution and remove the WIP label after that.

openshift-ci-robot · 2020-09-16T11:43:32Z

@jerolimov: This pull request references Bugzilla bug 1878700, which is valid.

3 validation(s) were run on this bug

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

In response to this:

[WIP] Bug 1878700: Fix Add Pipeline Trigger for Pipeline Operator 1.1

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 · 2020-09-16T14:10:57Z

...tend/packages/dev-console/src/components/pipelines/utils/__tests__/pipeline-operator.spec.ts

+    expect(k8sList).toHaveBeenCalledTimes(1);
+  });
+
+  it('should return null if there is no matching ClusterServiceVersion available', async () => {


Could you write one more test for valid items but not including a Pipeline Operator? I think you covered both sides (none and mixed), but I think there is still valid test possible here for just invalid ones.

added new test, it should return null if there is no pipeline operator at all

andrewballantyne · 2020-09-16T15:45:54Z

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

+      csv.status?.phase === ClusterServiceVersionPhase.CSVPhaseSucceeded,
+  );
+  const versions = matchingCSVs.map((csv) => parse(csv.spec.version)).filter(Boolean);
+  // Orders from small (oldest) to highest (newest) version


I'm confused by this comment vs the tests.

Suggested change

// Orders from small (oldest) to highest (newest) version

// Orders versions from latest to earliest

You're grabbing the last one, the earliest version based on your tests.

The order after this versions.sort(compare) is 1.0, 1.1, 2.0, ... And then I selected the last one (to get the newest version). The comment was only for the versions.sort line. I will update that comment so that it matches both lines or do some code change. don't know yet.

Ah I was confused by your test... I didn't read the phase value. You're right. Apologizes -- ignore this thread.

andrewballantyne · 2020-09-17T15:23:00Z

/lgtm

/assign @rohitkrai03 @christianvogt

christianvogt · 2020-09-17T15:28:51Z

/approve

openshift-ci-robot · 2020-09-17T15:29:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, jerolimov

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/OWNERS~~ [christianvogt]

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

jerolimov · 2020-09-17T16:03:07Z

/retest
/woof

openshift-ci-robot · 2020-09-17T16:03:09Z

@jerolimov:

In response to this:

/retest
/woof

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-bot · 2020-09-17T19:33:29Z

/retest

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

openshift-bot · 2020-09-17T21:05:01Z

/retest

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

openshift-bot · 2020-09-17T22:09:01Z

/retest

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

openshift-bot · 2020-09-17T23:40:21Z

/retest

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

openshift-bot · 2020-09-18T01:37:08Z

/retest

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

openshift-ci-robot · 2020-09-18T05:50:36Z

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

openshift/console#6612

Bugzilla bug 1878700 has been moved to the MODIFIED state.

In response to this:

Bug 1878700: Fix Add Pipeline Trigger for Pipeline Operator 1.1

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.

jerolimov · 2020-09-18T07:02:04Z

/cherry-pick release-4.5

openshift-cherrypick-robot · 2020-09-18T07:02:15Z

@jerolimov: #6612 failed to apply on top of branch "release-4.5":

Applying: Check Pipeline Operator CSV version to use matching Tekton api
Using index info to reconstruct a base tree...
M	frontend/package.json
M	frontend/packages/dev-console/src/components/pipelines/modals/triggers/resource-utils.ts
M	frontend/packages/dev-console/src/components/pipelines/modals/triggers/submit-utils.ts
M	frontend/packages/dev-console/src/components/pipelines/utils/triggers.ts
M	frontend/public/module/k8s/cluster-settings.ts
M	frontend/yarn.lock
Falling back to patching base and 3-way merge...
Auto-merging frontend/yarn.lock
Auto-merging frontend/public/module/k8s/cluster-settings.ts
CONFLICT (content): Merge conflict in frontend/public/module/k8s/cluster-settings.ts
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/modals/triggers/submit-utils.ts
Auto-merging frontend/packages/dev-console/src/components/pipelines/modals/triggers/resource-utils.ts
Auto-merging frontend/package.json
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Check Pipeline Operator CSV version to use matching Tekton api
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.5

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.

jerolimov · 2020-09-18T11:40:27Z

Because automatically check picking fails I created PR #6675 manually.

openshift-ci-robot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Sep 14, 2020

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/dev-console Related to dev-console labels Sep 14, 2020

openshift-ci-robot requested review from abhi-kn and andrewballantyne September 14, 2020 11:38

jerolimov changed the title ~~Bug 1878700: Fix Pipeline Triggers for Pipeline Operator 1.1~~ Bug 1878700: Fix Add Pipeline Trigger for Pipeline Operator 1.1 Sep 14, 2020

jerolimov force-pushed the odc-4496 branch 2 times, most recently from 2baac48 to 1fb87f6 Compare September 14, 2020 18:21

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 15, 2020

rottencandy reviewed Sep 15, 2020

View reviewed changes

jerolimov changed the title ~~Bug 1878700: Fix Add Pipeline Trigger for Pipeline Operator 1.1~~ [WIP] Bug 1878700: Fix Add Pipeline Trigger for Pipeline Operator 1.1 Sep 15, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 15, 2020

jerolimov force-pushed the odc-4496 branch 3 times, most recently from 39ab213 to 2468828 Compare September 16, 2020 11:40

jerolimov force-pushed the odc-4496 branch from 2468828 to a702d47 Compare September 16, 2020 11:59

jerolimov changed the title ~~[WIP] Bug 1878700: Fix Add Pipeline Trigger for Pipeline Operator 1.1~~ Bug 1878700: Fix Add Pipeline Trigger for Pipeline Operator 1.1 Sep 16, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2020

jerolimov force-pushed the odc-4496 branch from 3da41e1 to 4539f49 Compare September 16, 2020 13:25

andrewballantyne reviewed Sep 16, 2020

View reviewed changes

jerolimov force-pushed the odc-4496 branch from 4539f49 to 3fb6a1a Compare September 16, 2020 14:21

Check Pipeline Operator CSV version to use matching Tekton api

b73a8d3

jerolimov force-pushed the odc-4496 branch from 3fb6a1a to b73a8d3 Compare September 16, 2020 14:22

andrewballantyne reviewed Sep 16, 2020

View reviewed changes

andrewballantyne approved these changes Sep 17, 2020

View reviewed changes

openshift-ci-robot assigned christianvogt, rohitkrai03 and andrewballantyne Sep 17, 2020

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

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

openshift-merge-robot merged commit e7c24ad into openshift:master Sep 18, 2020

jerolimov mentioned this pull request Sep 18, 2020

[release-4.5] Bug 1880376: Fix Add Pipeline Trigger for Pipeline Operator 1.1 #6675

Merged

jerolimov mentioned this pull request Sep 18, 2020

Bug 1879089: Use new parameter prefix (tt.param.) for Tekton Trigger version 0.6+ #6627

Merged

4 tasks

spadgett added this to the v4.6 milestone Sep 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1878700: Fix Add Pipeline Trigger for Pipeline Operator 1.1 #6612

Bug 1878700: Fix Add Pipeline Trigger for Pipeline Operator 1.1 #6612

jerolimov commented Sep 14, 2020 •

edited

openshift-ci-robot commented Sep 14, 2020

openshift-ci-robot commented Sep 14, 2020

jerolimov commented Sep 14, 2020

jerolimov commented Sep 14, 2020

jerolimov commented Sep 14, 2020

andrewballantyne commented Sep 14, 2020

jerolimov commented Sep 14, 2020

jerolimov commented Sep 14, 2020

jerolimov commented Sep 15, 2020

jerolimov commented Sep 15, 2020

rottencandy Sep 15, 2020

jerolimov Sep 15, 2020

jerolimov commented Sep 15, 2020

openshift-ci-robot commented Sep 16, 2020

andrewballantyne Sep 16, 2020

jerolimov Sep 16, 2020

andrewballantyne Sep 16, 2020

jerolimov Sep 17, 2020

andrewballantyne Sep 17, 2020

andrewballantyne commented Sep 17, 2020

christianvogt commented Sep 17, 2020

openshift-ci-robot commented Sep 17, 2020

jerolimov commented Sep 17, 2020

openshift-ci-robot commented Sep 17, 2020

openshift-bot commented Sep 17, 2020

openshift-bot commented Sep 17, 2020

openshift-bot commented Sep 17, 2020

openshift-bot commented Sep 17, 2020

openshift-bot commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

jerolimov commented Sep 18, 2020

openshift-cherrypick-robot commented Sep 18, 2020

jerolimov commented Sep 18, 2020

	// Orders from small (oldest) to highest (newest) version
	// Orders versions from latest to earliest

Bug 1878700: Fix Add Pipeline Trigger for Pipeline Operator 1.1 #6612

Bug 1878700: Fix Add Pipeline Trigger for Pipeline Operator 1.1 #6612

Conversation

jerolimov commented Sep 14, 2020 • edited

openshift-ci-robot commented Sep 14, 2020

openshift-ci-robot commented Sep 14, 2020

jerolimov commented Sep 14, 2020

jerolimov commented Sep 14, 2020

jerolimov commented Sep 14, 2020

andrewballantyne commented Sep 14, 2020

jerolimov commented Sep 14, 2020

jerolimov commented Sep 14, 2020

jerolimov commented Sep 15, 2020

jerolimov commented Sep 15, 2020

rottencandy Sep 15, 2020

Choose a reason for hiding this comment

jerolimov Sep 15, 2020

Choose a reason for hiding this comment

jerolimov commented Sep 15, 2020

openshift-ci-robot commented Sep 16, 2020

andrewballantyne Sep 16, 2020

Choose a reason for hiding this comment

jerolimov Sep 16, 2020

Choose a reason for hiding this comment

andrewballantyne Sep 16, 2020

Choose a reason for hiding this comment

jerolimov Sep 17, 2020

Choose a reason for hiding this comment

andrewballantyne Sep 17, 2020

Choose a reason for hiding this comment

andrewballantyne commented Sep 17, 2020

christianvogt commented Sep 17, 2020

openshift-ci-robot commented Sep 17, 2020

jerolimov commented Sep 17, 2020

openshift-ci-robot commented Sep 17, 2020

openshift-bot commented Sep 17, 2020

openshift-bot commented Sep 17, 2020

openshift-bot commented Sep 17, 2020

openshift-bot commented Sep 17, 2020

openshift-bot commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

jerolimov commented Sep 18, 2020

openshift-cherrypick-robot commented Sep 18, 2020

jerolimov commented Sep 18, 2020

jerolimov commented Sep 14, 2020 •

edited