medik8s / node-maintenance-operator

Kubernetes Operator to manage node maintenance through NodeMaintenance custom resources
https://www.medik8s.io/maintenance-node/
Apache License 2.0
28 stars 14 forks source link

Add Events for the Maintenance Process #113

Closed razo7 closed 6 months ago

razo7 commented 8 months ago

ECOPROJECT-1745 ECOPROJECT-1898

openshift-ci[bot] commented 8 months ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

openshift-ci[bot] commented 8 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: razo7

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](https://github.com/medik8s/node-maintenance-operator/blob/main/OWNERS)~~ [razo7] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
razo7 commented 8 months ago

/test 4.15-openshift-e2e

razo7 commented 8 months ago

/hold The PR would be better reviewed in smaller chunks, so I am moving the refactoring of unit-test and reconciling to different PRs, and keeping the current one for adding the events (needs the unit-test refactoring).

razo7 commented 7 months ago

After rebasing it is ready for review

razo7 commented 7 months ago

/test 4.15-openshift-e2e

razo7 commented 7 months ago

/test 4.15-openshift-e2e

razo7 commented 6 months ago

/test 4.15-openshift-e2e

razo7 commented 6 months ago

/test 4.15-openshift-e2e

clobrano commented 6 months ago

Why are we not using common api for events?

razo7 commented 6 months ago

Why are we not using common api for events?

Because it is kind of meant for remediation events (include [remediation] in the message), and here is for maintenance events. We could modify that in common, but it will require some adaptations in the remediator repos afterward. I didn't think it was worth the effort but if you disagree then I would recommend first generalizing the events common API.

clobrano commented 6 months ago

(include [remediation] in the message)

I understand your point. The idea of the [remediation] tag was to allow filter and collect all the messages from our operators (it was [medik8s] at the beginning), but I understand that can cause misunderstanding now. We might need to reflect on a better tag :thinking:

razo7 commented 6 months ago

We might need to reflect on a better tag 🤔

I agree. Something for a follow up :)

slintes commented 6 months ago

We might need to reflect on a better tag 🤔

I agree. Something for a follow up :)

I don't think so. The idea was to filter remediation events easily indeed, not medik8s

clobrano commented 6 months ago

I don't think so. The idea was to filter remediation events easily indeed, not medik8s

Then I misunderstood

mshitrit commented 6 months ago

/lgtm /hold Giving a chance for others to add more comments. Feel free to unhold.

razo7 commented 6 months ago

/test 4.15-openshift-e2e

razo7 commented 6 months ago

/test 4.15-openshift-e2e

razo7 commented 6 months ago

/test 4.15-openshift-e2e

clobrano commented 6 months ago

/lgtm Giving a chance to get-more/close-pending reviews /hold

razo7 commented 6 months ago

/retest

razo7 commented 6 months ago

No pending reviews or new comments, so I am upholding /unhold