Skip to content
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

Integrate custom tasks into Pipelines #3463

Merged
merged 2 commits into from
Dec 4, 2020

Conversation

GregDritschler
Copy link
Contributor

@GregDritschler GregDritschler commented Oct 27, 2020

Changes

A Pipeline Task now can reference a custom task. This causes the PipelineRun reconciler to create a Run instead of a TaskRun.
Pipeline parameters can be passed to the custom task. The custom task's results can be used by other Pipeline tasks and by the Pipeline results.

/kind feature

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
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

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

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

In package pkg/apis/pipeline/v1alpha1, the type of field RunStatusFields.Results has changed from
v1beta1.TaskRunResult to v1alpha1.RunResult.

@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Oct 27, 2020
@tekton-robot tekton-robot requested review from dlorenc and a user October 27, 2020 14:02
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 27, 2020
@GregDritschler
Copy link
Contributor Author

@imjasonh @vdemeester This PR supercedes Jason's original PR #3115. I made the following changes:

  • Rebased to latest master.
  • Resolved most of the TODOs.
  • Resolved the import cycle problem getting the v1beta1 PipelineRun status to include the v1alpha1 Run status.
  • Added code to propagate PR cancel request to Runs.
  • Added code to handle result references to results produced by custom tasks.
  • Improved unit test coverage for Runs.
  • Updated docs.

The main TODO which I did not address was to handle Conditions in pipeline tasks that use custom tasks. At the time of Jason's PR, I believe that "when" support was not yet merged. Now that "when" support is merged and Conditions are being deprecated, I would like to propose that Conditions NOT be supported in pipeline tasks that use custom tasks. If everyone agrees then I can update this PR to validate that restriction or I can do that in a follow-on PR.

Thanks in advance for your review.

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/run_types.go 22.7% 43.8% 21.0
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 80.3% 81.0% 0.6

Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, I was really looking forward to it!
I'll be digging into the PR in the next days!

Quick question, without having looked at the code: would it make sense to split the part that solves the import cycle in a dedicated initial PR to reduce the size of this one?

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/run_types.go 22.7% 43.8% 21.0
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 80.3% 81.0% 0.6

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/run_types.go 22.7% 43.8% 21.0
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 80.3% 81.0% 0.6

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move all Run related types to pkg/apis/run/v1alpha1 instead of just the one that were a problem for import cycle. This would allow us to still not import anything from "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" but "github.com/tektoncd/pipeline/pkg/apis/run/v1alpha1" instead — similar to what we do with pkg/apis/resource/v1alpha1.

pkg/apis/run/v1alpha1/runstatus_types.go Show resolved Hide resolved
@imjasonh
Copy link
Member

Thanks so much for doing this Greg! 🎉

Now that "when" support is merged and Conditions are being deprecated, I would like to propose that Conditions NOT be supported in pipeline tasks that use custom tasks.

This seems reasonable to me, so long as it's documented and produces a descriptive error message. I'd prefer it to happen in a followup PR though to keep this one relatively narrowly scoped.

@afrittoli
Copy link
Member

/test check-pr-has-kind-label

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/run_types.go 22.7% 43.8% 21.0
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 80.3% 81.0% 0.6

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/run_types.go 22.7% 43.8% 21.0
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 80.3% 81.0% 0.6

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/run_types.go 22.7% 43.8% 21.0
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 80.3% 81.0% 0.6

@afrittoli afrittoli added this to the Pipelines v0.18 milestone Oct 30, 2020
@dibyom dibyom mentioned this pull request Nov 2, 2020
@bobcatfish
Copy link
Collaborator

@GregDritschler could you fill in the checklist:

image

Discussed in OWNERS meeting today, sounds like merging this fixes #3133 (correct me if I'm wrong @imjasonh @GregDritschler !)

@GregDritschler
Copy link
Contributor Author

Discussed in OWNERS meeting today, sounds like merging this fixes #3133 (correct me if I'm wrong @imjasonh @GregDritschler !)

There are a few things left to do (as separate PRs since this one is already very big):

  • Disallow use of deprecated Conditions with custom tasks
  • Enable use of service account name and workspaces with custom tasks
  • Provide a sample custom task controller (I have started one)

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 72.4% 71.0% -1.4
pkg/apis/pipeline/v1alpha1/run_types.go 22.7% 43.8% 21.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go 99.5% 99.6% 0.0
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 75.0% 75.8% 0.8
pkg/reconciler/pipelinerun/cancel.go 94.1% 84.0% -10.1
pkg/reconciler/pipelinerun/pipelinerun.go 84.6% 82.6% -2.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 91.9% 92.0% 0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go 71.0% 64.1% -6.9
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 91.9% 91.0% -0.9
test/controller.go 25.8% 24.5% -1.3

@GregDritschler
Copy link
Contributor Author

I've opened issues for the follow-on work to this PR.

Custom Tasks: Disallow use of Conditions #3592
Custom Tasks: Allow use of workspaces #3593
Custom Tasks: Allow use of service accounts #3594
Custom Tasks: Support Pipeline Results #3595
Custom Tasks: Move all Run-related Go types to pkg/apis/run #3596

@vdemeester @afrittoli @pritidesai FYI. I believe I've resolved all the review issues. If not let me know. I'd like to get this one closed out.

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/meow
Let's get this in for 0.19 😝

@tekton-robot
Copy link
Collaborator

@vdemeester: cat image

In response to this:

/meow
Let's get this in for 0.19 😝

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.

@tekton-robot
Copy link
Collaborator

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2020
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates and for the great work on this!

There are a few more comments that we could add to into the code to track design decisions for posterity, but nothing blocking.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2020
@vdemeester
Copy link
Member

arf, @GregDritschler needs yet another rebase, sorry 😓 🙇

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2020
@tekton-robot tekton-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 4, 2020
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 72.4% 71.0% -1.4
pkg/apis/pipeline/v1alpha1/run_types.go 22.7% 43.8% 21.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go 99.5% 99.6% 0.0
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 74.6% 75.4% 0.8
pkg/reconciler/pipelinerun/cancel.go 94.1% 84.0% -10.1
pkg/reconciler/pipelinerun/pipelinerun.go 84.6% 82.6% -2.0
test/controller.go 25.8% 24.5% -1.3

A PipelineTask now can reference a custom task.  This causes the PipelineRun reconciler to create a Run instead of a TaskRun.
Pipeline parameters can be passed to the custom task.  The custom task's results can be used by other Pipeline tasks and by the Pipeline results.
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 72.4% 71.0% -1.4
pkg/apis/pipeline/v1alpha1/run_types.go 22.7% 43.8% 21.0
pkg/apis/pipeline/v1beta1/pipeline_validation.go 99.5% 99.6% 0.0
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 74.6% 75.4% 0.8
pkg/reconciler/pipelinerun/cancel.go 94.1% 84.0% -10.1
pkg/reconciler/pipelinerun/pipelinerun.go 84.6% 82.6% -2.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 91.9% 92.0% 0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go 71.0% 64.1% -6.9
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 92.6% 91.0% -1.6
test/controller.go 25.8% 24.5% -1.3

@GregDritschler
Copy link
Contributor Author

@afrittoli @vdemeester Rebase completed.

Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2020
@tekton-robot tekton-robot merged commit 75f3776 into tektoncd:master Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants