Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix EventListener instance deletion #339

Merged
merged 2 commits into from
Jul 23, 2021

Conversation

nikhil-thomas
Copy link
Member

Separate deletion of deployments from the deletion of all other resources.
So that delete call to CRDs are executed first before calling delete on controllers.
This will help to create a window before controller deletion, where controller can
handle finalizers on CRDs and CRD instances.

This is not the perfect fix, but this is good enough.

Ideal fix will be to add finalizer on controllers and remove them after making sure
all other resources have been removed. But this could create other issues such as the
CRDs watched by controllers being deleted can cause the controllers t

Signed-off-by: Nikhil Thomas nikthoma@redhat.com
(cherry picked from commit 962c5bf)

Changes

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

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 30, 2021
@tekton-robot tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 30, 2021
@tekton-robot
Copy link
Contributor

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

File Old Coverage New Coverage Delta
pkg/reconciler/common/install.go 65.5% 64.5% -1.0

@vdemeester
Copy link
Member

/approve

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. 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 Jun 30, 2021
@tekton-robot
Copy link
Contributor

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

File Old Coverage New Coverage Delta
pkg/reconciler/common/install.go 65.5% 64.5% -1.0

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2021
Separate deletion of deployments from the deletion of all other resources.
So that delete call to CRDs are executed first before calling delete on controllers.
This will help to create a window before controller deletion, where controller can
handle finalizers on CRDs and CRD instances.

This is not the perfect fix, but this is good enough.

Ideal fix will be to add finalizer on controllers and remove them after making sure
all other resources have been removed. But this could create other issues such as the
CRDs watched by controllers being deleted can cause the controllers t

Signed-off-by: Nikhil Thomas <nikthoma@redhat.com>
(cherry picked from commit 962c5bf)
Signed-off-by: Nikhil Thomas <nikthoma@redhat.com>
@nikhil-thomas nikhil-thomas force-pushed the fix/eventlistener-uninstall branch from d508541 to 3201f65 Compare July 23, 2021 09:51
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2021
@tekton-robot
Copy link
Contributor

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

File Old Coverage New Coverage Delta
pkg/reconciler/common/install.go 62.5% 61.8% -0.7

@sm43
Copy link
Member

sm43 commented Jul 23, 2021

/lgtm
/meow

@tekton-robot
Copy link
Contributor

@sm43: cat image

In response to this:

/lgtm
/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 tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2021
@tekton-robot tekton-robot merged commit 8cbff87 into tektoncd:main Jul 23, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants