Bug 1763725: fix(tekton-migration): Fix trigger type and resource issue #3086

abhinandan13jan · 2019-10-24T10:25:43Z

Fixes https://jira.coreos.com/browse/ODC-2064

…e start

andrewballantyne

My review of the code - I'll have to do a deeper dive into the yaml changes before I can give more feedback on that.

andrewballantyne · 2019-10-24T14:43:05Z

frontend/packages/dev-console/src/components/pipelineruns/PipelineRunRow.tsx

-      <TableData className={tableColumnClasses[5]}>
-        {obj.spec && obj.spec.trigger && obj.spec.trigger.type ? obj.spec.trigger.type : '-'}
-      </TableData>
+      <TableData className={tableColumnClasses[5]}>-</TableData>


Is there any point in having the trigger column when it's no longer a valid field?

would require UX intervention

@abhinandan13jan Don't wait on me to say go talk to UX :) Reach out to UX and discuss this at your earliest convenience. Having a dead column helps no one.

I think it's fairly obvious that we need to remove this column now. Inform UX in case they want to substitute or there's an alternative. But right now it will never show any data and therefore it has no value with the current change.

I have removed the column, cascading the same to UX

andrewballantyne · 2019-10-24T14:44:40Z

frontend/packages/dev-console/src/utils/pipeline-actions.tsx

+  const resources = [];
+  const runResources = _.get(latestRun, ['spec', 'resources'], []);
+  runResources.forEach((resource) =>
+    resources.push({ name: resource.name, resourceRef: resource.resourceRef }),
+  );


const runResources = _.get(latestRun, ['spec', 'resources'], []); const resources = runResources.map((resource) => ( { name: resource.name, resourceRef: resource.resourceRef } ));

No sense in reinventing map.

Updated this

andrewballantyne · 2019-10-24T14:47:42Z

frontend/packages/dev-console/src/utils/pipeline-actions.tsx

-      resources:
-        latestRun && latestRun.spec && latestRun.spec.resources
-          ? latestRun.spec.resources
-          : pipeline && pipeline.spec && pipeline.spec.resources


You no longer have the fallback - was this intentional? latestRun is optional
https://github.com/openshift/console/pull/3086/files#diff-25f91b2db21c7ce21bcebedf9f18bb23R19

that's why the above lines, if no 'latestrun' and no resources, this field would be []

Sorry, perhaps I am misunderstanding - where are you looking into pipeline.spec.resources now? There are two fallbacks here... latestRun.spec.resources falls back into pipeline.spec.resources falls back into []. You're not doing this anymore as far as I can see - you just do latestRun and [].

Yes, we should not be using 'pipeline.specs' any longer as it would simply not work and fail the validation. For a fallback a [] is used instead now.

andrewballantyne · 2019-10-24T15:10:50Z

/assign

andrewballantyne · 2019-10-25T15:40:03Z

Looks like you missed the StartPipelineModal trigger value (not sure how you were able to start them in your gif).
Ref: frontend/packages/dev-console/src/components/pipelines/pipeline-form/StartPipelineModal.tsx

andrewballantyne · 2019-10-25T16:34:37Z

We should also look to remove the base.yaml as well as it contains incompatible webhook that is serving up 0.3.1. It was causing the error around needing trigger. Let's remove it so we don't have an accidental re-triggering of this problem.

From slack:

https://github.com/openshift/console/blob/master/frontend/packages/dev-console/yamls/pipelines/base.yaml shouldn't be there if you install openshift-pipelines using the operator

abhinandan13jan · 2019-10-29T12:03:16Z

Looks like you missed the StartPipelineModal trigger value (not sure how you were able to start them in your gif).
Ref: frontend/packages/dev-console/src/components/pipelines/pipeline-form/StartPipelineModal.tsx

@andrewballantyne Thanks for pointing that out. I have moved out that block.
Even without those changes it works as we are finally using getPipelineRunData(pipeline-action.ts) function for creating a new PipelineRun resource with the correct specs.

Also, I have removed the base.yaml so that it is not added to the cluster by mistake.

