TEP-0061, Allow custom task to be embedded. #3901

ScrapCodes · 2021-04-28T12:47:31Z

Changes

TEP-0061, Allow custom task to be embedded

API changes, This PR adds new APIs i.e. adds a field Spec *EmbeddedRunSpec to RunSpec
An embedded task will accept new field Spec with type runtime.RawExtension in addition to ApiVersion and Kind fields of type string (as part of runtime.TypeMeta) :
Validation changes, in addition to adding support for Run.RunSpec.Spec the validations will be changed to support "One of Run.RunSpec.Spec or Run.RunSpec.Ref" only and not both as part of a single API request to kubernetes.
Unit tests + e2e test to cover the newly added fields and above mentioned validations.
Documentation changes associated with TEP-0061.
References:
TEP-0061 Allow custom task to be embedded in pipeline, design. community#415
Custom Tasks: question with taskSpec #3682
https://github.com/tektoncd/community/blob/main/teps/0061-allow-custom-task-to-be-embedded-in-pipeline.md

@Tomcli
@pritidesai
@afrittoli
@animeshsingh

/cc @sbwsg @imjasonh

/kind feature

Closes #3682

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes


# It is now possible to embed the spec of a custom task in a Run resource, whether stand-alone or embedded in a Pipeline.

e.g.

          apiVersion: tekton.dev/v1alpha1
          kind: Run
          metadata:
            name: simpletasklooprun
          spec:
            params:
              - name: word
                value:
                  - jump
                  - land
              - name: suffix
                value: ing
            spec:
              apiVersion: custom.tekton.dev/v1alpha1
              kind: TaskLoop
              spec:
                # This is a new field, which comprises of custom-task spec.
                taskRef:
                  name: simpletask
                iterateParam: word
                timeout: 60s
                retries: 2


1. API changes, This PR adds new APIs i.e. adds a field `Spec *EmbeddedRunSpec` to `RunSpec`
2. An embedded task will accepts new field `Spec` with type `runtime.RawExtension` in addition to
    `ApiVersion` and `Kind` fields of type string (as part of `runtime.TypeMeta`) 
3. Validation changes, in addition to adding support for `Run.RunSpec.Spec` the validations will be changed
    to support "One of `Run.RunSpec.Spec` or `Run.RunSpec.Ref` " only and not both as part of a single
    API request to kubernetes.

action required: Developers of custom controllers (existing and new), who would like to support
 embedded spec for their custom task, need to implement the logic required to extract, validate
 and use the custom task spec from the new RunSpec.Spec field. Please review the documentation
 on upgrading, for more details and some examples.

tekton-robot · 2021-04-28T12:47:47Z

Hi @ScrapCodes. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

pritidesai · 2021-04-29T16:40:56Z

/ok-to-test

tekton-robot · 2021-04-29T16:42:58Z

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_validation.go	87.5%	90.9%	3.4
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.6%	95.9%	0.3
pkg/controller/filter.go	95.5%	97.1%	1.6
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	83.8%	-0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.1%	92.2%	0.1

Tomcli · 2021-04-29T16:58:38Z

@ScrapCodes looks like you haven't run hack/update-codegen.sh to regenerate the api code

afrittoli · 2021-04-30T09:45:10Z

Changes

##TEP-0061 (1/3), Allow custom task to be embedded.
    This is first in the three part series of PRs for implementing the TEP-0061.
API changes, This PR adds new APIs i.e. adds a field Spec *v1beta1.EmbeddedTask to RunSpec

An embedded task will accept new field Spec with type runtime.RawExtension in addition to ApiVersion and Kind fields of type string (as part of runtime.TypeMeta) :

Validation changes, in addition to adding support for Run.RunSpec.Spec the validations will be changed to support "One of Run.RunSpec.Spec or Run.RunSpec.Ref" only and not both as part of a single API request to kubernetes.

Unit tests to cover the newly added fields and above mentioned validations.

(2/3) Will consist of an e2e test.

To be honest, I would feel more comfortable having the E2E test part of this PR.
The PR is not too large, so it would be ok to have all in one go in fact.

(3/3) Documentation changes associated with TEP-0061.

Again, I would prefer to have this in the same PR, but I'm ok with having it as a follow-up, as long as we merge both PR as part of the same release.

/kind feature

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing

Tests included if any functionality added or changed

Follows the commit message standard

Meets the Tekton contributor standards (including
functionality, content, code)

Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes
 1. API changes, This PR adds new APIs i.e. adds a field `Spec *v1beta1.EmbeddedTask` to `RunSpec`
 2. An embedded task will accepts new field `Spec` with type `runtime.RawExtension` in addition to
     `ApiVersion` and `Kind` fields of type string (as part of `runtime.TypeMeta`) 
 3. Validation changes, in addition to adding support for `Run.RunSpec.Spec` the validations will be changed
     to support "One of `Run.RunSpec.Spec` or `Run.RunSpec.Ref` " only and not both as part of a single
    API request to kubernetes.

NIT: Could you add a more high level description on top or the release notes, something along the lines of
"It is now possible to embed the spec of a custom task in a Run resource, whether stand-alone or embedded in a Pipeline.
[include a yaml example]. Embedding the spec of a custom task depends on this being supported in each custom task controller that wants to support this feature."

action required: For developers of custom controllers (existing and new), now a `RunSpec`
 can have a `Ref` or `Spec`, Scenario, where custom controller supports either or both `Ref`
 and `Spec`. Please update the validation logic so that it responds with proper validation errors
 or response to the user for missing a `Ref` and or `Spec`.

Could you keep this block ^^^ together with the release notes? Having two blocks might break the release notes automation.

I think it's a good idea to make a call for action, but we could say that "Developers of custom controllers (existing and new), who would like to support embedded spec for their custom task, need to implement the logic required to extract, validate and use the custom task spec from the new RunSpec.Spec field. "

As discussed during the API WG and noted by @imjasonh - having validation of the spec via a validating webhook would mean that all validating webhook of all custom tasks would be invoked for each Run - which might be too much. Perhaps we should recommend async validation during the reconcile cycle instead for now?

References:

tektoncd/community#415

Custom Tasks: question with taskSpec #3682

https://github.com/tektoncd/community/blob/main/teps/0061-allow-custom-task-to-be-embedded-in-pipeline.md

@Tomcli
@pritidesai
@afrittoli
@animeshsingh

/cc @sbwsg @imjasonh

afrittoli · 2021-04-30T09:46:48Z

Thanks a lot @ScrapCodes for this, I look forward to this feature! I will review the code today.
We will have release v0.24 next week (it's planned for the 5th), do you think we include this in the milestone for the release?

ScrapCodes · 2021-04-30T12:00:03Z

@ScrapCodes: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Looks like this robot does not interpret nested backticks. So, removed them.

pritidesai · 2021-05-24T22:54:09Z

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -765,6 +767,22 @@ func (c *Reconciler) createRun(ctx context.Context, rprt *resources.ResolvedPipe
 		},
 	}

