Thank you very much for the suggestion and for taking the time to improve the code! 🙏

The reason for keeping the original implementation as it is, is because:

Separation of concerns:
We first check if the finalizer is already present using ContainsFinalizer, and only attempt to add it if necessary. This makes the logic clear and avoids unnecessary operations.

Efficient error handling:
By explicitly checking first, we can log and manage errors more precisely at each step, which makes debugging and monitoring easier.

Avoid unnecessary API server writes:
Updating the resource only when a change is needed helps reduce API server load, avoids unnecessary reconciliation retries, and keeps the controller efficient.

Even though AddFinalizer internally checks, doing the explicit check first improves clarity, and bring the above benefits which seems more aligned.

For these reasons, I think it might be better to keep the original implementation as it is.

Thanks again for your contribution and for sparking this valuable discussion! 🌟

/hold

If you do not mind, I think we should close this one as not accepted.

Thank you for the quick and thorough review
I agree that using ContainsFinalizer and AddFinalizer together improves readability. However, I still believe the original code block is misleading and can be improved. It appears to contain dead code within the conditional block:

		if ok := controllerutil.AddFinalizer({{ lower .Resource.Kind }}, {{ lower .Resource.Kind }}Finalizer); !ok {
			err = fmt.Errorf("finalizer for {{ .Resource.Kind }} was not added")
			log.Error(err, "Failed to add finalizer for {{ .Resource.Kind }}")
			return ctrl.Result{}, err
		}

Since the presence of the finalizer is checked before this block (by ContainsFinalizer), the condition where AddFinalizer returns false (and thus the error logging) will never be reached. Examining the implementation of AddFinalizer confirms this, as it only appends the finalizer and returns true if it wasn't already present.

My proposal aims for a more precise approach:

	if !controllerutil.ContainsFinalizer(memcached, memcachedFinalizer) {
		log.Info("Adding Finalizer for Memcached")
		controllerutil.AddFinalizer(memcached, memcachedFinalizer)
		if err = r.Update(ctx, memcached); err != nil {
			log.Error(err, "Failed to update custom resource to add finalizer")
			return ctrl.Result{}, err
		}
	}

This revised code explicitly checks for the finalizer's absence, logs the intention to add it, performs the addition, and then handles potential errors during the update operation.

@camilamacedo86 Would you agree with this approach?

Hi @matrus2

Thank you for looking into that.
Your observation seems to be accurate, and your proposal seems to solve it well. 👍

Could you please update this PR with the proposal?
Also, please ensure that you squash the commits for we have a good git history.

Thank you a lot 🥇

Hi @matrus2

Please, ping me when you think that be good to fly
Thank you

@camilamacedo86 Changes committed, we are good.

I see some test failed not related to changes incorporated here, needs some investigation.