⚠️ Call MutateFn on CreateOrUpdate regardless of the operation

[calind]

#313 breaks CreateOrUpdate by not calling the MutateFn when new objects are created. This patch fixes that and updates the tests to better reflect the expected behavior.

[adracus]

I had it like that but @mengqiy commented in #313 that this is maybe not the thing we want to do...
If you're anyways working on this function, then I guess after this change, MutateFn is again not the right name but rather sth like ReconcileFn. Also, do we always want to have the pointer of the existing object being passed in to our function? This way, since it's runtime.Object, we'll have to typecast which somehow eliminates the niceness of this library....

[calind]

The behavior was to allways call on MutateFn. Check https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/controller/controllerutil#example-CreateOrUpdate.

obj is not the desired state and CreateOrUpdate cannot reliably work by just passing in a desired object state. That is because some fields are immutable (eg. a deployment selector) and there is not logic to detect which ones. So here is the need to have a user defined behavior for this situations, thus the MuateFn.

MutateFn needs the pointer passed-in since it need access to existing state omehow as it's role is to mutate the existing state towards desired state.

[adracus]

Yes but doesn't the caller of CreateOrUpdate anyways have to pass in a pointer to the object that should be created / updated? If yes, then he should also be able to supply a closure that reconciles the object accordingly

[calind]

I understand your point now.

I think that can work but it's a breaking change and might deserve another PR. For now you can still get away without casting like this:

controllerutil.CreateOrUpdate(context.TODO(), c, deploy, func(_ runtime.Object) error {
    deploy.Spec.Replicas = 5
    return nil
})

[adracus]

Ok so I'm not the only one seeing this 😀 anyways then I'm also fine with it

[mengqiy]

I think the debate is if CreateOrUpdate should do real mutation work.
It seems in the very first implementation it did real mutation work.
IMO CreateOrUpdate should behave like server-side apply which ensure an object exists in the server with desired state. It should takes a ReconcileFn, since we may want to preserve some fields set by the server. ReconcileFn should be able to reconcile the provided object and the live object got from the server and merge them and update the live object. e.g. when updating a service, I may want to preserve the .spec.clusterIP and .spec.port[*].nodePort.

#313 has not been released yet, so we still have time to decide what we want CreateOrUpdate to do.

@DirectXMan12 what was the initial intend of this method? I will let you make the final call since you are the reviewer of that PR.

[calind]

Probably I'm biased here doing the original implementation but my take on this is that is better to keep the things the way they are with CreateOrUpdate and when server-side apply is ready provide a method for that.

Otherwise controller-runtime users will probably end up implementing something that's like server-side apply in MutateFn/ReconcileFn which is less than ideal. Probably server-side apply is not mainstream yet because it's hard to do it and I think that users having to implement it on their own is very prone to errors.

The breaking change that I'd be inclined to do is to remove or make optional the obj from MutateFn as per @adracus suggestion since you can allways use the passed-in obj to the CreateOrUpdate.

Btw. we're relying to the original behavior on 3 operators already and the feedback on the original PR was that it enables good patterns in user's code.

[calind]

This is the original PR: #98. I think it's worth revisiting it first.

[DirectXMan12]

I'm 👍 on changing MutateFn to not take an additional argument. I think it makes sense, especially since the user already has to pass that in.

I'm also 👍 on fixing this to always call MutateFn, like the original PR -- I liked the original behavior, because it prevented bugs where you accidentally put different date in place in create and update.

[DirectXMan12]

(now is the time to make breaking changes, if we want to)

[DirectXMan12]

P.S. We should add really obvious tests that make it clear that this is the desired behavior as well.

[calind]

I've removed the parameter in MutateFn. The tests were improved in previous commit. @DirectXMan12 PTAL.

[DirectXMan12]

minor nits inline, otherwise looks good

[DirectXMan12]

nit: docs are wrong now (nothing is passed to mutate function, the sentence about obj needs to be updated too)

[DirectXMan12]

can improve nesting and readability here a bit by inverting this:

Suggested change

	if errors.IsNotFound(err) {
	if !errors.IsNotFound(err) {
	return OperationResultNone, err
	}
	if err := f(); err != nil {

[DirectXMan12]

will probably fix the cyclo issue too.

[calind]

Implemented the feedback and also addressed the gocyclo issue. @DirectXMan12 PTAL.

[DirectXMan12]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: calind, DirectXMan12

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 [DirectXMan12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment