Fix error showing TaskRun steps that have not run #653

ncskier · 2019-10-18T18:59:36Z

Changes

Fixes #644
This fix shows a neutral "Not run" status for steps that follow a step with an
error. Previously, these steps showed a misleading green "Completed" status.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide
for more details.

ncskier · 2019-10-18T19:01:05Z

/uncc @dibbles
/uncc @vtereso
/cc @a-roberts
/cc @steveodonovan

AlanGreene

We should not hide steps that have not executed, and the current behaviour where we do not display Task(Run)s that have not begun executing is a known issue we've been tracking for a while.

When we introduce the pipeline diagram we will display all nodes, decorated appropriately to indicate their status.

A fix for #644 should ensure steps that have not run are displayed as such, the component already supports this state, we may need to make some minor changes to ensure the correct props are passed to render this as expected: https://alangreene.github.io/dashboard/?path=/story/step--default

ncskier · 2019-10-21T13:47:41Z

Thanks for the feedback @AlanGreene! I'll update this PR to show the default step state instead of hiding the steps 👍
/hold

ncskier · 2019-10-21T20:11:21Z

/hold cancel

packages/components/src/components/Log/Log.js

packages/components/src/components/PipelineRun/PipelineRun.js

packages/components/src/components/Task/Task.js

src/utils/index.js

AlanGreene · 2019-10-22T11:35:01Z

src/utils/index.js

