search

samba-in-kubernetes / samba-operator

An operator for a Samba as a service on PVCs in kubernetes
Apache License 2.0
108 stars 24 forks source link

Break Update reconcile function into smaller parts #235

Closed phlogistonjohn closed 2 years ago

phlogistonjohn commented 2 years ago

First, we add a Yield method that will return true if the result's content indicates that no further processing should be done.

Then using the Result.Yield method, we can more easily separate parts of the rather large Update function into sub-routines each returning Done if that sub-routine has nothing else to do or setting an error or requesting a Requeue.

View with -w (ignore whitespace) as well as default to see that most of this patch is moving chunks of Update into new functions.

I think this is a nice small tweak on the existing Result type that enables building "nested" reconcile functions. I'm throwing ths out there as a Draft first to see what you think.

Depends on: #239

phlogistonjohn commented 2 years ago

/test centos-ci/sink-clustered/mini-k8s-1.24

phlogistonjohn commented 2 years ago

/retest centos-ci/sink-clustered/mini-k8s-1.24

phlogistonjohn commented 2 years ago

/test centos-ci/sink-clustered/mini-k8s-1.24

synarete commented 2 years ago

Overall, looks very nice! Few minor style comments.

dpulls[bot] commented 2 years ago

:tada: All dependencies have been resolved !

phlogistonjohn commented 2 years ago

/retest centos-ci/sink-clustered/mini-k8s-1.24

phlogistonjohn commented 2 years ago

centos ci is stuck? I'm going to poke it again but right now it appears to be still pending since yesterday (?): Pending — Build triggered for merge commit.

phlogistonjohn commented 2 years ago

/test centos-ci/sink-clustered/mini-k8s-1.24

anoopcs9 commented 2 years ago

Pending — Build triggered for merge commit.

This is the current status for centos-ci/sink-clustered/mini-k8s-1.23 and was a side effect of some changes done at jenkins deployment. We should be looking at centos-ci/sink-clustered/mini-k8s-1.24.

phlogistonjohn commented 2 years ago

Oh, I totally missed that. Thanks.

phlogistonjohn commented 2 years ago

Since that 1.23 item probably isn't going to get unstuck I'm joint going to merge with admin power.