Improve display of when expressions and retries

[Megan-Wright]

Expected behavior

If a Condition fails in my PipelineRun it should be displayed clearly on the PipelineRun details page.
Successful Conditions should not distract from other TaskRuns in the PipelineRun, especially when there are many of them. (originally reported in #1574)

Actual behavior

The failed Condition is rendered the same as a failed TaskRun which can be confusing for users. See 'deployment-condition' in the screenshot:

The use of red + error icon suggests a problem, but this is not the case. A failed Condition does not affect the PipelineRun status and is often expected. We should consider a different icon / colour / label to differentiate between failed Conditions and other TaskRuns.

Also for Pipelines making heavy use of Conditions it can be difficult to focus on the 'interesting' content and creates a very busy UI.

Additional Info

This happened with a PipelineRun triggered by a webhook when I opened a pull request...
YAML here:

(base) Megans-MacBook-Pro:services megan.wright@ibm.com$ k get pr buildah-pipeline-run-nkxv6 -o yaml
apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"tekton.dev/v1beta1","kind":"Pipeline","metadata":{"annotations":{},"name":"buildah-pipeline","namespace":"openshift-pipelines"},"spec":{"params":[{"description":"The event type","name":"event-type","type":"string"}],"resources":[{"name":"git-source","type":"git"},{"name":"docker-image","type":"image"}],"tasks":[{"name":"build-simple","resources":{"inputs":[{"name":"source","resource":"git-source"}],"outputs":[{"name":"image","resource":"docker-image"}]},"taskRef":{"name":"buildah"}},{"conditions":[{"conditionRef":"deployment-condition","params":[{"name":"event-type","value":"$(params.event-type)"}]}],"name":"deploy-simple","resources":{"inputs":[{"name":"git-source","resource":"git-source"},{"name":"image-out","resource":"docker-image"}]},"runAfter":["build-simple"],"taskRef":{"name":"deploy-simple-kubectl-task"}}]}}
  creationTimestamp: "2020-05-27T15:09:02Z"
  generateName: buildah-pipeline-run-
  generation: 1
  labels:
    tekton.dev/pipeline: buildah-pipeline
    triggers.tekton.dev/eventlistener: tekton-webhooks-eventlistener
    triggers.tekton.dev/trigger: test-openshift-pipelines-pullrequest-event
    triggers.tekton.dev/triggers-eventid: tgk65
    webhooks.tekton.dev/gitBranch: foo
    webhooks.tekton.dev/gitOrg: megan-wright
    webhooks.tekton.dev/gitRepo: knative-helloworld
    webhooks.tekton.dev/gitServer: github.ibm.com
  name: buildah-pipeline-run-nkxv6
  namespace: openshift-pipelines
  resourceVersion: "63472068"
  selfLink: /apis/tekton.dev/v1beta1/namespaces/openshift-pipelines/pipelineruns/buildah-pipeline-run-nkxv6
  uid: 7106a325-f7d8-465f-b81e-d87a85ce44c5
spec:
  params:
  - name: event-type
    value: pull_request
  pipelineRef:
    name: buildah-pipeline
  resources:
  - name: git-source
    resourceRef:
      name: git-source-j4zp5
  - name: docker-image
    resourceRef:
      name: docker-image-j4zp5
  serviceAccountName: tekton-dashboard
  timeout: 1h0m0s
status:
  completionTime: "2020-05-27T15:12:33Z"
  conditions:
  - lastTransitionTime: "2020-05-27T15:12:33Z"
    message: 'Tasks Completed: 1, Skipped: 1'
    reason: Completed
    status: "True"
    type: Succeeded
  startTime: "2020-05-27T15:09:02Z"
  taskRuns:
    buildah-pipeline-run-nkxv6-build-simple-c75s7:
      pipelineTaskName: build-simple
      status:
        completionTime: "2020-05-27T15:12:09Z"
        conditions:
        - lastTransitionTime: "2020-05-27T15:12:09Z"
          message: All Steps have completed executing
          reason: Succeeded
          status: "True"
          type: Succeeded
        podName: buildah-pipeline-run-nkxv6-build-simple-c75s7-pod-bq78v
        resourcesResult:
        - key: commit
          resourceRef:
            name: git-source-j4zp5
          value: e2673e3e6829e994f52104bac433c3a08bcedccf
        startTime: "2020-05-27T15:09:03Z"
        steps:
        - container: step-build
          imageID: quay.io/buildah/stable@sha256:15ecf2c5a3d013221e366549802c79eed2db7aebeb6bbf492b13d2d877df792a
          name: build
          terminated:
            containerID: cri-o://79257b9b6e6f81a0ad982ebabc00ababab59b9c0d9cbe14ebdeff5c96fdc9cd2
            exitCode: 0
            finishedAt: "2020-05-27T15:12:00Z"
            reason: Completed
            startedAt: "2020-05-27T15:09:31Z"
        - container: step-push
          imageID: quay.io/buildah/stable@sha256:15ecf2c5a3d013221e366549802c79eed2db7aebeb6bbf492b13d2d877df792a
          name: push
          terminated:
            containerID: cri-o://74fd9f350b72aef2457207a053f12b0a7371bc3ce0bda59ebe8063b6b7a6a9a9
            exitCode: 0
            finishedAt: "2020-05-27T15:12:09Z"
            reason: Completed
            startedAt: "2020-05-27T15:12:00Z"
        - container: step-git-source-git-source-j4zp5-kfmpq
          imageID: gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/git-init@sha256:add85f33c5ac0aa02712ec6e6caad3d4bb7faa33043c5ca252a824b050b4b8e2
          name: git-source-git-source-j4zp5-kfmpq
          terminated:
            containerID: cri-o://c30c54567c8bbea381f933a23c409f4f3cde7c1888af9158c577dbc691be61d9
            exitCode: 0
            finishedAt: "2020-05-27T15:09:31Z"
            message: '[{"key":"commit","value":"e2673e3e6829e994f52104bac433c3a08bcedccf","resourceRef":{"name":"git-source-j4zp5"}}]'
            reason: Completed
            startedAt: "2020-05-27T15:09:27Z"
        - container: step-image-digest-exporter-ffpjw
          imageID: gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/imagedigestexporter@sha256:c0ac90740dc6d3771f77f8bca5e3fef6b5bb97b7eef189f90488553a874aa602
          name: image-digest-exporter-ffpjw
          terminated:
            containerID: cri-o://5591706d3cdccaf589410dbe9d8a670511cf4c4985d1b2069bf125aaebe90dcc
            exitCode: 0
            finishedAt: "2020-05-27T15:12:09Z"
            reason: Completed
            startedAt: "2020-05-27T15:12:09Z"
        - container: step-create-dir-image-9lfgz
          imageID: docker.io/library/busybox@sha256:836945da1f3afe2cfff376d379852bbb82e0237cb2925d53a13f53d6e8a8c48c
          name: create-dir-image-9lfgz
          terminated:
            containerID: cri-o://a84a178eb5cc6f44c76609df05739ba3b4584b3d822d81f7930a516a611552be
            exitCode: 0
            finishedAt: "2020-05-27T15:09:27Z"
            reason: Completed
            startedAt: "2020-05-27T15:09:27Z"
    buildah-pipeline-run-nkxv6-deploy-simple-j8bhj:
      conditionChecks:
        buildah-pipeline-run-nkxv6-deploy-simple-j8bhj-deployment-jbnld:
          conditionName: deployment-condition
          status:
            check:
              terminated:
                containerID: cri-o://362f6182954555c1c3a640b6a8cb3e108e4b171f301ad8cda2f31c9224f9c2cd
                exitCode: 1
                finishedAt: "2020-05-27T15:12:32Z"
                reason: Error
                startedAt: "2020-05-27T15:12:32Z"
            completionTime: "2020-05-27T15:12:33Z"
            conditions:
            - lastTransitionTime: "2020-05-27T15:12:33Z"
              message: '"step-deployment-condition" exited with code 1 (image: "docker.io/library/alpine@sha256:39eda93d15866957feaee28f8fc5adb545276a64147445c64992ef69804dbf01");
                for logs run: kubectl -n openshift-pipelines logs buildah-pipeline-run-nkxv6-deploy-simple-j8bhj-deployment-p924q
                -c step-deployment-condition'
              reason: Failed
              status: "False"
              type: Succeeded
            podName: buildah-pipeline-run-nkxv6-deploy-simple-j8bhj-deployment-p924q
            startTime: "2020-05-27T15:12:10Z"
      pipelineTaskName: deploy-simple
      status:
        conditions:
        - lastTransitionTime: "2020-05-27T15:12:33Z"
          message: ConditionChecks failed for Task buildah-pipeline-run-nkxv6-deploy-simple-j8bhj
            in PipelineRun buildah-pipeline-run-nkxv6
          reason: ConditionCheckFailed
          status: "False"
          type: Succeeded
        podName: ""

