Create Run resources from PipelineTask references #3115
Conversation
tekton-robot
added
the
release-note
tekton-robot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/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.
|
|
The following is the coverage report on the affected files.
|
tekton-robot
added
the
needs-rebase
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
tekton-robot
removed
the
needs-rebase
tekton-robot
added
size/XL
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
imjasonh
added
the
do-not-merge/work-in-progress
tekton-robot
removed
the
do-not-merge/work-in-progress
|
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.
|
|
There are a couple of other TaskRun-related functions in pipelinerun.go that might need equivalents for Run. I think these are for corner case situations so it's not needed immediately but maybe just put TODOs to revisit them.
|
| TaskRun *v1beta1.TaskRun | ||
|
|
||
| // If the PipelineTask is a Custom Task, RunName and Run will be set. | ||
| RunName string |
There was a problem hiding this comment.
This is more of an FYI than a comment but I noticed that our changes overlap here quite a bit. Overall, I think our approach to resolving pipeline tasks could use a cleaning up since now its OCI images and CustomTasks but who knows what else we will need to resolve in the future.
There was a problem hiding this comment.
Yeah, absolutely! I was mostly trying to change as little as possible with this change, and try to keep track of cleanups for future PRs. If you have things in mind could you open an issue to help track them? I can add some smells I've noticed too.
Yeah I agree, AIUI in theory this shouldn't be necessary -- it's updating TaskRun statuses to match the PipelineRun's view, which shouldn't be necessary. It's also kind of gross that TaskRuns are given names before they're ever run, so
This also seems like a smell to me, we're supposed to be enqueueing PipelineRun reconciliations when TaskRun statuses change already, as well as doing periodic reconciliations in case we miss something. (This was introduced in #2573 for context) I'll add a TODO in both cases, but before I do them I'll make sure it's actually necessary at all. |
imjasonh
force-pushed
the
pipelinerun
branch
from
1360d36 to
9a604f0
Compare
|
The following is the coverage report on the affected files.
|
imjasonh
force-pushed
the
pipelinerun
branch
from
9a604f0 to
b74d346
Compare
|
The following is the coverage report on the affected files.
|
imjasonh
force-pushed
the
pipelinerun
branch
from
b74d346 to
0b4f7ee
Compare
|
The following is the coverage report on the affected files.
|
|
@imjasonh: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
1 similar comment
|
@imjasonh: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
| // PipelineTaskName is the name of the PipelineTask. | ||
| PipelineTaskName string `json:"pipelineTaskName,omitempty"` | ||
|
|
||
| // TODO(#3133): Add v1alpha1.RunStatus here, without introducing an import cycle. |
There was a problem hiding this comment.
Is it a TODO for this PR or a follow-up ?
One way would be to do like pipeline resources, aka extracting the run v1alpha1 go api in its own package.
| err := c.reconcile(ctx, pr) | ||
| if err != nil { |
There was a problem hiding this comment.
Any reason for not doing if err := c.reconcile(ctx, pr); err != nil { instead ?
| if t.TaskRun == nil { | ||
| return false | ||
| } | ||
| return t.TaskRun.IsSuccessful() |
| if _, ok := candidateTasks[t.PipelineTask.Name]; ok && t.TaskRun == nil && t.Run == nil { | ||
| tasks = append(tasks, t) | ||
| } | ||
| if _, ok := candidateTasks[t.PipelineTask.Name]; ok && t.TaskRun != nil { | ||
| status := t.TaskRun.Status.GetCondition(apis.ConditionSucceeded) | ||
| if _, ok := candidateTasks[t.PipelineTask.Name]; ok { |
There was a problem hiding this comment.
I find the following easier to follow that current code.
if _, ok := candidateTasks[t.PipelineTask.Name]; ok {
if t.TaskRun == nil && t.Run == nil {
tasks = append(tasks, t)
}
// […]
}or at least, extracting _, ok := candidateTasks[t.PipelineTask.Name] into its own variable and using it in the if or a switch/case 🙃
| _ "k8s.io/client-go/plugin/pkg/client/auth/oidc" | ||
| knativetest "knative.dev/pkg/test" | ||
| "knative.dev/pkg/test/logging" // Mysteriously by k8s libs, or they fail to create `KubeClient`s from config. Apparently just importing it is enough. @_@ side effects @_@. https://github.com/kubernetes/client-go/issues/242 |
There was a problem hiding this comment.
This comment seems misplaced — same goes for the one above.. I wonder how it passes the linter 🤔
tekton-robot
added
the
needs-rebase
|
@imjasonh: PR needs rebase. 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. |
|
Closing in favor of #3463 |
Changes
Progress toward #3133
This change allows
TaskRefs inPipelineTasks to refer to a non-Tekton ("custom") task type. When such a Pipeline is run, the controller will create a newRunresource that embeds that reference, at which point a custom task controller can take over and begin reconciling that execution, eventually to completion.Followups for future PRs:
PipelineRunStatusis not yet surfaced in thePipelineRunStatussince it currently introduces an import cycle 💣 -- need to moveRunStatustov1beta1even thoughRunis still inv1alpha1.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