update pipelinerun graph to show past pipeline structure #6960

karthikjeeyar · 2020-10-20T11:36:09Z

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

Description:
As a user, I would like to be able to view my past Pipeline Run visualization graphs without them being impacted by my current Pipeline status.

Problem:
Pipeline visualization component was always fetching the tasks from the current pipeline.

Solution:
Visualization component now uses the tasks from the pipelinerun.status.pipelineSpec field, which holds the reference of the pipeline at the time of execution. As a fallback, we still read from the pipeline if this pipelinerun.status.pipelineSpec field is not available.

Screenshots:

Test cases:

cc: @siamaksade @andrewballantyne
/kind feature

jerolimov · 2020-10-21T00:16:10Z

/retest

jerolimov

Tested this locally, works as expected

/lgtm

andrewballantyne

hmm, I don't think we need to keep the dependancy on Pipeline. It is true that 1.0.1 of the Operator does still require it but based on the Compatibility Matrix 4.7 is going to be a far cry away from upstream Pipelines not having this feature (which was added in 0.12).

For sanity for the customer's cluster, I think we could go back 1 (1.2) or even more safety 2 (1.1) versions and still give that support so they are not getting a broken UI immediately after upgrading but not changing their channel. If memory serves correctly, when the Operator updates to give us an ocp-4.7 channel, they'll remove the ocp-4.5 channel. Which will remove any chance for unsupported code to exist.

The easiest approach to solving this ask I think is to just remove the requirement of Pipeline from getExecutedPipeline (see my comment below to get you started).

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

andrewballantyne · 2020-10-30T16:40:38Z

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

+export const getExecutedPipeline = (pipeline: Pipeline, pipelineRun: PipelineRun): Pipeline => {
+  return {
+    ...pipeline,
+    ...(pipelineRun?.status?.pipelineSpec && { spec: pipelineRun?.status?.pipelineSpec }),
+  };
+};


I think we want to do two things here:

