Remove RunControllerName from the Codebase #6531

jsminem · 2023-04-13T12:45:19Z

Remove RunControllerName from the CodeBase
Fixes #6528

/kind cleanup

Changes

Submitter Checklist

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

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
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

NONE

tekton-robot · 2023-04-13T12:45:29Z

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

lbernick · 2023-04-13T13:23:05Z

/ok-to-test

vdemeester · 2023-04-13T13:42:07Z

@jsminem @lbernick note, as long as it is a Draft PR, it won't run the CI 👼🏼

tekton-robot · 2023-04-13T13:52:31Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	97.0%	96.4%	-0.6

tekton-robot · 2023-04-13T13:55:17Z

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/reconciler/pipelinerun/resources/pipelinerunresolution.go	97.0%	96.4%	-0.6

tekton-robot · 2023-04-13T13:56:50Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	97.0%	96.4%	-0.6

jerop

Thank you @jsminem and welcome to Tekton!

/meow

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

tekton-robot · 2023-04-13T13:59:58Z

@jerop:

In response to this:

Thank you @jsminem and welcome to Tekton!

/meow

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 · 2023-04-13T14:00:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop, 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:

~~OWNERS~~ [jerop,vdemeester]

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

pkg/apis/pipeline/v1alpha1/run_types.go

tekton-robot · 2023-04-13T16:51: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	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	97.0%	96.4%	-0.6

tekton-robot · 2023-04-13T16:53:11Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	97.0%	96.4%	-0.6

tekton-robot · 2023-04-13T16:54:27Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	97.0%	96.4%	-0.6

wlynch · 2023-04-14T15:25:18Z

pkg/apis/pipeline/v1beta1/customrun_types.go

 // GetGroupVersionKind implements kmeta.OwnerRefable.
 func (*CustomRun) GetGroupVersionKind() schema.GroupVersionKind {
-	return SchemeGroupVersion.WithKind(pipeline.RunControllerName)
+	return SchemeGroupVersion.WithKind(pipeline.CustomRunControllerName)


We should call this out as a breaking change.

This is changing the GVK from Run -> CustomRun, which is going to break anything that is relying on this for GVK references (including owner refs for deletion).
That said, this is also a bug fix for v1beta1.CustomRun since the correct kind is CustomRun. 😬

We should probably include documentation in the release notes warning people about the bug and how they should go about identifying incorrect references for v1beta1.CustomRun. OwnerRefs are probably going to be the most common - we may want to consider making a small tool to identify bad references.

Ohh okay.. sounds good! I've reverted the changes from CustomRunControllerName to RunControllerName. Thanks so much!

@wlynch do you mean we should fix the bug, or fix it and add a note in the release notes?
CustomRun is a v1beta1 API but I wouldn't feel ok waiting for many months for fixing this.

@tektoncd/core-collaborators FYI

@wlynch do you mean we should fix the bug, or fix it and add a note in the release notes?

Fix it and call it out in the release notes as a breaking - it's a bug fix, not a feature deprecation so I don't think we need to wait. If the alpha type needs this as Run, then we should fork the type.

That said, I don't see a reason to hold up this PR - we can follow up in another change.

remove RunControllerName from the CodeBase remove RunControllerName from the CodeBase remove RunControllerName from the CodeBase remove RunControllerName from the CodeBase remove RunControllerName from the CodeBase remove RunControllerName from the CodeBase remove RunControllerName from the CodeBase remove RunControllerName from the CodeBase

tekton-robot · 2023-04-14T21:52: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/reconciler/pipelinerun/resources/pipelinerunresolution.go	97.0%	96.4%	-0.6

tekton-robot · 2023-04-14T21:54:17Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	97.0%	96.4%	-0.6

wlynch · 2023-04-19T16:25:09Z

/lgtm

tekton-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none Denotes a PR that doesnt merit a release note. labels Apr 13, 2023

tekton-robot requested review from lbernick and vdemeester April 13, 2023 12:45

tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 13, 2023

jsminem marked this pull request as draft April 13, 2023 13:02

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 13, 2023

jsminem force-pushed the remove-runcontrollername branch from d6060c3 to 0a616fc Compare April 13, 2023 13:32

tekton-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 13, 2023

jsminem closed this Apr 13, 2023

jsminem force-pushed the remove-runcontrollername branch from 0a616fc to abd1849 Compare April 13, 2023 13:33

jsminem reopened this Apr 13, 2023

vdemeester approved these changes Apr 13, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2023

jsminem marked this pull request as ready for review April 13, 2023 13:43

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 13, 2023

tekton-robot requested review from dibyom and wlynch April 13, 2023 13:43

tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 13, 2023

jsminem force-pushed the remove-runcontrollername branch from c624244 to 6603d57 Compare April 13, 2023 13:50

jerop approved these changes Apr 13, 2023

View reviewed changes

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go Show resolved Hide resolved

jsminem force-pushed the remove-runcontrollername branch 3 times, most recently from 70f27e1 to b331ced Compare April 13, 2023 14:29

afrittoli reviewed Apr 13, 2023

View reviewed changes

pkg/apis/pipeline/v1alpha1/run_types.go Outdated Show resolved Hide resolved

jsminem force-pushed the remove-runcontrollername branch 2 times, most recently from e95233b to faf67bc Compare April 13, 2023 14:33

tekton-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 13, 2023

jsminem force-pushed the remove-runcontrollername branch from 429d248 to 8d1b4ae Compare April 13, 2023 16:46

wlynch requested changes Apr 14, 2023

View reviewed changes

jsminem force-pushed the remove-runcontrollername branch from 8d1b4ae to e3b3f6d Compare April 14, 2023 21:47

jsminem requested a review from wlynch April 14, 2023 21:47

lbernick removed their request for review April 19, 2023 16:25

tekton-robot assigned wlynch Apr 19, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2023

wlynch mentioned this pull request Apr 19, 2023

Fix v1beta1.CustomRun GVK #6562

Merged

7 tasks

tekton-robot merged commit 7543a47 into tektoncd:main Apr 19, 2023

Remove RunControllerName from the Codebase #6531

Remove RunControllerName from the Codebase #6531

Uh oh!

Conversation

jsminem commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented Apr 13, 2023

Uh oh!

lbernick commented Apr 13, 2023

Uh oh!

vdemeester commented Apr 13, 2023

Uh oh!

tekton-robot commented Apr 13, 2023

Uh oh!

tekton-robot commented Apr 13, 2023

Uh oh!

tekton-robot commented Apr 13, 2023

Uh oh!

jerop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tekton-robot commented Apr 13, 2023

Uh oh!

tekton-robot commented Apr 13, 2023

Uh oh!

Uh oh!

tekton-robot commented Apr 13, 2023

Uh oh!

tekton-robot commented Apr 13, 2023

Uh oh!

tekton-robot commented Apr 13, 2023

Uh oh!

wlynch Apr 14, 2023

Choose a reason for hiding this comment

Uh oh!

jsminem Apr 14, 2023

Choose a reason for hiding this comment

Uh oh!

afrittoli Apr 17, 2023

Choose a reason for hiding this comment

Uh oh!

afrittoli Apr 17, 2023

Choose a reason for hiding this comment

Uh oh!

wlynch Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

wlynch Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Apr 14, 2023

Uh oh!

tekton-robot commented Apr 14, 2023

Uh oh!

wlynch commented Apr 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jsminem commented Apr 13, 2023 •

edited

Loading