🌱 Relocate version change validation to preflight check #166
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/L
now operator supports only v1alpha4 and v1beta1, but it's an artificial limitation that we can avoid.
Previously, the check was executed at the end of reconciliation and in multiple instances. By moving it to the beginning, the process is halted when installed and desired versions match, streamlining the reconciliation workflow.
Fedosin
force-pushed
the
preflight_checks
branch
from
b461a00 to
b17b9c9
Compare
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
The issue is addressed in #168, so the only meaningful thing we can take from this PR is to move preflight checks into a separate package. |
|
PR needs rebase. 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. |
k8s-ci-robot
added
the
needs-rebase
| func (r *GenericProviderReconciler) reconcile(ctx context.Context, provider genericprovider.GenericProvider, genericProviderList genericprovider.GenericProviderList) (ctrl.Result, error) { | ||
| reconciler := newPhaseReconciler(*r, provider, genericProviderList) | ||
| phases := []reconcilePhaseFn{ | ||
| reconciler.preflightChecks, |
There was a problem hiding this comment.
What if we kept the preflightChecks as a phase here, so we can benefit from the centralised condition setting logic, and we defined the halt pattern you came up with, as a pattern for all the phases.
Maybe with a customisable halting message (similar to the one you specified here) that can be logged before returning reconcile()? We could even have halt as a message string (haltMsg?), and when the phase returns a non empty msg then we log and halt?
So here doing something like
res, halt, err = phase(ctx)
...
| // versionChanged try to get installed version from provider status and decide if it has changed. | ||
| func versionChanged(provider genericprovider.GenericProvider) (bool, error) { | ||
| installedVersion := provider.GetStatus().InstalledVersion | ||
| if installedVersion == nil { | ||
| return true, nil | ||
| } | ||
|
|
||
| currentVersion, err := version.ParseSemantic(*installedVersion) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| res, err := currentVersion.Compare(provider.GetSpec().Version) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| // we need to delete installed components if versions are different | ||
| versionChanged := res != 0 | ||
|
|
||
| return versionChanged, nil | ||
| } |
There was a problem hiding this comment.
I know this is just codemotion, but maybe we could make this even more useful and if
currentVersion.Compare != 0 perform a deep.Equal() and return the []string with the diff between the two or else return []string{}, nil.
This way we could log what is the detected difference and provide useful info to the user about it.
3 participants
What this PR does / why we need it:
Previously, the check was executed at the end of reconciliation and in multiple instances. By moving it to the beginning, the process is halted when installed and desired versions match, streamlining the reconciliation workflow.