Remove finalizer if present #1244
Conversation
tekton-robot
added
the
release-note
tekton-robot
added
the
size/L
|
The following is the coverage report on the affected files.
|
| startResources: test.Resources{ | ||
| Namespaces: []*corev1.Namespace{namespaceResource}, | ||
| EventListeners: []*v1beta1.EventListener{makeEL(func(el *v1beta1.EventListener) { | ||
| el.Finalizers = append(el.Finalizers, "eventlisteners.triggers.tekton.dev") |
There was a problem hiding this comment.
I verified the code actually works but I'm not sure why the unit test does not. Maybe something to do with the fake clients
There was a problem hiding this comment.
Here is what was happening in the test:
- The initial EL did not have a status
- The reconcile did two updates - 1) one to remove the finalizer directly in ReconcileKind (since ReconcileKind only syncs changes to spec and status but not metadata) and 2) the generated Reconcile method making its own update that seems to overwrite the metadata from the first update.
- To fix this, I simply started the EL off with a status. Also verified that the finalizer actually gets removed in an actual cluster.
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
Previous versions of Triggers required a finalizer to clean up logging configMaps. They are no longer necessary. We need to remove the finalizer from any old EventListener objects so that they can be properly deleted in newer versions of Triggers. Also fixes some unit tests that started failing once I removed the bit that ignored `Finalizers` when comparing test output. Fixes tektoncd#1243 Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
|
The following is the coverage report on the affected files.
|
|
/assign @savitaashture |
| _, err := r.TriggersClientSet.TriggersV1beta1().EventListeners(el.Namespace).Update(ctx, el, metav1.UpdateOptions{}) | ||
| if err != nil { | ||
| logging.FromContext(ctx).Errorf("failed to update EventListener to remove finalizer: %v", err) | ||
| } |
There was a problem hiding this comment.
Instead of calling update for every finalizer removed, should we make 1 update call with all the removed finalizers at the end?
There was a problem hiding this comment.
There is only one finalizer called "eventlisteners.triggers.tekton.dev". We check for that name and remove it and leave any others alone (though generally there shouldn't be any other finalizers present)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wlynch 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 |
tekton-robot
added
the
approved
Changes
Previous versions of Triggers required a finalizer to clean up logging
configMaps. They are no longer necessary. We need to remove the finalizer from
any old EventListener objects so that they can be properly deleted in newer
versions of Triggers.
Also fixes some unit tests that started failing once I removed the bit that
ignored
Finalizerswhen comparing test output.Fixes #1243
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes