TEP-0114: Added CustomRun references and specifications to v1beta1 #5403

vsinghai · 2022-08-31T15:14:04Z

Changes

Prior to this commit, we proposed migrating
Custom Tasks and Runs to beta. Today, embedded specs
are in Run.Spec.Spec.

In this commit, we will align the embedded specs
with the rest of the API such that it's
CustomRun.Spec.CustomSpec.

Fixes issue #5154
Fixes issue #5153
/cc @jerop
/cc @dibyom

/kind feature

Submitter Checklist

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

[n/a] Has Docs included if any changes are user facing

Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
[n/a] Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

tekton-robot · 2022-08-31T15:14:14Z

Hi @vsinghai. 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.

dibyom · 2022-08-31T16:23:59Z

/ok-to-test

tekton-robot · 2022-08-31T16:28:33Z

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/v1beta1/customrun_types.go	Do not exist	0.0%

tekton-robot · 2022-09-01T15:10:57Z

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
pkg/apis/pipeline/v1beta1/customrun_defaults.go	Do not exist	0.0%
pkg/apis/pipeline/v1beta1/customrun_types.go	Do not exist	59.3%
pkg/apis/pipeline/v1beta1/customrun_validation.go	Do not exist	92.6%

pkg/apis/pipeline/v1beta1/customrun_types.go

tekton-robot · 2022-09-01T19:53:59Z

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
pkg/apis/pipeline/v1beta1/customrun_defaults.go	Do not exist	0.0%
pkg/apis/pipeline/v1beta1/customrun_types.go	Do not exist	59.3%
pkg/apis/pipeline/v1beta1/customrun_validation.go	Do not exist	92.6%

tekton-robot · 2022-09-02T15:03:27Z

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
pkg/apis/pipeline/v1beta1/customrun_defaults.go	Do not exist	0.0%
pkg/apis/pipeline/v1beta1/customrun_types.go	Do not exist	59.3%
pkg/apis/pipeline/v1beta1/customrun_validation.go	Do not exist	92.6%

tekton-robot · 2022-09-02T15:25:03Z

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
pkg/apis/pipeline/v1beta1/customrun_defaults.go	Do not exist	0.0%
pkg/apis/pipeline/v1beta1/customrun_types.go	Do not exist	59.3%
pkg/apis/pipeline/v1beta1/customrun_validation.go	Do not exist	92.6%

tekton-robot · 2022-09-02T15:43:14Z

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
pkg/apis/pipeline/v1beta1/customrun_defaults.go	Do not exist	0.0%
pkg/apis/pipeline/v1beta1/customrun_types.go	Do not exist	59.3%
pkg/apis/pipeline/v1beta1/customrun_validation.go	Do not exist	92.6%

vsinghai · 2022-09-06T18:25:14Z

Below are some problems and resolutions I encountered while creating this PR:

Problems:

./hack/update-codegen.sh kept creating a file docs/pipeline-api.md.backup that would mess up the CI testing
./hack/update-codegen.sh would add indents to register.go files for go versions that are run locally on a mac. This would cause build failures because the CI is running a different version of go and the diff between the two register.go files would vary.

Resolutions:

The issue was brought up in Redundant md.backup file by update-reference-docs.sh #5417 and resolved in Add docs/pipeline-api.md.backup to .gitignore #5418 . I also manually removed the docs/pipeline-api.md.backup file.
I manually removed the indents from the files.

@lbernick

In v1alpha1, `Runs` have `Pod` templates. However, `Runs` do not always create `Pods`. In this change, we propose that `Runs` in v1beta1 should not have `Pod` templates - specific `Custom Tasks` implementation can support that in their own specifications. Context: tektoncd/pipeline#5403 (comment) Many thanks to @lbernick for catching this issue!

XinruZhang · 2022-09-06T20:44:36Z

/assign

pkg/apis/pipeline/v1beta1/customrun_types_test.go

pkg/apis/pipeline/v1beta1/customrun_validation.go

pkg/apis/pipeline/v1beta1/customrun_validation_test.go

XinruZhang

As review goes, I can see this is such a large and important effort for Custom Task Beta. Appreciate your work here!

pkg/apis/pipeline/v1beta1/customrun_validation_test.go

pkg/apis/pipeline/v1beta1/swagger.json

XinruZhang · 2022-09-07T14:15:23Z

pkg/apis/pipeline/v1beta1/swagger.json

+        },
+        "metadata": {
+          "default": {},
+          "$ref": "#/definitions/v1beta1.PipelineTaskMetadata"


I'm not sure if this is correct? Why is it PipelineTaskMetadata? 🤔

it is correct, Metadata PipelineTaskMetadata json:"metadata,omitempty", found in customrun_types.go line 39. The swagger.json files are autogenerated files.

@jerop same here

@XinruZhang this is also another case of reusing the metadata type -- we can rename it to something more appropriate for all places that it's used but would rather leave it out of scope for this change

pkg/apis/run/v1beta1/customrunstatus_types.go

vsinghai · 2022-09-07T14:39:54Z

Thank you @XinruZhang for the feedback! :)

tekton-robot · 2022-09-07T15:31:36Z

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
pkg/apis/pipeline/v1beta1/customrun_defaults.go	Do not exist	0.0%
pkg/apis/pipeline/v1beta1/customrun_types.go	Do not exist	59.3%
pkg/apis/pipeline/v1beta1/customrun_validation.go	Do not exist	92.6%

pkg/apis/pipeline/v1beta1/customrun_types.go

tekton-robot · 2022-09-08T14:19: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
pkg/apis/pipeline/v1beta1/customrun_defaults.go	Do not exist	0.0%
pkg/apis/pipeline/v1beta1/customrun_types.go	Do not exist	59.3%
pkg/apis/pipeline/v1beta1/customrun_validation.go	Do not exist	92.6%

tekton-robot · 2022-09-08T15:18:48Z

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
pkg/apis/pipeline/v1beta1/customrun_defaults.go	Do not exist	0.0%
pkg/apis/pipeline/v1beta1/customrun_types.go	Do not exist	59.3%
pkg/apis/pipeline/v1beta1/customrun_validation.go	Do not exist	92.6%

@jerop

Prior to this commit, we proposed migrating `Custom Tasks` and `Runs` to beta. Today, [embedded specs](https://github.com/tektoncd/pipeline/blob/main/docs/runs.md#2-specifying-the-target-custom-task-by-embedding-its-spec) are in `Run.Spec.Spec`. In this commit, we will align the embedded specs with the rest of the API such that it's `CustomRun.Spec.CustomSpec`. Fixes issue [tektoncd#5154](tektoncd#5154) /cc @jerop /cc @dibyom /kind feature

tekton-robot · 2022-09-08T15:35:55Z

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
pkg/apis/pipeline/v1beta1/customrun_defaults.go	Do not exist	0.0%
pkg/apis/pipeline/v1beta1/customrun_types.go	Do not exist	59.3%
pkg/apis/pipeline/v1beta1/customrun_validation.go	Do not exist	92.6%

jerop

/lgtm

tekton-robot · 2022-09-08T16:05:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop, lbernick

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:

~~OWNERS~~ [jerop,lbernick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@lbernick

In v1alpha1, `Runs` have `Pod` templates. However, `Runs` do not always create `Pods`. In this change, we propose that `Runs` in v1beta1 should not have `Pod` templates - specific `Custom Tasks` implementation can support that in their own specifications. Context: tektoncd/pipeline#5403 (comment) Many thanks to @lbernick for catching this issue!

tekton-robot requested review from dibyom and jerop August 31, 2022 15:14

vsinghai marked this pull request as draft August 31, 2022 15:14

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 Aug 31, 2022

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 Aug 31, 2022

vsinghai force-pushed the references branch from 0b691ed to 8d237ed Compare September 1, 2022 14:58

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 1, 2022

vsinghai marked this pull request as ready for review September 1, 2022 15:06

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 Sep 1, 2022

lbernick reviewed Sep 1, 2022

View reviewed changes

pkg/apis/pipeline/v1beta1/customrun_types.go Outdated Show resolved Hide resolved

vsinghai force-pushed the references branch from 8d237ed to c1f6188 Compare September 1, 2022 19:48

vsinghai force-pushed the references branch from c1f6188 to 63b09d7 Compare September 2, 2022 14:57

vsinghai force-pushed the references branch 2 times, most recently from 22d3640 to e1c0422 Compare September 2, 2022 15:20

vsinghai force-pushed the references branch from e1c0422 to 4abdaea Compare September 2, 2022 15:37

vsinghai force-pushed the references branch from 4abdaea to 481e231 Compare September 2, 2022 16:01

jerop changed the title ~~Added CustomRun references and specifications to v1beta1~~ TEP-0114: Added CustomRun references and specifications to v1beta1 Sep 6, 2022

jerop assigned jerop and lbernick Sep 6, 2022

jerop mentioned this pull request Sep 6, 2022

TEP-0114: Custom Tasks Beta - Remove Pod Template tektoncd/community#815

Merged

tekton-robot assigned XinruZhang Sep 6, 2022

XinruZhang reviewed Sep 7, 2022

View reviewed changes

vsinghai force-pushed the references branch from 2e9c673 to 18c3603 Compare September 7, 2022 15:25

lbernick approved these changes Sep 8, 2022

View reviewed changes

pkg/apis/pipeline/v1beta1/customrun_types.go Outdated Show resolved Hide resolved

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 8, 2022

vsinghai force-pushed the references branch from 18c3603 to 9dce957 Compare September 8, 2022 14:14

vsinghai force-pushed the references branch from 9dce957 to 4aab123 Compare September 8, 2022 15:13

vsinghai force-pushed the references branch from 4aab123 to f6937bd Compare September 8, 2022 15:30

jerop approved these changes Sep 8, 2022

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2022

tekton-robot merged commit 04e1150 into tektoncd:main Sep 8, 2022

vsinghai deleted the references branch September 8, 2022 17:06

This was referenced Sep 12, 2022

TEP-0114: Custom Tasks Beta - Support v1beta1.CustomRun #5153

Closed

TEP-0114: Custom Tasks Beta - References and Specifications #5154

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TEP-0114: Added CustomRun references and specifications to v1beta1 #5403

TEP-0114: Added CustomRun references and specifications to v1beta1 #5403

vsinghai commented Aug 31, 2022 •

edited

Loading

tekton-robot commented Aug 31, 2022

dibyom commented Aug 31, 2022

tekton-robot commented Aug 31, 2022

tekton-robot commented Sep 1, 2022

tekton-robot commented Sep 1, 2022

tekton-robot commented Sep 2, 2022

tekton-robot commented Sep 2, 2022

tekton-robot commented Sep 2, 2022

vsinghai commented Sep 6, 2022 •

edited

Loading

XinruZhang commented Sep 6, 2022

XinruZhang left a comment

XinruZhang Sep 7, 2022

vsinghai Sep 7, 2022

XinruZhang Sep 7, 2022

jerop Sep 8, 2022

vsinghai commented Sep 7, 2022

tekton-robot commented Sep 7, 2022

tekton-robot commented Sep 8, 2022

tekton-robot commented Sep 8, 2022

tekton-robot commented Sep 8, 2022

jerop left a comment

tekton-robot commented Sep 8, 2022

TEP-0114: Added CustomRun references and specifications to v1beta1 #5403

TEP-0114: Added CustomRun references and specifications to v1beta1 #5403

Conversation

vsinghai commented Aug 31, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Aug 31, 2022

dibyom commented Aug 31, 2022

tekton-robot commented Aug 31, 2022

tekton-robot commented Sep 1, 2022

tekton-robot commented Sep 1, 2022

tekton-robot commented Sep 2, 2022

tekton-robot commented Sep 2, 2022

tekton-robot commented Sep 2, 2022

vsinghai commented Sep 6, 2022 • edited Loading

XinruZhang commented Sep 6, 2022

XinruZhang left a comment

Choose a reason for hiding this comment

XinruZhang Sep 7, 2022

Choose a reason for hiding this comment

vsinghai Sep 7, 2022

Choose a reason for hiding this comment

XinruZhang Sep 7, 2022

Choose a reason for hiding this comment

jerop Sep 8, 2022

Choose a reason for hiding this comment

vsinghai commented Sep 7, 2022

tekton-robot commented Sep 7, 2022

tekton-robot commented Sep 8, 2022

tekton-robot commented Sep 8, 2022

tekton-robot commented Sep 8, 2022

jerop left a comment

Choose a reason for hiding this comment

tekton-robot commented Sep 8, 2022

vsinghai commented Aug 31, 2022 •

edited

Loading

vsinghai commented Sep 6, 2022 •

edited

Loading