+    return steps;
+  }
+  let errorIndex = steps.length - 1;
+  return steps.map((step, index) => {


I think it's worth raising tektoncd/pipeline#1439 in the next working group meeting at least to ask for feedback on the issue.

It also looks like the behaviour when cancelling a PipelineRun is different, I see steps marked as Waiting instead of Completed. Maybe this would be more appropriate than Error?

Having this workaround in the dashboard for now resolves the immediate user-facing issue for users of the dashboard, but users of kubectl or other tools still have the same misleading information presented (i.e. a step that hasn't executed is marked as Completed), and each tool needs to handle this themselves.

AlanGreene · 2019-10-22T11:45:01Z

@ncskier can you also update the PR title + description to reflect the change in approach taken here? Looks like the commit message is already updated. Thanks

ncskier · 2019-10-22T19:45:39Z

Thanks for the feedback @AlanGreene! I've updated the PR title & description, and I think I've addressed all of your concerns. Let me know if there's anything else that I can do 🙂

AlanGreene

almost there, just a couple of imports that need updating to reference the utils package instead

packages/components/src/components/PipelineRun/PipelineRun.js

packages/components/src/components/Task/Task.js

Fixes tektoncd#644 This fix shows a neutral "Not run" status for steps that follow a step with an error. Previously, these steps showed a misleading green "Completed" status.

ncskier · 2019-10-23T15:01:05Z

Ah, thanks @AlanGreene! I just fixed the imports 😁

AlanGreene

Looks good, thanks for making those changes 👍

/lgtm

tekton-robot · 2019-10-24T14:25:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlanGreene

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:

~~OWNERS~~ [AlanGreene]

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

This reverts commit c1aa3e4. Reverts tektoncd#653 Tekton Pipelines now correctly sets the status of skipped steps tektoncd/pipeline#1439

This reverts commit c1aa3e4. Reverts #653 Tekton Pipelines now correctly sets the status of skipped steps tektoncd/pipeline#1439

This fixes the error seen when using the Tekton Dashboard with Tekton Pipelines v0.8.0. In order to support users on Tekton Pipelines v0.8.0 and earlier, the fix from tektoncd#653 is going back into the Dashboard codebase. The fix was previously reverted when Tekton Pipelines v0.9.0 fixed the root cause of the issue (https://github.com/tektoncd/dashboard/pull/795/files).

This fixes an error seen when using the Tekton Dashboard with Tekton Pipelines v0.8.0. In order to support users on Tekton Pipelines v0.8.0 and earlier, the fix from tektoncd#653 is going back into the Dashboard codebase. The fix was previously reverted when Tekton Pipelines v0.9.0 fixed the root cause of the issue (https://github.com/tektoncd/dashboard/pull/795/files).

This fixes an error seen when using the Tekton Dashboard with Tekton Pipelines v0.8.0. In order to support users on Tekton Pipelines v0.8.0 and earlier, the fix from #653 is going back into the Dashboard codebase. The fix was previously reverted when Tekton Pipelines v0.9.0 fixed the root cause of the issue (https://github.com/tektoncd/dashboard/pull/795/files).

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2019

tekton-robot requested review from dibbles and vtereso October 18, 2019 18:59

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 18, 2019

tekton-robot requested review from a-roberts and steveodonovan and removed request for dibbles and vtereso October 18, 2019 19:01

ncskier marked this pull request as ready for review October 18, 2019 19:13

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2019

ncskier mentioned this pull request Oct 18, 2019

If the build step in tekton pipeline fails, tekton dashboard shows the push step as green #644

Closed

AlanGreene requested changes Oct 21, 2019

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2019

ncskier force-pushed the issue644 branch from 19b44ca to ef6828b Compare October 21, 2019 19:51

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 21, 2019

ncskier force-pushed the issue644 branch from ef6828b to 3b9f1d5 Compare October 21, 2019 19:53

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2019

AlanGreene requested changes Oct 22, 2019

View reviewed changes

ncskier changed the title ~~Hide TaskRun steps that are not executed~~ Fix error showing TaskRun steps that have not run Oct 22, 2019

ncskier force-pushed the issue644 branch 5 times, most recently from 7bb486c to b927224 Compare October 22, 2019 18:22

AlanGreene requested changes Oct 23, 2019

View reviewed changes

packages/components/src/components/PipelineRun/PipelineRun.js Outdated Show resolved Hide resolved

packages/components/src/components/Task/Task.js Outdated Show resolved Hide resolved

Fix error showing TaskRun steps that have not run

c2b6ab6

Fixes tektoncd#644 This fix shows a neutral "Not run" status for steps that follow a step with an error. Previously, these steps showed a misleading green "Completed" status.

ncskier force-pushed the issue644 branch from b927224 to c2b6ab6 Compare October 23, 2019 14:59

AlanGreene approved these changes Oct 24, 2019

View reviewed changes

tekton-robot assigned AlanGreene Oct 24, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2019

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

tekton-robot merged commit c1aa3e4 into tektoncd:master Oct 24, 2019

This was referenced Nov 28, 2019

Steps displayed after a failed one are marked as "not-run" #782

Closed

Revert "Fix error showing TaskRun steps that have not run" #795

Merged

tekton-robot pushed a commit that referenced this pull request Nov 29, 2019

Revert "Fix error showing TaskRun steps that have not run"

4213db8

This reverts commit c1aa3e4. Reverts #653 Tekton Pipelines now correctly sets the status of skipped steps tektoncd/pipeline#1439

ncskier mentioned this pull request Jan 16, 2020

if a build step in tekton pipeline fails, it should not go on to the next build step kabanero-io/kabanero-pipelines#86

Closed

ncskier mentioned this pull request Jan 16, 2020

Fix error showing TaskRun steps that have not run (again) #906

Merged

3 tasks

aadeshpa mentioned this pull request Jan 16, 2020

nodejs-loopback-build-push-deploy-pipeline-run result status is inconsistent kabanero-io/kabanero-pipelines#155

Closed

AlanGreene mentioned this pull request May 12, 2020

Pipelineruns in Tekton dashboard show incorrect status for each step #1367

Closed

This was referenced Jun 18, 2020

[Snyk] Upgrade @vx/event from 0.0.192 to 0.0.196 jperdikis/dashboard#1

Open

[Snyk] Upgrade @vx/network from 0.0.192 to 0.0.196 jperdikis/dashboard#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error showing TaskRun steps that have not run #653

Fix error showing TaskRun steps that have not run #653

ncskier commented Oct 18, 2019 •

edited

Loading

ncskier commented Oct 18, 2019

AlanGreene left a comment

ncskier commented Oct 21, 2019

ncskier commented Oct 21, 2019

AlanGreene Oct 22, 2019 •

edited

Loading

AlanGreene commented Oct 22, 2019 •

edited

Loading

ncskier commented Oct 22, 2019

AlanGreene left a comment

ncskier commented Oct 23, 2019 •

edited

Loading

AlanGreene left a comment

tekton-robot commented Oct 24, 2019

Fix error showing TaskRun steps that have not run #653

Fix error showing TaskRun steps that have not run #653

Conversation

ncskier commented Oct 18, 2019 • edited Loading

Changes

Submitter Checklist

ncskier commented Oct 18, 2019

AlanGreene left a comment

Choose a reason for hiding this comment

ncskier commented Oct 21, 2019

ncskier commented Oct 21, 2019

AlanGreene Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

AlanGreene commented Oct 22, 2019 • edited Loading

ncskier commented Oct 22, 2019

AlanGreene left a comment

Choose a reason for hiding this comment

ncskier commented Oct 23, 2019 • edited Loading

AlanGreene left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 24, 2019

ncskier commented Oct 18, 2019 •

edited

Loading

AlanGreene Oct 22, 2019 •

edited

Loading

AlanGreene commented Oct 22, 2019 •

edited

Loading

ncskier commented Oct 23, 2019 •

edited

Loading