[Megan-Wright]

@AlanGreene @steveodonovan FYI

[AlanGreene]

The PipelineRun has actually passed. One of the Condition checks failed (the red item in the TaskTree) so a Task was skipped, but this doesn't affect the PipelineRun status and it was still successful.

[a-roberts]

Yeah, so that's an interesting one - think it from a dev team's perspective (CI/CD set up on their GitHub repository, lots of folks just looking to build/push, but maybe not to master yet, lots of Pipelines) - they're gonna see loads of red builds (on the PipelineRuns/TaskRun page) when really things were built fine but the condition failed (maybe the condition is: was it a push to master), and they wouldn't know to click on through.

@AlanGreene you may have considered this already but how about a new status icon (orange perhaps?), and this means the PipelineRun/TaskRun passed but, warning, one or more conditions failed? Or a warning icon?

Getting a sense of deja vu...

https://github.com/pipeline-hotel/example-pipelines/blob/master/triggers-resources/config/github/buildah/deployment-condition.yaml being the condition in question that Megan's used. So I agree there isn't a problem here but there's scope for making things better.

[AlanGreene]

Yeah I agree there's scope for improvement here and thought we had an issue tracking that but can't find it now. I'm gonna repurpose this one.

[AlanGreene]

Found the original issue: #1340
Closed it as a duplicate and linked to this one.

We should consider how best to differentiate Conditions from other TaskRuns. In case of success this is probably less important, but for failed Conditions it should be clear they're not a regular failed TaskRun. Perhaps this can be done by changing the status displayed from 'Failed' to 'ConditionFailed' or similar, along with changing from the red highlight to orange / yellow (see Carbon color usage guidelines).

When we resolve #1263 it would be great to indicate the relationship between TaskRuns and Conditions so a user can easily see which TaskRuns were skipped and why.

[AlanGreene]

Updated to include #1574
We should handle these together in a sensible way.

Original description from that issue:

I have a lot of conditions in my pipelinerun. 90% of the time the pipelines run fine and I don't care about seeing all the conditional tasks, all I care about are the tasks that actually do the work. Would be nice to have a toggle switch in the UI to hide conditional based tasks, and just leave the rest. Having to wade through all the conditional tasks in the view just clutters things imho

It might also be useful to consider some of the recent discussions about display of TaskRun retries as it shares some concerns with the UI becoming cluttered with potentially unimportant information.

[bitsofinfo]

also agree with the note in the op about non-firing conditions being flagged as the color red. Makes it a bit misleading.

different icons or "look" for conditions to differentiate?

[bitsofinfo]

any rough eta when this feature might be out? everyone on my team that sees the dashboard, gets immediately confused by the red/green, where the red is just "conditions" "failures"

[a-roberts]

One thing to note, I believe this is the main issue we're using for tracking this, @AlanGreene correct me if I'm wrong, this was pointed out on the working group call today when I gave our update:

Quoting @dibyom
Conditions and condition checks are eventually going away in TEP-007: https://github.com/tektoncd/community/blob/master/teps/0007-conditions-beta.md