We must be looking for other pieces to improve upon with regards to the migration, for the time being this is the minimum that must go in to unblock other stories around Tekton. It's already a XXL PR 😄

openshift-ci-robot · 2019-10-29T12:07:08Z

@abhinandan13jan: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-console-olm	`5e5a498`	link	`/test e2e-aws-console-olm`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your 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. I understand the commands that are listed here.

abhinandan13jan · 2019-10-29T12:09:45Z

/test e2e-aws-console-olm
/test e2e-gcp-console

andrewballantyne · 2019-10-29T12:17:53Z

frontend/packages/dev-console/src/components/pipelineruns/PipelineRunRow.tsx

@@ -47,9 +47,6 @@ const PipelineRunRow: React.FC<PipelineRunRowProps> = ({ obj, index, key, style
      </TableData>
      <TableData className={tableColumnClasses[4]}>-</TableData>


Wow, there is another column here doing nothing.

It's the duration field, which should come as a backend response. It is blank from the very beginning

Can you log a bug for us to address it later. We'll investigate with UX to see if there was any intent here.

andrewballantyne · 2019-10-29T12:20:09Z

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

@@ -94,3 +94,369 @@ export const testData: PipelineAugmentData[] = [
    },
  },
 ];
+
+export enum DataState {


Conflict resolution problem? This should have been moved to dev-console/src/test/pipeline-data.ts in one of my last PRs that were merged yesterday.

Did you add something to the lines below?

nopes, I just accepted incoming changes

Well, if you didn't change anything, just remove all the "added" lines here. Not sure how you got them back :) (are you on latest?)

I verified, and all the code is using the ones from pipeline-data so these lines are just dead code.

yup, ok. I m removing them from pipeline-augment-test-data

…e start

andrewballantyne · 2019-10-29T13:14:43Z

/lgtm

We'll need to see what we can do to get a z-stream fix for 4.2

vdemeester

/lgtm

christianvogt · 2019-10-29T15:26:06Z

/approve

openshift-ci-robot · 2019-10-29T15:26:42Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

andrewballantyne · 2019-11-01T17:50:16Z

/retitle Bug 1763725: fix(tekton-migration): Fix trigger type and resource issue

REF: https://bugzilla.redhat.com/show_bug.cgi?id=1763725

openshift-ci-robot · 2019-11-01T17:50:21Z

@abhinandan13jan: All pull requests linked via external trackers have merged. Bugzilla bug 1763725 has been moved to the MODIFIED state.

In response to this:

Bug 1763725: fix(tekton-migration): Fix trigger type and resource issue

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 · 2019-11-04T15:05:01Z

/cherrypick release-4.2

QE Verification was just completed, cherry picking to 4.2.

openshift-cherrypick-robot · 2019-11-04T15:05:13Z

@andrewballantyne: #3086 failed to apply on top of branch "release-4.2":

Using index info to reconstruct a base tree...
M	frontend/packages/dev-console/src/components/pipelineruns/PipelineRunRow.tsx
A	frontend/packages/dev-console/src/test/pipeline-data.ts
M	frontend/packages/dev-console/src/utils/pipeline-actions.tsx
M	frontend/packages/dev-console/src/utils/pipeline-augment.ts
Falling back to patching base and 3-way merge...
Removing frontend/packages/dev-console/yamls/pipelines/base.yaml
Auto-merging frontend/packages/dev-console/src/utils/pipeline-augment.ts
CONFLICT (content): Merge conflict in frontend/packages/dev-console/src/utils/pipeline-augment.ts
Auto-merging frontend/packages/dev-console/src/utils/pipeline-actions.tsx
CONFLICT (modify/delete): frontend/packages/dev-console/src/test/pipeline-data.ts deleted in HEAD and modified in fix(tekton-migration): Fix trigger type and resource issue in Pipeline start. Version fix(tekton-migration): Fix trigger type and resource issue in Pipeline start of frontend/packages/dev-console/src/test/pipeline-data.ts left in tree.
Auto-merging frontend/packages/dev-console/src/components/pipelineruns/PipelineRunRow.tsx
error: Failed to merge in the changes.
Patch failed at 0001 fix(tekton-migration): Fix trigger type and resource issue in Pipeline start

In response to this:

/cherrypick release-4.2

QE Verification was just completed, cherry picking to 4.2.

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 · 2019-11-04T15:15:16Z

~~/bugzilla refresh~~

Was seeing if missed a step for it to be linked.

openshift-ci-robot · 2019-11-04T15:15:18Z

@andrewballantyne: Bugzilla bug 1763725 is in an unrecognized state (VERIFIED) and will not be moved to the MODIFIED state.

In response to this:

/bugzilla refresh

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 added component/dev-console Related to dev-console size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 24, 2019

openshift-ci-robot requested review from rohitkrai03 and vikram-raj October 24, 2019 10:26

andrewballantyne suggested changes Oct 24, 2019

View reviewed changes

openshift-ci-robot assigned andrewballantyne Oct 24, 2019

abhinandan13jan force-pushed the tekton-migration branch from 5e5a498 to 62898d9 Compare October 29, 2019 11:57

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 29, 2019

andrewballantyne suggested changes Oct 29, 2019

View reviewed changes

fix(tekton-migration): Fix trigger type and resource issue in Pipelin…

6065128

…e start

abhinandan13jan force-pushed the tekton-migration branch from 62898d9 to 6065128 Compare October 29, 2019 13:10

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2019

vdemeester approved these changes Oct 29, 2019

View reviewed changes

openshift-ci-robot assigned vdemeester Oct 29, 2019

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

openshift-merge-robot merged commit 5685539 into openshift:master Oct 29, 2019

spadgett added this to the v4.3 milestone Oct 31, 2019

openshift-ci-robot changed the title ~~fix(tekton-migration): Fix trigger type and resource issue in Pipelin…~~ Bug 1763725: fix(tekton-migration): Fix trigger type and resource issue Nov 1, 2019

andrewballantyne mentioned this pull request Nov 4, 2019

[release-4.2] Bug 1767961: Client Fix for Pipeline Operator 0.7.0 #3220

Merged

		@@ -47,9 +47,6 @@ const PipelineRunRow: React.FC<PipelineRunRowProps> = ({ obj, index, key, style
		</TableData>
		<TableData className={tableColumnClasses[4]}>-</TableData>

Bug 1763725: fix(tekton-migration): Fix trigger type and resource issue #3086

Bug 1763725: fix(tekton-migration): Fix trigger type and resource issue #3086

Conversation

abhinandan13jan commented Oct 24, 2019

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

christianvogt Oct 25, 2019 • edited Loading

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 Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewballantyne commented Oct 24, 2019

andrewballantyne commented Oct 25, 2019 • edited Loading

andrewballantyne commented Oct 25, 2019

abhinandan13jan commented Oct 29, 2019 • edited Loading

openshift-ci-robot commented Oct 29, 2019 • edited Loading

abhinandan13jan commented Oct 29, 2019

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

abhinandan13jan Oct 29, 2019 • edited Loading

Choose a reason for hiding this comment

andrewballantyne commented Oct 29, 2019

vdemeester left a comment

Choose a reason for hiding this comment

christianvogt commented Oct 29, 2019

openshift-ci-robot commented Oct 29, 2019

andrewballantyne commented Nov 1, 2019

openshift-ci-robot commented Nov 1, 2019

andrewballantyne commented Nov 4, 2019

openshift-cherrypick-robot commented Nov 4, 2019

andrewballantyne commented Nov 4, 2019 • edited Loading

openshift-ci-robot commented Nov 4, 2019

christianvogt Oct 25, 2019 •

edited

Loading

andrewballantyne Oct 25, 2019 •

edited

Loading

andrewballantyne commented Oct 25, 2019 •

edited

Loading

abhinandan13jan commented Oct 29, 2019 •

edited

Loading

openshift-ci-robot commented Oct 29, 2019 •

edited

Loading

abhinandan13jan Oct 29, 2019 •

edited

Loading

andrewballantyne commented Nov 4, 2019 •

edited

Loading