Refining status when Condition failed

[vincent-pli]

Introduce new state: SucceededWithConditionCheckFailed, indicates the scenario pipeline succeeded but with Condition checking failed.
fix issue: #1692

Changes

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.

Double check this list of stuff that's easy to miss:

• If you are adding a new binary/image to the cmd dir, please update
  the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Status messages for completed and running PipelineRuns have been updated to include the count of succeeded, skipped, and incomplete Tasks. Additionally a new status Reason, "Completed", has been introduced for finished PipelineRuns that have skipped Tasks due to Condition check failures.

[tekton-robot]

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.7%	94.9%	0.2

[vincent-pli]

/retest

[afrittoli]

Thank you for this patch, and sorry I'm a bit late in the discussion.
While it could be interesting to signal that a pipeline did not execute any task, I think SucceededWithConditionCheckFailed does not express this information. I could have a pipeline with several conditions, some passed, some failed, which could be successful or failed. Perhaps SucceededWithNoTaskRuns would represent better the situation?
Note that it is already possible to know that no TaskRun was executed, so this would only make it slightly easier to discover. I'm not entirely convinced it is needed though. Do you have specific use cases in mind to explain why this is required?

[vincent-pli]

@afrittoli thanks for comments :)
The SucceededWithConditionCheckFailed indicate that there is at least one condition checking failed in the whole pipelinerun. Just a reminder for user to hint there is something abnormal occurred to avoid misleading.

SucceededWithNoTaskRuns express there is no task really executed since condition checking failed.
I think just a reminder is enough for end user.

[dibyom]

So, we have a Reason and a Message field. I think what could be confusing for the user is the Message saying All Tasks have completed executing when no tasks have been executed. So, my two cents would be to use a different message like No Tasks left to execute or something and keep the Reason the same as before.

[imjasonh]

I think @dibyom is right that we can/should use the Message field better, whether or not these different states end up having different Reasons.

I like the Reason being no more tasks to run because that's both true, and meaningfully vague. 😄

The Message then could be completed 10 tasks, 3 were not executed due to conditions (or something along those lines). This could even be useful when the PipelineRun is ongoing (completed 6 tasks, running 2 tasks, 1 skipped due to conditions). This of course is something a user should also be able to determine from taskrun statuses, but it's nice to have a human-readable summary, and it should be fairly straightforward to generate it.

[vincent-pli]

Could we just use Completed for reason rather than Succeeded
then we have message for both Complete and Running:
Completed 6 tasks, not start/running 2 tasks, skipped 1 tasks
@afrittoli @dibyom @imjasonh

[dibyom]

Could we just use Completed for reason rather than Succeeded

SGTM especially since condition.type: Succeeded already. What do others think -- @imjasonh @vdemeester @bobcatfish ?

[ghost]

Hey @vincent-pli do you intend to continue working on this or should we close for now? For what it's worth it sounds like we have consensus on the desired implementation:

Could we just use Completed for reason rather than Succeeded
then we have message for both Complete and Running:
Completed 6 tasks, not start/running 2 tasks, skipped 1 tasks

SGTM2

[vincent-pli]

@sbwsg
Sure, I will work on it soon.

[vincent-pli]

/assign @sbwsg

[ghost]

This looks good to me. I've made a suggestion to make the format strings more uniform. Let me know what you think. But otherwise I think this is ready.

Is this a backwards compatibility change? Do we need to give it extra highlight in release notes?

[vincent-pli]

@sbwsg For the release, I think I agree with you.

[ghost]

For the release, I think I agree with you.

OK cool, I suggest updating the Release Notes on this PR's description to be something like:

Status messages for completed and running PipelineRuns have been updated to include the count of succeeded, skipped, and incomplete Tasks. Additionally a new status Reason, "Completed", has been introduced for finished PipelineRuns that have skipped Tasks due to Condition check failures.

Then I think this is ready to merge!

[vincent-pli]

@sbwsg
Done, thanks

[vdemeester]

/lgtm

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

• OWNERS [sbwsg,vdemeester]

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