Remove the dependancy on Pipeline and trim it out along all the call paths (I just did this and it's quite a bit of changes and adjustments, no references to Pipeline should exist in the call paths)

Rename it to something like (don't have to take this if you have something better) getPipelineFromPipelineRun

Tests will need to be updated naturally when this happens, since getTaskStatus heavily uses Pipeline. We may even need to update the test data to add pipelineSpec (which should be a mirror - maybe break that out to a var? - of the pipeline.spec used in the same data structure)

Removed the pipeline dependency in the call paths, updated test data and tests as well.

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

andrewballantyne

Few more touch ups please.

andrewballantyne · 2020-11-03T21:41:27Z

...ckages/dev-console/src/components/pipelineruns/detail-page-tabs/PipelineRunVisualization.tsx

-  return <PipelineVisualization pipeline={pipeline} pipelineRun={pipelineRun} />;
+  return (
+    <PipelineVisualization
+      pipeline={getPipelineFromPipelineRun(pipelineRun) || pipeline}


This is a PipelineRun visualization, it never needs the Pipeline, let's kill the fetch call for it.

andrewballantyne · 2020-11-03T21:53:12Z

.../src/components/topology/components/nodes/build-decorators/PipelineBuildDecoratorTooltip.tsx

@@ -22,7 +22,7 @@ const PipelineBuildDecoratorTooltip: React.FC<PipelineBuildDecoratorTooltipProps
    return null;
  }

-  const taskStatus = getTaskStatus(pipeline.latestRun, pipeline);
+  const taskStatus = getTaskStatus(pipeline.latestRun);


Hmm, wonder if we want to just remove the Pipeline from this data and just handle the PipelineRun as the Decorator is only really reading the PipelineRun information.

The caller of this component is build-decorator-utils and that creates a currentPipelineStatus that contains currentPipeline which is the pipeline in the above line. I believe my metholodolgy when I wrote it was to manage the standard Pipeline + .latestRun as a single param, instead of handling two variables Pipeline and PipelineRun.

This is all to say, I think we can change currentPipelineStatus to just have the PipelineRun, as nothing else really needs the Pipeline.

The work Divyanshi is doing for the "not started" icon and text is based around having a Pipeline but not having a PipelineRun. So I think we are safe on that front.

Removed the pipeline dependency here.

andrewballantyne · 2020-11-03T21:57:11Z

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

@@ -35,7 +35,216 @@ type CombinedPipelineTestData = {
 };

 type PipelineTestData = { [key in PipelineExampleNames]?: CombinedPipelineTestData };
-
+const pipelineSpec = {


Suggested change

const pipelineSpec = {

type PipelineSpecData = { [key in PipelineExampleNames]?: PipelineSpec };

const pipelineSpec: PipelineSpecData = {

andrewballantyne · 2020-11-03T22:01:11Z

frontend/packages/dev-console/src/utils/__tests__/pipeline-actions.spec.ts

-    status: { creationTimestamp: '31', conditions: [{ type: 'Succeeded', status: 'True' }] },
+    status: {
+      pipelineSpec: { tasks: [] },
+      creationTimestamp: '31',
+      conditions: [{ type: 'Succeeded', status: 'True' }],
+    },
  },
  {
    apiVersion: 'abhiapi/v1',
    kind: 'Pipeline',
    metadata: { name: 'dragonstone', namespace: 'corazon' },
    spec: { pipelineRef: { name: 'danaerys-targaeryen' } },
-    status: { creationTimestamp: '31', conditions: [{ type: 'Succeeded', status: 'Unknown' }] },
+    status: {
+      pipelineSpec: { tasks: [] },
+      creationTimestamp: '31',
+      conditions: [{ type: 'Succeeded', status: 'Unknown' }],
+    },


Well this is duplicate data from frontend/packages/dev-console/src/components/pipelines/modals/common/__tests__/utils.spec.ts 😞

Think we could move this data into pipeline-data.ts and then import it back?

Removed this mock data and used the data from pipeline-data.ts and also refactored utils.spec.ts to avoid duplication of mock data.

andrewballantyne · 2020-11-03T22:34:16Z

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

+    return null;
+  }
+  return {
+    apiVersion: PipelineModel.apiVersion,


Suggested change

apiVersion: PipelineModel.apiVersion,

apiVersion: apiVersionForModel(PipelineModel),

Awkward, I know... but apiVersion in the Model is actually the version, whereas the K8s CR is the "group + version".

andrewballantyne · 2020-11-03T22:35:47Z

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

-  const totalTasks =
-    pipeline && pipeline.spec && pipeline.spec.tasks ? pipeline.spec.tasks.length : 0;
+export const getPipelineFromPipelineRun = (pipelineRun: PipelineRun): Pipeline => {
+  if (!pipelineRun?.status?.pipelineSpec) {


It's probably possible to pass in a inlineSpec for Pipeline, can we also check to see if the label for Pipeline is here on the PipelineRun?

Suggested change

if (!pipelineRun?.status?.pipelineSpec) {

const pipelineName = pipelineRun.metadata.labels[TektonResourceLabel.pipeline];

if (!pipelineName || !pipelineRun?.status?.pipelineSpec) {

andrewballantyne · 2020-11-03T22:37:04Z

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

+    apiVersion: PipelineModel.apiVersion,
+    kind: PipelineModel.kind,
+    metadata: {
+      name: pipelineRun.metadata[TektonResourceLabel.pipeline],


❓ Shouldn't this be pipelineRun.metadata.labels?

Suggested change

name: pipelineRun.metadata[TektonResourceLabel.pipeline],

name: pipelineRun.metadata.labels[TektonResourceLabel.pipeline],

Also, perhaps make use of the variable based on my last comment.

Yes, it should be. updated.

andrewballantyne · 2020-11-03T22:40:14Z

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

+      },
+    };
+    const executedPipeline = getPipelineFromPipelineRun(plrWithOneTask);
+    expect(currentPipeline.spec.tasks).toHaveLength(2);


This is a weird expect... you're not testing data you used to deliver the test of getPipelineFromPipelineRun you trimmed it out on 324 to being only the first one. What is this expect doing other than testing you have data that has 2 points (which is a bit odd of a test)?

Yeah I Agree, after the pipeline dependancy removal this test wasn't serving the purpose. So i updated it now.

andrewballantyne · 2020-11-03T22:40:53Z

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

+    const executedPipeline = getPipelineFromPipelineRun(plrWithOneTask);
+    expect(currentPipeline.spec.tasks).toHaveLength(2);
+    expect(executedPipeline.spec.tasks).toHaveLength(1);
+    expect(executedPipeline).not.toEqual(currentPipeline);


Confused about this too... this follows my previous comment's confusion.

Updated the test

andrewballantyne · 2020-11-03T22:41:27Z

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

-  },
+  pipelineTestData[PipelineExampleNames.SIMPLE_PIPELINE].pipelineRuns[DataState.SUCCESS],
+  pipelineTestData[PipelineExampleNames.WORKSPACE_PIPELINE].pipelineRuns[DataState.SUCCESS],
+  pipelineTestData[PipelineExampleNames.CLUSTER_PIPELINE].pipelineRuns[DataState.SUCCESS],


Hard approve of this change. Very nicely done, consolidating test data is awesome!

andrewballantyne

Great test changes, thanks for cleaning that up. Couple odd changes in the test data and one minor test needed.

andrewballantyne · 2020-11-04T17:12:11Z

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

-              status: '',
-              type: 'Running',
+              status: 'Unknown',
+              type: 'Succeeded',


Interesting, this is DataState.IN_PROGRESS, you have changed it to completed... I think you're mixing data points here.

reverted back.

andrewballantyne · 2020-11-04T17:13:59Z

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

          completionTime: '2020-07-13T17:21:05Z',
          conditions: [
            {
              lastTransitionTime: '2020-07-13T17:21:05Z',
-              message:
-                'Pipeline andrew/task-ref-error can\'t be Run; it contains Tasks that don\'t exist: Couldn\'t retrieve Task "not-a-task": task.tekton.dev "not-a-task" not found',
+              message: '',


Any reason you got rid of this data text? We could trim out my name for sure haha, it was log dumps when I created. We could make the message something like Test error text, but I think it shouldn't be empty... as our tests could send that data in and it be mistaken as no error text rather than just an empty error text for testing.

While consolidating the test data, there was a test case, where it returns a static message Unknown failure condition if the message field is emtpy. For that reason i added this. I have reverted back the above one and handled the mock data in the test case itself.

andrewballantyne · 2020-11-04T17:25:22Z

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

@@ -328,3 +296,34 @@ describe('Pipeline exists test to determine whether a pipeline is linked to a pi
    expect(pipelineFound).toBeFalsy();
  });
 });
+
+describe('Pipelinerun graph to show the executed pipeline structure', () => {


We are missing a test for the actual result. You definitely have tested the paths to our current usage, but the method is, in isolation, just creating a Pipeline. We don't have a test to make sure it did do that. Goals of Unit Tests for functions should be to cover edge cases, any conditional paths, intended uses today and the function is performing the action it prescribes to - which in this case is making a PL CR out of the PLR CR.

andrewballantyne

/lgtm

andrewballantyne · 2020-11-04T19:26:05Z

/retest

Looks to be a connection issue

openshift-bot · 2020-11-04T19:57:49Z

/retest

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

openshift-bot · 2020-11-05T01:48:52Z

/retest

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

openshift-bot · 2020-11-05T02:01:49Z

/retest

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

openshift-bot · 2020-11-05T03:19:54Z

/retest

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

openshift-bot · 2020-11-05T03:58:49Z

/retest

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

openshift-bot · 2020-11-05T04:24:49Z

/retest

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

openshift-bot · 2020-11-05T04:50:50Z

/retest

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

karthikjeeyar · 2020-11-05T05:06:02Z

I rebased due to merge conflicts.

andrewballantyne · 2020-11-05T14:22:55Z

/retest
/lgtm

openshift-ci-robot · 2020-11-05T14:23:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, jerolimov, 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/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-bot · 2020-11-05T16:06:41Z

/retest

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

openshift-bot · 2020-11-05T16:19:41Z

/retest

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

jerolimov · 2020-11-05T16:38:26Z

/retest

openshift-bot · 2020-11-05T18:16:44Z

/retest

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

openshift-bot · 2020-11-05T19:47:46Z

/retest

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

openshift-bot · 2020-11-05T20:52:41Z

/retest

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

openshift-bot · 2020-11-05T21:05:47Z

/retest

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

openshift-bot · 2020-11-05T22:23:54Z

/retest

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

openshift-bot · 2020-11-05T23:41:41Z

/retest

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

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

openshift-ci-robot requested review from andrewballantyne and jerolimov October 20, 2020 11:36

openshift-ci-robot added the component/dev-console Related to dev-console label Oct 20, 2020

jerolimov approved these changes Oct 21, 2020

View reviewed changes

openshift-ci-robot assigned jerolimov Oct 21, 2020

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

andrewballantyne suggested changes Oct 30, 2020

View reviewed changes

openshift-ci-robot assigned andrewballantyne Oct 30, 2020

karthikjeeyar force-pushed the feat-pipelinerun-graph branch from 7434a46 to b99a150 Compare November 3, 2020 19:24

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2020

andrewballantyne suggested changes Nov 3, 2020

View reviewed changes

karthikjeeyar force-pushed the feat-pipelinerun-graph branch from b99a150 to f33b7df Compare November 4, 2020 14:28

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2020

karthikjeeyar force-pushed the feat-pipelinerun-graph branch from f33b7df to 470ac6c Compare November 4, 2020 15:50

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2020

andrewballantyne suggested changes Nov 4, 2020

View reviewed changes

karthikjeeyar force-pushed the feat-pipelinerun-graph branch from 470ac6c to f482b76 Compare November 4, 2020 18:52

andrewballantyne approved these changes Nov 4, 2020

View reviewed changes

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 4, 2020

update pipelinerun graph to show past pipeline structure

3df3d8e

karthikjeeyar force-pushed the feat-pipelinerun-graph branch from f482b76 to 3df3d8e Compare November 5, 2020 04:57

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2020

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

openshift-merge-robot merged commit 0ff60c4 into openshift:master Nov 6, 2020

spadgett added this to the v4.7 milestone Nov 16, 2020

	const pipelineSpec = {
	type PipelineSpecData = { [key in PipelineExampleNames]?: PipelineSpec };
	const pipelineSpec: PipelineSpecData = {

	apiVersion: PipelineModel.apiVersion,
	apiVersion: apiVersionForModel(PipelineModel),

	if (!pipelineRun?.status?.pipelineSpec) {
	const pipelineName = pipelineRun.metadata.labels[TektonResourceLabel.pipeline];
	if (!pipelineName \|\| !pipelineRun?.status?.pipelineSpec) {

	name: pipelineRun.metadata[TektonResourceLabel.pipeline],
	name: pipelineRun.metadata.labels[TektonResourceLabel.pipeline],

update pipelinerun graph to show past pipeline structure #6960

update pipelinerun graph to show past pipeline structure #6960

Conversation

karthikjeeyar commented Oct 20, 2020

jerolimov commented Oct 21, 2020

jerolimov left a comment

Choose a reason for hiding this comment

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

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

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

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

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne commented Nov 4, 2020

openshift-bot commented Nov 4, 2020

openshift-bot commented Nov 5, 2020

openshift-bot commented Nov 5, 2020

openshift-bot commented Nov 5, 2020

openshift-bot commented Nov 5, 2020

openshift-bot commented Nov 5, 2020

openshift-bot commented Nov 5, 2020

karthikjeeyar commented Nov 5, 2020

andrewballantyne commented Nov 5, 2020

openshift-ci-robot commented Nov 5, 2020

openshift-bot commented Nov 5, 2020

openshift-bot commented Nov 5, 2020

jerolimov commented Nov 5, 2020

openshift-bot commented Nov 5, 2020

openshift-bot commented Nov 5, 2020

openshift-bot commented Nov 5, 2020

openshift-bot commented Nov 5, 2020

openshift-bot commented Nov 5, 2020

openshift-bot commented Nov 5, 2020