fix: add finalizers to cleanup cluster scoped resources on stack dele… #608
Conversation
jan--f
requested review from
danielmellado and
JoaoBraveCoding
and removed request for
a team
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jan--f 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 |
…tion Signed-off-by: Jan Fajerski <[email protected]>
jan--f
force-pushed
the
finalizer-cluster-scoped-resources
branch
from
3cb25f0
to
7115cc7
Compare
| logger.V(6).Info("skipping reconcile since object is already schedule for deletion") | ||
| logger.V(6).Info("removing cluster scoped resources") | ||
|
|
||
| reconcilers := stackComponentCleanup(ms) |
There was a problem hiding this comment.
just a nit over here, if we have the prometheus and the alertamanager names, why not adding that also as a log? It may be useful when debugging. Otherwise lgtm!
There was a problem hiding this comment.
This is just an info log. In line 158 an error is logged in case something goes wrong, that'll include the resource name that creates an error. Is that what you're after?
Not sure we should log 6 resource names on the happy path.
There was a problem hiding this comment.
nvm, mistook the 'Info' for 'Err' in my mind, LGTM then, thanks!
|
oh, missed the info label, let's get this in then!
El mar, 29 oct 2024, 20:31, Jan Fajerski ***@***.***>
escribió:
… ***@***.**** commented on this pull request.
------------------------------
In pkg/controllers/monitoring/monitoring-stack/controller.go
<#608 (comment)>
:
> if !ms.ObjectMeta.DeletionTimestamp.IsZero() {
- logger.V(6).Info("skipping reconcile since object is already schedule for deletion")
+ logger.V(6).Info("removing cluster scoped resources")
+
+ reconcilers := stackComponentCleanup(ms)
This is just an info log. In line 158 an error is logged in case something
goes wrong, that'll include the resource name that creates an error. Is
that what you're after?
Not sure we should log 6 resource names on the happy path.
—
Reply to this email directly, view it on GitHub
<#608 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKAVQZH6I7WKMR6Z6XZNQTZ57O65AVCNFSM6AAAAABQXYGDXCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMBSHA4TQOBVG4>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
|
/lgtm |
…tion
Fixes: COO-495