+	if rprt.PipelineTask.TaskSpec != nil {
+		j, err := json.Marshal(rprt.PipelineTask.TaskSpec.Spec)


what happens when spec is empty here? do we need a check here rprt.PipelineTask.TaskSpec.Spec?

there is no error for empty spec, in fact a non empty byte sequence is generated. Do you think we should be more defensive here?

I am trying to think if there is any use case where the spec could be empty, could this ever be used in any way?

Since this is not producing any error, its safe to go with the way its implemented now and revisit if needed in future.

pritidesai · 2021-05-24T22:57:29Z

the changes looks great @ScrapCodes. I am done reviewing the PR and have requested couple of changes. 🙏

One more thing I am not sure about is all the third party changes, without changing the mod file, how come we are seeing the dependency change 🤔

tekton-robot · 2021-05-25T07:31:22Z

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_validation.go	87.5%	95.5%	8.0
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.6%	95.9%	0.3
pkg/controller/filter.go	95.5%	97.1%	1.6
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	83.8%	-0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.1%	92.2%	0.1

ScrapCodes · 2021-05-25T07:35:08Z

the changes looks great @ScrapCodes. I am done reviewing the PR and have requested couple of changes. 🙏

One more thing I am not sure about is all the third party changes, without changing the mod file, how come we are seeing the dependency change 🤔

I am not sure either, they are generated as part of running, hack/update-codegen.sh.

tekton-robot · 2021-05-25T07:41:22Z

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_validation.go	87.5%	95.5%	8.0
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.6%	95.9%	0.3
pkg/controller/filter.go	95.5%	97.1%	1.6
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	83.8%	-0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.1%	92.2%	0.1

It is now possible to embed the spec of a custom task in a Run resource, whether stand-alone in Run or embedded in a Pipeline. e.g. in a Run using example TaskLoop controller in tektoncd/experimental/task-loop/ apiVersion: tekton.dev/v1alpha1 kind: Run metadata: name: simpletasklooprun spec: params: - name: word value: - jump - land - name: suffix value: ing spec: apiVersion: custom.tekton.dev/v1alpha1 kind: TaskLoop spec: # This is a new field, which comprises of custom-task spec. taskRef: name: simpletask iterateParam: word timeout: 60s retries: 2 1. API changes, This PR adds new APIs i.e. adds a field `Spec EmbeddedRunSpec` to `RunSpec` 2. An embedded task will accepts new field `Spec` with type `runtime.RawExtension` in addition to `ApiVersion` and `Kind` fields of type string (as part of `runtime.TypeMeta`) 3. Validation changes, in addition to adding support for `Run.RunSpec.Spec` the validations will be changed to support "One of `Run.RunSpec.Spec` or `Run.RunSpec.Ref` " only and not both as part of a single API request to kubernetes. Developers of custom controllers (existing and new), who would like to support embedded spec for their custom task, need to implement the logic required to extract, validate and use the custom task spec from the new RunSpec.Spec field. Please review the documentation in docs/runs.md and docs/pipelines.md

tekton-robot · 2021-05-25T08:41:21Z

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_validation.go	87.5%	95.5%	8.0
pkg/apis/pipeline/v1beta1/pipeline_types.go	95.6%	95.9%	0.3
pkg/controller/filter.go	95.5%	97.1%	1.6
pkg/reconciler/pipelinerun/pipelinerun.go	83.8%	83.8%	-0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.1%	92.2%	0.1

ScrapCodes · 2021-05-25T08:53:09Z

Looks like it is a problem with my local environment. Even after removing those third_party additions, build passes i.e. build passes regardless of those third_party additions are present or not. Is this concerning from the point of view, how build is setup.

ScrapCodes · 2021-05-25T09:16:42Z

Thank you @pritidesai for taking a look. Please take a look again.

Thank you @sbwsg !

Thank you @animeshsingh and @Tomcli for all the support 😄

pritidesai · 2021-05-25T19:10:55Z

thank you for all the changes, please address the validation in a follow up PR.

I have updated the PR description to include the issue number and an attempt to fix release-tag.

/lgtm

Tomcli · 2021-05-25T19:15:56Z

thanks @pritidesai for your review

ScrapCodes · 2021-05-26T11:31:28Z

Thank @pritidesai, for correcting me on this.

I have opened a follow-up PR with the validation: #3977

We already designed and implemented support for `Timeouts` and `TaskSpec` in `Custom Tasks`. `Timeouts`: - TEP: tektoncd/community#433 - PR: tektoncd#3976 `TaskSpec`: - TEP: https://github.com/tektoncd/community/blob/main/teps/0061-allow-custom-task-to-be-embedded-in-pipeline.md - PR: tektoncd#3901 This item cleans up the todo items in `RunSpec`.

We already designed and implemented support for `Timeouts` and `TaskSpec` in `Custom Tasks`. `Timeouts`: - TEP: tektoncd/community#433 - PR: #3976 `TaskSpec`: - TEP: https://github.com/tektoncd/community/blob/main/teps/0061-allow-custom-task-to-be-embedded-in-pipeline.md - PR: #3901 This item cleans up the todo items in `RunSpec`.

We already designed and implemented support for `Timeouts` and `TaskSpec` in `Custom Tasks`. `Timeouts`: - TEP: tektoncd/community#433 - PR: tektoncd#3976 `TaskSpec`: - TEP: https://github.com/tektoncd/community/blob/main/teps/0061-allow-custom-task-to-be-embedded-in-pipeline.md - PR: tektoncd#3901 This item cleans up the todo items in `RunSpec`.

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 28, 2021

tekton-robot requested review from bobcatfish and dlorenc April 28, 2021 12:47

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 28, 2021

ScrapCodes force-pushed the implementation_tep_61 branch 2 times, most recently from 967685d to 716d12a Compare April 28, 2021 13:37

ScrapCodes changed the title ~~TEP-0061 Implementation Part(1/3), Allow custom task to be embedded in PipelineRun.~~ TEP-0061 (1/3), Allow custom task to be embedded. Apr 28, 2021

ScrapCodes marked this pull request as draft April 28, 2021 13:50

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 29, 2021

ScrapCodes marked this pull request as ready for review April 29, 2021 05:26

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 29, 2021

ScrapCodes force-pushed the implementation_tep_61 branch from 716d12a to d06d8dd Compare April 29, 2021 06:30

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 29, 2021

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 29, 2021

ScrapCodes changed the title ~~TEP-0061 (1/3), Allow custom task to be embedded.~~ TEP-0061 (2/2), Allow custom task to be embedded. Apr 30, 2021

pritidesai reviewed May 24, 2021

View reviewed changes

ScrapCodes force-pushed the implementation_tep_61 branch from 4dc118d to 6fbe435 Compare May 25, 2021 07:29

ScrapCodes force-pushed the implementation_tep_61 branch from 6fbe435 to a2d151e Compare May 25, 2021 07:39

ScrapCodes added 4 commits May 25, 2021 13:53

TEP-0061, Added documentation for embedding spec in custom-task

dcdfe07

TEP-0061, updated custom-task e2e to test embedded custom task

239f98c

Ran hack/update-codegen.sh

4e7a9ff

ScrapCodes force-pushed the implementation_tep_61 branch from a2d151e to 4e7a9ff Compare May 25, 2021 08:39

tekton-robot assigned pritidesai May 25, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 25, 2021

tekton-robot merged commit f911d73 into tektoncd:main May 25, 2021

ScrapCodes mentioned this pull request May 26, 2021

Validate run for both ref and spec as nil. #3977

Merged

5 tasks

ScrapCodes deleted the implementation_tep_61 branch May 26, 2021 11:32

Tomcli mentioned this pull request May 28, 2021

feat(pipelineloop) : Support latest tektoncd pipeline version. kubeflow/kfp-tekton#602

Merged

2 tasks

This was referenced Jan 13, 2022

Timeout and TaskSpec supported in Custom Tasks jerop/pipeline#2

Closed

Timeout and TaskSpec supported in Custom Tasks #4481

Merged

vdemeester mentioned this pull request Feb 3, 2022

Pipelines don't respect mixed Task API versions #4543

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TEP-0061, Allow custom task to be embedded. #3901

TEP-0061, Allow custom task to be embedded. #3901

ScrapCodes commented Apr 28, 2021 •

edited by pritidesai

Loading

tekton-robot commented Apr 28, 2021

pritidesai commented Apr 29, 2021

tekton-robot commented Apr 29, 2021

Tomcli commented Apr 29, 2021

afrittoli commented Apr 30, 2021

Changes

Submitter Checklist

Release Notes

afrittoli commented Apr 30, 2021

ScrapCodes commented Apr 30, 2021

pritidesai May 24, 2021

ScrapCodes May 25, 2021

pritidesai May 25, 2021

pritidesai commented May 24, 2021

tekton-robot commented May 25, 2021

ScrapCodes commented May 25, 2021

tekton-robot commented May 25, 2021

tekton-robot commented May 25, 2021

ScrapCodes commented May 25, 2021

ScrapCodes commented May 25, 2021

pritidesai commented May 25, 2021

Tomcli commented May 25, 2021

ScrapCodes commented May 26, 2021 •

edited

Loading

TEP-0061, Allow custom task to be embedded. #3901

TEP-0061, Allow custom task to be embedded. #3901

Conversation

ScrapCodes commented Apr 28, 2021 • edited by pritidesai Loading

Changes