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

TEP-0114: Add support for PipelineRun reconciler to create CustomRuns #5832

Merged

Conversation

abayer
Copy link
Contributor

@abayer abayer commented Dec 5, 2022

Changes

Modifies the PipelineRun reconciler to allow creating CustomRuns, rather than Runs, if the new custom-task-version feature flag is set to v1beta1, rather than the default value of v1alpha1.

See TEP-0114 for more information on the feature flag.

The Run-flavored wait-task is renamed to wait-task-alpha for better clarity.

Also adds a new integration test for using the beta CustomRun with the custom-task-version feature flag set to v1beta1.

Followups needed:

  • Docs updates
  • Deprecation notice on the custom-task-version flag
  • After the next release, test/custom-task-ctrls/wait-task-beta/cmd/controller/main.go needs to switch to using tkncontroller.FilterCustomRunRef
  • And also after that release, test/custom-task-ctrls/wait-task-beta/pkg/reconciler/reconciler_test.go needs to switch to using parse.MustParseCustomRun

/kind feature

Submitter Checklist

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

  • 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
  • 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

Custom task runs created from PipelineRuns can now be v1beta1.CustomRuns instead of v1alpha1.Runs, if the "custom-task-version" feature flag is set to "v1beta1", instead of the default "v1alpha1". Note that custom task controllers would need to be updated to listen for *v1beta1.CustomRun instead of *v1alpha1.Run in order to respond to v1beta1.CustomRuns.

@abayer abayer added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 5, 2022
@tekton-robot tekton-robot added the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Dec 5, 2022
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 5, 2022
@abayer
Copy link
Contributor Author

abayer commented Dec 5, 2022

/assign @jerop

@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 80.3% 79.5% -0.8
pkg/apis/pipeline/v1alpha1/run_types.go 57.1% 55.2% -2.0
pkg/apis/pipeline/v1beta1/customrun_types.go 57.1% 55.2% -2.0
pkg/reconciler/pipelinerun/cancel.go 94.0% 84.3% -9.7
pkg/reconciler/pipelinerun/pipelinerun.go 86.2% 86.2% 0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 94.7% 93.8% -0.9
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go 97.9% 95.7% -2.2
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 96.3% 92.9% -3.5
pkg/reconciler/pipelinerun/timeout.go 88.2% 84.1% -4.1

@abayer
Copy link
Contributor Author

abayer commented Dec 5, 2022

Also cc @XinruZhang due to the changes I had to make to the wait-task-beta controller to get it to actually work. =)

