-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add CloudEvents for Run definitions #4659
Conversation
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/test pull-tekton-pipeline-integration-tests |
The following is the coverage report on the affected files.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
/test pull-tekton-pipeline-alpha-integration-tests |
1 similar comment
/test pull-tekton-pipeline-alpha-integration-tests |
@@ -6,11 +6,12 @@ weight: 700 | |||
--> | |||
# Events in Tekton | |||
|
|||
Tekton's task controller emits [Kubernetes events](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#event-v1-core) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some grammar nits: controllers emits
-> controllers emit
and the underlying TaskRun do
-> the underlying TaskRuns do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, thank you!
@@ -68,6 +69,12 @@ Resource |Event |Event Type | |||
`PipelineRun` | `Condition Change while Running` | `dev.tekton.event.pipelinerun.unknown.v1` | |||
`PipelineRun` | `Succeed` | `dev.tekton.event.pipelinerun.successful.v1` | |||
`PipelineRun` | `Failed` | `dev.tekton.event.pipelinerun.failed.v1` | |||
`Run` | `Started` | `dev.tekton.event.run.started.v1` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason not to send a condition change while running event? is this because we don't know what the run's previous status was, and can't make assumptions based on what it currently is? Is this a feature users might want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot make assumptions about the Run
and it will use its Conditions
beyond what we specify in the custom task interface. So we can see that the resource has been created (start), we can see that an initial condition was set (running) and we can see when it's finished (successfully or not).
}, | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: runName, | ||
Namespace: "marshmallow", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
|
||
for _, c := range runTests { | ||
t.Run(c.desc, func(t *testing.T) { | ||
names.TestingSeed() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see names.TestingSeed
a lot in our tests-- what's the reason it's needed here, just out of curiosity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy paste? I wondered the same but I lazy :P I will try to find out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its there for a reason, I have been wondering about it as well since I started writing unit tests.
It was introduced to use fixed
seed for the generated names as part of PR #491. This PR fixed the issue where it was reported the resource names were too long #481.
@afrittoli some of these might no longer be needed since now we have fixed names for some of the resources as your implemented in #4361.
I generally comment out one line at a time to see if its breaking my test if I am not sure what that code is doing 😄 Or add the same call twice 🙃 to understand the impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pritidesai - good idea :)
names.TestingSeed
is used to initialise the random generator used in names, so that you can get know what name to expect to validate results in tests. As @pritidesai mentioned, the need for this is probably reduced now that children resources have a more predictable name.
CloudEvent tests only match event types, so the name seed is not strictly required. It does cost anything or do any harm either, but I'll remove it as it's not needed.
Add the definitions for new CloudEvents for Runs. The events are not sent yet. The event types are documented and the cloudevent package supports creating events for Runs. Partially addresses tektoncd#3862 Signed-off-by: Andrea Frittoli <[email protected]>
The following is the coverage report on the affected files.
|
/test pull-tekton-pipeline-alpha-integration-tests |
/lgtm |
Changes
Add CloudEvents for Run definitions
Add the definitions for new CloudEvents for Runs.
The events are not sent yet. The event types are documented and
the cloudevent package supports creating events for Runs.
Partially addresses #3862
Depends-on: #4658
/kind feature
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes