Add path to taskrun finalizer name#1391
Conversation
Fixes the following warning: "prefer a domain-qualified finalizer name including a path (/) to avoid accidental conflicts with other finalizer writers".
tekton-robot
added
the
kind/misc
tekton-robot
added
the
size/XS
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lcarva The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
|
/retest |
|
@lcarva @vdemeester Isn't this a breaking change? |
If you believe that there are other controllers or clients managing or using that finalizer, that will be a breaking change for them. I find this quite unlikely though. The finalizer name is also not part of the CR specification, so another reason to not consider that a breaking change. That's IMO. |
|
If we deploy the latest chains controller, it could leave the older finalizer. Won't that cause an issue? User would need to manually delete the old finalizers. @enarha |
Yes, you are right. During upgrade, existing finalizers won't be removed by the new controller instance. The user will have to remove them manually. Another option is we add some extra code, which only removes the old finalizer if found. |
I created #1394 to address this issue. I need some more testing and I'll remove the WIP once done. |
#1394 is ready for review. @khrm @vdemeester @lcarva PTAL. |
5 participants
Fixes the following warning:
"API Warning: metadata.finalizers: "chains.tekton.dev": prefer a domain-qualified finalizer name including a path (/) to avoid accidental conflicts with other finalizer writers".
Changes
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes
/kind misc