Bug 1876515: Fix inline taskSpec error in pipeline page #6541

karthikjeeyar · 2020-09-07T11:31:51Z

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

Problem:
Issue with inline spec over taskref causes the pipeline details page to break

Solution:
Handle the inline spec issue and skip the visualization

Screenshots:

Test cases:

PipelineVisualization tests

pipeline-actions tests

openshift-ci-robot · 2020-09-07T11:39:47Z

@karthikjeeyar: This pull request references Bugzilla bug 1876515, 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 1876515: Fix inline taskSpec error in pipeline page

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.

divyanshiGupta · 2020-09-07T11:37:45Z

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

@@ -181,7 +181,16 @@ const appendPipelineRunStatus = (pipeline, pipelineRun) => {
    return mTask;
  });
 };
-
+export const hasInlineTaskSpec = (pipeline: K8sResourceKind) => {
+  const tasks = (pipeline.spec && pipeline.spec.tasks) || [];


Suggested change

const tasks = (pipeline.spec && pipeline.spec.tasks) || [];

const tasks = pipeline.spec?.tasks ?? [];

divyanshiGupta · 2020-09-07T11:41:39Z

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

@@ -192,7 +201,7 @@ export const getPipelineTasks = (
 ): PipelineVisualizationTaskItem[][] => {
  // Each unit in 'out' array is termed as stage | out = [stage1 = [task1], stage2 = [task2,task3], stage3 = [task4]]
  const out = [];
-  if (!pipeline.spec || !pipeline.spec.tasks) {
+  if (!pipeline.spec || !pipeline.spec.tasks || hasInlineTaskSpec(pipeline)) {


Suggested change

if (!pipeline.spec || !pipeline.spec.tasks || hasInlineTaskSpec(pipeline)) {

if (!pipeline.spec?.tasks || hasInlineTaskSpec(pipeline)) {

sahil143

/lgtm

verified changes locally

sahil143 · 2020-09-09T09:57:08Z

/lgtm cancel

It breaks in the Edit Pipeline page

karthikjeeyar · 2020-09-09T10:25:11Z

Added the check in edit scenario, so that if there is an inline taskSpec, it will not throw error and will allow us to add/modify the tasks in the builder form.

andrewballantyne · 2020-09-09T13:48:04Z

@karthikjeeyar I think we need UX here... I don't think it's appropriate to edit a Pipeline and clear out the tasks because they are using inline spec - this could have wildly unexpected results for the user. I think we need to take a path of least resistance here - it is an unsupported feature of the DevConsole. Maybe we showcase a message saying "Using tasks inlineSpec, which is unsupported" in the "visualization area" and removing the Edit Pipeline kebab option.

cc @bgliwa01 / @openshift/team-devconsole-ux

andrewballantyne · 2020-09-09T13:50:21Z

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

+  let hasInlineSpec = false;
+  tasks.forEach((task) => {
+    if (!hasInlineSpec) {
+      hasInlineSpec = !!(task.taskSpec && !task.taskRef);
+    }
+  });


You're re-implementing Array.prototype.some... best just use it instead.

Updated it.

bgliwa01 · 2020-09-10T19:16:08Z

@karthikjeeyar I agree with @andrewballantyne we should remove the edit pipeline option in the kebab dropdown and then we can display the message "This Pipeline cannot be visualized. Pipeline taskSpec is not supported."

andrewballantyne · 2020-09-10T19:31:33Z

@karthikjeeyar I agree with @andrewballantyne we should remove the edit pipeline option in the kebab dropdown and then we can display the message "This Pipeline cannot be visualized. Pipeline taskSpec is not supported."

@karthikjeeyar When you do this, make sure there is a gap under the message before the first label.

Thanks @bgliwa01 !

openshift-ci-robot · 2020-09-14T11:11:46Z

@karthikjeeyar: This pull request references Bugzilla bug 1876515, 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 1876515: Fix inline taskSpec error in pipeline page

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

Updated the code to hide the edit pipeline option and added the UX provided message to the visualization area.
@bgliwa01 Please take a look at the updated screenshots.
cc: @andrewballantyne

andrewballantyne · 2020-09-14T11:42:41Z

...console/src/components/pipelines/detail-page-tabs/pipeline-details/PipelineVisualization.tsx

    // Nothing to render
    // TODO: Confirm wording with UX; ODC-1860
-    return <Alert variant="info" isInline title="This Pipeline has no tasks to visualize." />;


Why did you remove the return? I think we can just have another return here.

EDIT: In case it's unclear, I'm asking you to just keep this simple and revert the changes you did here. Add the new if-statement and make it a return of the new alert. There is no reason not to exit early.

I have moved it inside the block, so that it gets the margin-bottom style (for the gap underneath the message) from the odc-pipeline-visualization class.

Ah, I see... why don't we clean this up more and instead of treating errors differently, just have a variable called content or something and have 3 if-else statements... landing on rendering the Component as the fallback else.

It'll look cleaner and be easier to read.

I agree, its more readable now. Updated it

andrewballantyne · 2020-09-14T11:44:24Z

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

@@ -181,7 +181,10 @@ const appendPipelineRunStatus = (pipeline, pipelineRun) => {
    return mTask;
  });
 };
-
+export const hasInlineTaskSpec = (pipeline: K8sResourceKind) => {
+  const tasks = pipeline?.spec.tasks ?? [];


spec is not optional.

andrewballantyne · 2020-09-14T11:45:40Z

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

@@ -192,7 +195,7 @@ export const getPipelineTasks = (
 ): PipelineVisualizationTaskItem[][] => {
  // Each unit in 'out' array is termed as stage | out = [stage1 = [task1], stage2 = [task2,task3], stage3 = [task4]]
  const out = [];
-  if (!pipeline.spec || !pipeline.spec.tasks) {
+  if (!pipeline?.spec.tasks) {


Don't think this can ever happen anymore. Tasks of 1 is required by the operator. We can keep this if you change it to look for tasks also being an empty array.

Also spec is not optional.

andrewballantyne · 2020-09-14T12:28:10Z

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

@@ -12,7 +12,7 @@ export const mockPipelinesJSON: K8sResourceKind[] = [
      params: [
        {
          name: 'APP_NAME',
-          type: 'string',


type should be allowed 🤔 Why did you remove this?

Ah because it's not part of the type 😞

Can you just add it to the type instead?
It's supported by upstream

Each declared parameter has a type field, which can be set to either array or string.

We'll want the type to probably be:

type: 'string' | 'array';

Instead of a straight string type... this way we can easily see what we support.

bgliwa01

lgtm

andrewballantyne · 2020-09-14T15:32:00Z

...console/src/components/pipelines/detail-page-tabs/pipeline-details/PipelineVisualization.tsx

    // Nothing to render
    // TODO: Confirm wording with UX; ODC-1860
-    return <Alert variant="info" isInline title="This Pipeline has no tasks to visualize." />;


Ah, I see... why don't we clean this up more and instead of treating errors differently, just have a variable called content or something and have 3 if-else statements... landing on rendering the Component as the fallback else.

It'll look cleaner and be easier to read.

jerolimov · 2020-09-14T18:33:32Z

...ole/src/components/pipelines/detail-page-tabs/pipeline-details/PipelineVisualizationTask.tsx

-      kind?: string;
-    };
+    taskSpec?: PipelineTaskSpec;
+    taskRef?: PipelineTaskRef;


jerolimov

Tested it locally, works fine 👍

/lgtm

openshift-ci-robot · 2020-09-15T12:49:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, bgliwa01, 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

jerolimov · 2020-09-15T12:49:46Z

/retest

openshift-bot · 2020-09-15T12:58:38Z

/retest

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

openshift-ci-robot · 2020-09-15T17:51:24Z

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

openshift/console#6541

Bugzilla bug 1876515 has been moved to the MODIFIED state.

In response to this:

Bug 1876515: Fix inline taskSpec error in pipeline page

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-15T18:51:59Z

/cherry-pick release-4.5

openshift-cherrypick-robot · 2020-09-15T18:52:08Z

@andrewballantyne: new pull request created: #6638

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.

openshift-ci-robot requested review from abhi-kn and christianvogt September 7, 2020 11:32

openshift-ci-robot added the component/dev-console Related to dev-console label Sep 7, 2020

karthikjeeyar changed the title ~~Fix inline taskSpec error in pipeline page~~ Bug 1876515: Fix inline taskSpec error in pipeline page Sep 7, 2020

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 Sep 7, 2020

divyanshiGupta suggested changes Sep 7, 2020

View reviewed changes

karthikjeeyar force-pushed the fix-inline-spec branch 2 times, most recently from e5f062c to c9f7470 Compare September 8, 2020 06:46

sahil143 approved these changes Sep 9, 2020

View reviewed changes

openshift-ci-robot assigned sahil143 Sep 9, 2020

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

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

karthikjeeyar force-pushed the fix-inline-spec branch from c9f7470 to bdf19b0 Compare September 9, 2020 10:12

andrewballantyne reviewed Sep 9, 2020

View reviewed changes

karthikjeeyar force-pushed the fix-inline-spec branch from bdf19b0 to 3fb1f56 Compare September 9, 2020 16:21

karthikjeeyar force-pushed the fix-inline-spec branch from 3fb1f56 to fa864a9 Compare September 14, 2020 11:11

andrewballantyne suggested changes Sep 14, 2020

View reviewed changes

andrewballantyne reviewed Sep 14, 2020

View reviewed changes

bgliwa01 approved these changes Sep 14, 2020

View reviewed changes

karthikjeeyar force-pushed the fix-inline-spec branch from fa864a9 to 13cf376 Compare September 14, 2020 14:34

andrewballantyne suggested changes Sep 14, 2020

View reviewed changes

handle inline taskSpec in pipeline detail page

b5466a5

karthikjeeyar force-pushed the fix-inline-spec branch from 13cf376 to b5466a5 Compare September 14, 2020 16:49

andrewballantyne approved these changes Sep 14, 2020

View reviewed changes

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

jerolimov reviewed Sep 14, 2020

View reviewed changes

jerolimov approved these changes Sep 15, 2020

View reviewed changes

openshift-ci-robot assigned jerolimov Sep 15, 2020

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

openshift-merge-robot merged commit 4907c9d into openshift:master Sep 15, 2020

openshift-cherrypick-robot mentioned this pull request Sep 15, 2020

[release-4.5] Bug 1879247: Fix inline taskSpec error in pipeline page #6638

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1876515: Fix inline taskSpec error in pipeline page #6541

Bug 1876515: Fix inline taskSpec error in pipeline page #6541

karthikjeeyar commented Sep 7, 2020 •

edited

openshift-ci-robot commented Sep 7, 2020

divyanshiGupta Sep 7, 2020

divyanshiGupta Sep 7, 2020

sahil143 left a comment

sahil143 commented Sep 9, 2020

karthikjeeyar commented Sep 9, 2020

andrewballantyne commented Sep 9, 2020 •

edited

andrewballantyne Sep 9, 2020

karthikjeeyar Sep 9, 2020

bgliwa01 commented Sep 10, 2020

andrewballantyne commented Sep 10, 2020

openshift-ci-robot commented Sep 14, 2020

karthikjeeyar commented Sep 14, 2020

andrewballantyne Sep 14, 2020 •

edited

karthikjeeyar Sep 14, 2020

andrewballantyne Sep 14, 2020

karthikjeeyar Sep 14, 2020

andrewballantyne Sep 14, 2020

andrewballantyne Sep 14, 2020

andrewballantyne Sep 14, 2020

andrewballantyne Sep 14, 2020

bgliwa01 left a comment

andrewballantyne Sep 14, 2020

jerolimov Sep 14, 2020

jerolimov left a comment

openshift-ci-robot commented Sep 15, 2020

jerolimov commented Sep 15, 2020

openshift-bot commented Sep 15, 2020

openshift-ci-robot commented Sep 15, 2020

andrewballantyne commented Sep 15, 2020

openshift-cherrypick-robot commented Sep 15, 2020

	const tasks = (pipeline.spec && pipeline.spec.tasks) \|\| [];
	const tasks = pipeline.spec?.tasks ?? [];

	if (!pipeline.spec \|\| !pipeline.spec.tasks \|\| hasInlineTaskSpec(pipeline)) {
	if (!pipeline.spec?.tasks \|\| hasInlineTaskSpec(pipeline)) {

Bug 1876515: Fix inline taskSpec error in pipeline page #6541

Bug 1876515: Fix inline taskSpec error in pipeline page #6541

Conversation

karthikjeeyar commented Sep 7, 2020 • edited

openshift-ci-robot commented Sep 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil143 left a comment

Choose a reason for hiding this comment

sahil143 commented Sep 9, 2020

karthikjeeyar commented Sep 9, 2020

andrewballantyne commented Sep 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgliwa01 commented Sep 10, 2020

andrewballantyne commented Sep 10, 2020

openshift-ci-robot commented Sep 14, 2020

karthikjeeyar commented Sep 14, 2020

andrewballantyne Sep 14, 2020 • edited

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

bgliwa01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerolimov left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Sep 15, 2020

jerolimov commented Sep 15, 2020

openshift-bot commented Sep 15, 2020

openshift-ci-robot commented Sep 15, 2020

andrewballantyne commented Sep 15, 2020

openshift-cherrypick-robot commented Sep 15, 2020

karthikjeeyar commented Sep 7, 2020 •

edited

andrewballantyne commented Sep 9, 2020 •

edited

andrewballantyne Sep 14, 2020 •

edited