So unfortunately for @bitsofinfo this may not be something resolved quickly - totally agree with your points too!

@eddycharly @steveodonovan FYI as well

[AlanGreene]

@a-roberts yes this is the right one.

I had been planning to move conditions checks and retries into their owning TaskRun in the UI to reduce the clutter in the TaskTree component, but with Conditions going away and custom Tasks possibly supporting sub-pipelines we may need to revisit this and come up with a different design.

I'll review the colour option as a short-term solution to reduce confusion, here's a rough mockup of some related changes I had in mind to tone down the heavy block colours which should help too:

That design would also address some feedback we've received from multiple users about wanting to easily view status of steps in multiple TaskRuns in parallel as it would allow expanding multiple TaskRuns in the list. There are a few more things to work out there, but the colour change would be a nice improvement while we iron out the rest of the details.

[AlanGreene]

I think it would be a good idea to consider this along with #572 and #972 to ensure we provide a good experience and don't introduce too many different mechanisms for accessing errors / logs.

[AlanGreene]

The design update mentioned above is being tracked in #1775 and is currently in progress. Once the initial updates are merged we can start tackling the changes for retries. I would suggest holding off on making changes for Conditions since they're deprecated.

[AlanGreene]

From the Carbon color usage guide the most appropriate colour appears to be $support-03 which is used for warnings.

In Carbon's gray 10 theme this is $yellow-30 or #f1c21b
We'll need to test for colour contrast etc.

Alternative colours if this doesn't work out are:

• $yellow-20 (#fdd13a)
• $orange-40 (#ff832b) - seems a little heavy...

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[AlanGreene]

Many parts of this are still needed / wanted, in particular the improvements to display of retries. Freezing so it doesn't get auto-closed.

/lifecycle frozen

[wingyplus]

I've try to match on real UI (I test it by changing the color on dogfood site):

$yellow-30:

$yellow-20:

$orange-40:

In my opinion, the $yellow-30 looks greater than $yellow-20.The $yellow-20 is quite hard for me to read on the white background color.

Do you want to change the icon for this?

[AlanGreene]

If we're going with $yellow-30 we should use the design token $support-03 as this will ensure consistency if we ever change the underlying colour palette or if Carbon makes changes in future releases.

We should also test that it still looks OK in dark mode. This is not enabled by default yet as there are still a few improvements needed, but you can test it by setting a flag in localStorage via the browser console:
localStorage.setItem('tkn-theme', 'dark')

You can disable it by clearing localStorage: localStorage.clear(), or by setting the value back to the default: localStorage.setItem('tkn-theme', 'light')

[wingyplus]

If we're going with $yellow-30 we should use the design token $support-03 as this will ensure consistency if we ever change the underlying colour palette or if Carbon makes changes in future releases.

We should also test that it still looks OK in dark mode. This is not enabled by default yet as there are still a few improvements needed, but you can test it by setting a flag in localStorage via the browser console:
localStorage.setItem('tkn-theme', 'dark')

You can disable it by clearing localStorage: localStorage.clear(), or by setting the value back to the default: localStorage.setItem('tkn-theme', 'light')

Oh, I just know how to set the dark theme. 😆 Here's on dark mode:

$yellow-30 (or $support-03):

$yellow-20:

I think we can choose $support-03 which following the design system.

[AlanGreene]

Note to self: We should also consider potential impact on the graph view. Would when expressions be rendered separate from the guarded task or as some sort of additional metadata / visual annotation? Retries make sense to be treated as part of the task rather than cluttering the UI with additional fake TaskRuns as we do today.

[AlanGreene]

/area roadmap

[AlanGreene]

#3062 updates the way retries are displayed in the UI. Instead of each retry being displayed as if it were an independent TaskRun on the PipelineRun details page, they're now correctly collapsed into a single entry and an overflow menu is provided to switch between the retries if desired.

This provides a much clearer experience as a successful PipelineRun will show no red / error states by default.

It also allows for viewing logs of the retries on the TaskRun details page which were previously not available.

This change will be included in the upcoming v0.39.0 release due in the next ~1 week.