customruninformer.Get(ctx).Informer().AddEventHandler(cache.FilteringResourceEventHandler{
FilterFunc: tkncontroller.FilterRunRef("wait.testing.tekton.dev/v1beta1", "Wait"),
Handler: controller.HandleAll(impl.Enqueue),
// TODO: Replace with the following once tkncontroller.FilterCustomRunRef is merged (fromhttps://github.com/tektoncd/pipeline/pull/5822)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The relevant changes have been merged (#5822) but unless we change the wait-task-beta's go.mod to depend on a specific sha of tektoncd/pipeline instead of a release, we still need to wait for the next to release to switch this properly.

@abayer abayer force-pushed the customrun-and-run-with-feature-flag-rebase branch from 51dd522 to 0f9e257 Compare December 5, 2022 16:10
@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 80.3% 80.0% -0.3
pkg/apis/pipeline/v1alpha1/run_types.go 57.1% 55.2% -2.0
pkg/apis/pipeline/v1beta1/customrun_types.go 57.1% 55.2% -2.0
pkg/reconciler/pipelinerun/cancel.go 94.0% 84.3% -9.7
pkg/reconciler/pipelinerun/pipelinerun.go 86.2% 86.2% 0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 94.7% 93.8% -0.9
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go 97.9% 95.7% -2.2
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 96.3% 92.9% -3.5
pkg/reconciler/pipelinerun/timeout.go 88.2% 84.1% -4.1

docs/install.md Outdated Show resolved Hide resolved
pkg/apis/config/feature_flags.go Outdated Show resolved Hide resolved
Copy link
Member

@XinruZhang XinruZhang left a comment

Choose a reason for hiding this comment

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

Thanks @abayer!

FilterFunc: func(obj interface{}) bool {
r, ok := obj.(*v1beta1.CustomRun)
if !ok {
// Somehow got informed of a non-Run object.
Copy link
Member

@XinruZhang XinruZhang Dec 5, 2022

Choose a reason for hiding this comment

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

nit: non-CustomRun

@abayer abayer force-pushed the customrun-and-run-with-feature-flag-rebase branch from 0f9e257 to c0cd44b Compare December 5, 2022 19:23
@abayer
Copy link
Contributor Author

abayer commented Dec 5, 2022

/hold

@XinruZhang - default changed. I'm holding the PR for the moment so that if for some reason we change our minds and want v1beta1 to be the default again, I don't have to spend an hour or two undoing what I just did. =)

@tekton-robot tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Dec 5, 2022
@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 80.3% 80.0% -0.3
pkg/apis/pipeline/v1alpha1/run_types.go 57.1% 55.2% -2.0
pkg/apis/pipeline/v1beta1/customrun_types.go 57.1% 55.2% -2.0
pkg/reconciler/pipelinerun/cancel.go 94.0% 90.4% -3.7
pkg/reconciler/pipelinerun/pipelinerun.go 86.2% 86.2% 0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 94.7% 94.3% -0.4
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go 97.9% 95.7% -2.2
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 95.4% 92.9% -2.6
pkg/reconciler/pipelinerun/timeout.go 88.2% 84.1% -4.1

Copy link
Member

@XinruZhang XinruZhang left a comment

Choose a reason for hiding this comment

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

Several minor comments, thanks!

config/config-feature-flags.yaml Outdated Show resolved Hide resolved
pkg/apis/config/feature_flags.go Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/pipelinerun_types.go Outdated Show resolved Hide resolved
@jerop
Copy link
Member

jerop commented Dec 6, 2022

I'm holding the PR for the moment so that if for some reason we change our minds and want v1beta1 to be the default again, I don't have to spend an hour or two undoing what I just did. =)

I support defaulting to v1alpha1 in the initial release as described in TEP-0114

cc @tektoncd/core-maintainers

@abayer abayer changed the title TEP-0114: Switch default custom task creation to CustomRun TEP-0114: Add support for PipelineRun reconciler to create CustomRuns Dec 6, 2022
@abayer abayer force-pushed the customrun-and-run-with-feature-flag-rebase branch from c0cd44b to 2eb32e2 Compare December 6, 2022 15:23
@abayer
Copy link
Contributor Author

abayer commented Dec 6, 2022

/hold cancel

Ok, that's enough confirmation for me - I've squashed and removed the hold.

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 6, 2022
@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 80.3% 80.0% -0.3
pkg/apis/pipeline/v1alpha1/run_types.go 57.1% 55.2% -2.0
pkg/apis/pipeline/v1beta1/customrun_types.go 57.1% 55.2% -2.0
pkg/reconciler/pipelinerun/cancel.go 94.0% 90.4% -3.7
pkg/reconciler/pipelinerun/pipelinerun.go 86.2% 86.2% 0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 94.7% 94.3% -0.4
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go 97.9% 95.7% -2.2
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 95.4% 93.8% -1.7
pkg/reconciler/pipelinerun/timeout.go 88.2% 84.1% -4.1

docs/install.md Outdated Show resolved Hide resolved
pkg/apis/config/feature_flags_test.go Outdated Show resolved Hide resolved
pkg/apis/config/feature_flags.go Show resolved Hide resolved
pkg/apis/pipeline/v1alpha1/run_types.go Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/customrun_types.go Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/pipelinerun_types.go Outdated Show resolved Hide resolved
@abayer abayer force-pushed the customrun-and-run-with-feature-flag-rebase branch from 2eb32e2 to 2e02810 Compare December 6, 2022 15:59
@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 84.8% 86.1% 1.4
pkg/apis/pipeline/v1alpha1/run_types.go 57.1% 58.6% 1.5
pkg/apis/pipeline/v1beta1/customrun_types.go 57.1% 58.6% 1.5
pkg/reconciler/pipelinerun/cancel.go 94.0% 90.4% -3.7
pkg/reconciler/pipelinerun/pipelinerun.go 86.2% 86.2% 0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 94.7% 94.3% -0.4
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go 97.9% 95.7% -2.2
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 95.4% 92.9% -2.6
pkg/reconciler/pipelinerun/timeout.go 88.2% 84.1% -4.1

Name: "r1",
PipelineTaskName: "run-1",
},
{
TypeMeta: runtime.TypeMeta{Kind: "Run"},
TypeMeta: runtime.TypeMeta{Kind: pipeline.CustomRunControllerName},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TypeMeta: runtime.TypeMeta{Kind: pipeline.CustomRunControllerName},
TypeMeta: runtime.TypeMeta{Kind: pipeline.RunControllerName},

Copy link
Member

Choose a reason for hiding this comment

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

also, can we please add customRuns to the TestCancelPipelineRun test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@abayer abayer force-pushed the customrun-and-run-with-feature-flag-rebase branch from d35791b to 0e5bbde Compare December 8, 2022 13:24
@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 84.8% 86.1% 1.4
pkg/apis/pipeline/v1alpha1/run_types.go 57.1% 58.6% 1.5
pkg/reconciler/pipelinerun/cancel.go 94.0% 90.4% -3.7
pkg/reconciler/pipelinerun/pipelinerun.go 86.2% 86.2% 0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 94.7% 94.3% -0.4
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go 97.9% 95.7% -2.2
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 95.4% 92.9% -2.6
pkg/reconciler/pipelinerun/timeout.go 88.2% 84.1% -4.1

@abayer abayer force-pushed the customrun-and-run-with-feature-flag-rebase branch from 0e5bbde to f0b33b0 Compare December 8, 2022 16:40
@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 84.8% 86.1% 1.4
pkg/apis/pipeline/v1alpha1/run_types.go 57.1% 58.6% 1.5
pkg/apis/pipeline/v1beta1/customrun_types.go 57.1% 58.6% 1.5
pkg/reconciler/pipelinerun/cancel.go 94.0% 90.4% -3.7
pkg/reconciler/pipelinerun/pipelinerun.go 86.2% 86.2% 0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 94.7% 94.3% -0.4
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go 97.9% 95.7% -2.2
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 95.4% 92.9% -2.6
pkg/reconciler/pipelinerun/timeout.go 88.2% 84.1% -4.1

@abayer
Copy link
Contributor Author

abayer commented Dec 8, 2022

/retest

2 similar comments
@abayer
Copy link
Contributor Author

abayer commented Dec 8, 2022

/retest

@abayer
Copy link
Contributor Author

abayer commented Dec 8, 2022

/retest

pkg/apis/run/v1beta1/customrunstatus_types.go Show resolved Hide resolved
pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved
pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved
pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved
pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved
pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved
@abayer abayer force-pushed the customrun-and-run-with-feature-flag-rebase branch from f0b33b0 to 69881a2 Compare December 9, 2022 13:23
@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 84.8% 86.1% 1.4
pkg/apis/pipeline/v1alpha1/run_types.go 57.1% 58.6% 1.5
pkg/apis/pipeline/v1beta1/customrun_types.go 57.1% 58.6% 1.5
pkg/apis/run/v1beta1/customrunstatus_types.go Do not exist 17.6%
pkg/reconciler/pipelinerun/cancel.go 94.0% 90.4% -3.7
pkg/reconciler/pipelinerun/pipelinerun.go 86.2% 86.2% 0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 94.7% 94.3% -0.4
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go 97.9% 95.7% -2.2
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 95.4% 92.9% -2.6
pkg/reconciler/pipelinerun/timeout.go 88.2% 84.1% -4.1

pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved
pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved
pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved
pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved
pkg/reconciler/pipelinerun/timeout.go Outdated Show resolved Hide resolved
Modifies the `PipelineRun` reconciler to allow creating `CustomRun`s, rather than `Run`s, if the new `custom-task-version` feature flag is set to `v1beta1`, rather than the default value of `v1alpha1`.

See [TEP-0114](https://github.com/tektoncd/community/blob/main/teps/0114-custom-tasks-beta.md#new-feature-flag-custom-task-version) for more information on the feature flag.

The `Run`-flavored `wait-task` is renamed to `wait-task-alpha` for better clarity.

Also adds a new integration test for using the beta `CustomRun` with the `custom-task-version` feature flag set to `v1beta1`.

Followups needed:
* Docs updates
* Deprecation notice on the `custom-task-version` flag
* After the next release, `test/custom-task-ctrls/wait-task-beta/cmd/controller/main.go` needs to switch to using `tkncontroller.FilterCustomRunRef`
* And also after that release, `test/custom-task-ctrls/wait-task-beta/pkg/reconciler/reconciler_test.go` needs to switch to using `parse.MustParseCustomRun`

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
@abayer abayer force-pushed the customrun-and-run-with-feature-flag-rebase branch from 69881a2 to e1eee36 Compare December 9, 2022 15:10
@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 84.8% 86.1% 1.4
pkg/apis/pipeline/v1alpha1/run_types.go 57.1% 58.6% 1.5
pkg/apis/pipeline/v1beta1/customrun_types.go 57.1% 58.6% 1.5
pkg/apis/run/v1beta1/customrunstatus_types.go Do not exist 17.6%
pkg/reconciler/pipelinerun/cancel.go 94.0% 90.4% -3.7
pkg/reconciler/pipelinerun/pipelinerun.go 86.2% 86.2% 0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 94.7% 94.3% -0.4
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go 97.9% 95.7% -2.2
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 95.4% 93.8% -1.7
pkg/reconciler/pipelinerun/timeout.go 88.2% 84.1% -4.1

@abayer
Copy link
Contributor Author

abayer commented Dec 9, 2022

/retest

Copy link
Member

@jerop jerop left a comment

Choose a reason for hiding this comment

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

thank you @abayer!

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop

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 9, 2022
@dibyom
Copy link
Member

dibyom commented Dec 9, 2022

Thanks @abayer - this looks good to me!

There are quite a few follow ups - are we tracking them somewhere besides the PR description (just want to make sure we get all of them in for the release) EDIT: looks like we only need 2 of the follow ups before the release

@dibyom
Copy link
Member

dibyom commented Dec 9, 2022

/lgtm

/hold

(feel free to cancel the hold if the docs/deprecation notice stuff is already being tracked and is on track for the release)

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2022
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2022
@jerop
Copy link
Member

jerop commented Dec 10, 2022

looks like we only need 2 of the follow ups before the release

Documentation: tracking docs in #5158 and there's a PR for migration docs #5850

Deprecation notice on the custom-task-version flag: tracking in #5836 and #5837

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 10, 2022
@tekton-robot tekton-robot merged commit 902c142 into tektoncd:main Dec 10, 2022
@jerop jerop linked an issue Dec 10, 2022 that may be closed by this pull request
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.

TEP-0114: Updating pipelineTask spec to accomodate for CustomRun and Run
5 participants