Add support for more cloud events #2677
Conversation
tekton-robot
added
the
do-not-merge/work-in-progress
tekton-robot
added
the
size/M
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
2 similar comments
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
afrittoli
force-pushed
the
issues/2082-cloudevents
branch
from
17d8b41
to
252c038
Compare
tekton-robot
added
size/L
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
afrittoli
force-pushed
the
issues/2082-cloudevents
branch
from
252c038
to
705c84c
Compare
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
afrittoli
changed the title
tekton-robot
removed
the
do-not-merge/work-in-progress
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
The following is the coverage report on the affected files.
|
|
/kind feature |
tekton-robot
added
the
kind/feature
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
afrittoli
force-pushed
the
issues/2082-cloudevents
branch
from
474b2bb
to
a95b740
Compare
|
The following is the coverage report on the affected files.
|
afrittoli
force-pushed
the
issues/2082-cloudevents
branch
from
a95b740
to
f879a57
Compare
tekton-robot
added
size/XL
|
The following is the coverage report on the affected files.
|
afrittoli
force-pushed
the
issues/2082-cloudevents
branch
from
f879a57
to
c459fa5
Compare
|
The following is the coverage report on the affected files.
|
afrittoli
force-pushed
the
issues/2082-cloudevents
branch
from
c459fa5
to
cf79a9b
Compare
The start event requires #2545 :) |
afrittoli
force-pushed
the
issues/2082-cloudevents
branch
from
ac439fb
to
613c995
Compare
|
The following is the coverage report on the affected files.
|
afrittoli
force-pushed
the
issues/2082-cloudevents
branch
from
613c995
to
ced568e
Compare
|
The following is the coverage report on the affected files.
|
afrittoli
force-pushed
the
issues/2082-cloudevents
branch
from
ced568e
to
49ac756
Compare
|
The following is the coverage report on the affected files.
|
afrittoli
force-pushed
the
issues/2082-cloudevents
branch
from
49ac756
to
b13c277
Compare
|
The following is the coverage report on the affected files.
|
|
Rebased and addressed comments @pritidesai @vdemeester |
|
/lgtm cancel |
|
In response to this:
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlpettersson, 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:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
Change the event type string for taskruns from dev.tekton.event.task.* to dev.tekton.event.taskrun.*. Since cloud events are only sent by pipeline resources - which are alpha - we don't need to comply with the beta deprecation policy for this. Extend the cloudevents module to include more event types for TaskRuns and event types for PipelineRuns, with unit test coverage. This is achieved by introducing the RunsToCompletion and RunsToCompletionStatus interface which are met by TaskRun, PipelineRun and their respective status types. At this stage there is no event producer that generates these new events. This is preparing the stage for the next changes where we will start to emit cloud events in addition to the k8s events we emit today. Partially addresses tektoncd#2082 Partially addresses tektoncd#2684 Signed-off-by: Andrea Frittoli <[email protected]>
afrittoli
force-pushed
the
issues/2082-cloudevents
branch
from
b13c277
to
f3f418d
Compare
|
yet another rebase :) |
|
The following is the coverage report on the affected files.
|
TaskRuns and PipelineRuns use the "Reason" field to complement the value of the "Succeeded" condition. Those values are not part of the API and are even owned by the underlying resource (pod) in case of TaskRuns. This makes it difficult to rely on them to understand that the state of the resource is. In case of corev1.ConditionTrue, the reason can be used to distinguish between: - Successful - Successful, some parts were skipped (pipelinerun only) In case of corev1.ConditionFalse, the reason can be used to distinguish between: - Failed - Failed because of timeout - Failed because of cancelled by the user In case of corev1.ConditionUnknown, the reason can be used to distinguish between: - Just started reconciling - Validation done, running (or still running) - Cancellation requested This is implemented through the following changes: - Bubble-up reasons for taskrun and pipelinerun to the v1beta1 API, except for reason that are defined by the underlying resource - Enforce the start reason to be set during condition init This allows for an additional change in the eventing module: the condition before and after can be used to decide whether to send an event at all. If they are different, the after condition now contains enough information to send the event. The cloudevent module is extended with ability to send the correct event based on both status and reason.
afrittoli
force-pushed
the
issues/2082-cloudevents
branch
from
f3f418d
to
84fbdb4
Compare
|
The following is the coverage report on the affected files.
|
|
thanks @afrittoli, it looks great 🙏 |
Changes
This PR is split into two commits, to help with the review.
Add support for more cloud events
Change the event type string for taskruns from dev.tekton.event.task.*
to dev.tekton.event.taskrun.*. Since cloud events are only sent by
pipeline resources - which are alpha - we don't need to comply with
the beta deprecation policy for this.
Extend the cloudevents module to include more event types for TaskRuns
and event types for PipelineRuns, with unit test coverage.
This is achieved by introducing the RunsToCompletion and
RunsToCompletionStatus interface which are met by TaskRun, PipelineRun
and their respective status types.
At this stage there is no event producer that generates these new
events. This is preparing the stage for the next changes where
we will start to emit cloud events in addition to the k8s events
we emit today.
No extra docs yet since the new events are not produced by anything yet.
Partially addresses #2082
Partially addresses #2684
The second:
Make phase condition reasons part of the API
TaskRuns and PipelineRuns use the "Reason" field to complement the
value of the "Succeeded" condition. Those values are not part of
the API and are even owned by the underlying resource (pod) in
case of TaskRuns. This makes it difficult to rely on them to
understand that the state of the resource is.
In case of corev1.ConditionTrue, the reason can be used to
distinguish between:
In case of corev1.ConditionFalse, the reason can be used to
distinguish between:
In case of corev1.ConditionUnknown, the reason can be used to
distinguish between:
This is implemented through the following changes:
v1beta1 API, except for reason that are defined by the
underlying resource
This allows for an additional change in the eventing module: the
condition before and after can be used to decide whether to send
an event at all. If they are different, the after condition now
contains enough information to send the event.
The cloudevent module is extended with ability to send the correct
event based on both status and reason.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe 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