remove code usage of InFlightRemediation

medik8s / node-healthcheck-operator

K8s Node Health Check Operator

Apache License 2.0

91 stars 18 forks source link

remove code usage of InFlightRemediation #320

Closed mshitrit closed 6 months ago

mshitrit commented 6 months ago

Why we need this PR

InFlightRemediation status field is deprecated and ultimately we want to remove it. As a second Phase (it was deprecated in the first phase), before removing it from the API we remove code usages to it therefore ceasing to update it. Since it's optional it will not show even though it is still part of the API.

Changes made

Removing code usages of InFlightRemediation

Which issue(s) this PR fixes

ECOPROJECT-1933

Test plan

openshift-ci[bot] commented 6 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

slintes commented 6 months ago

what about clearing existing InFlightRemediations?

mshitrit commented 6 months ago

what about clearing existing InFlightRemediations?

I am not sure it's worth the effort.

To do so we will need to add a webhook which will be need to be remove later (when InFlightRemediation is removed from API)
I think that doing so will be almost identical to removing the field from the API. Assuming we'll do it with a Defaulting webhook it means that InFlightRemediation will never show in the CR, which IIUC as far as the user is concerned is identical to removing it from the API.

The trade-off of not doing so is having out of sync data in a deprecated informational only in case user upgraded NHC while InFlightRemediation existed.

I'm leaning into taking the chance with that trade-off considering the value/effort of the alternative.

slintes commented 6 months ago

you don't need a webhook, just clean it in reconcile. I don't see a difference API wise to not filling it 🤔

mshitrit commented 6 months ago

/test 4.15-openshift-e2e

openshift-ci[bot] commented 6 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clobrano, mshitrit, 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-healthcheck-operator/blob/main/OWNERS)~~ [clobrano,mshitrit,razo7] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment