feat: support notification template

[chenlujjj]

related issue: #1736

[theSuess]

Thanks for the contribution! Can you use the common fields introduced in #1765? I'd also like to wait for #1770 to be merged just so we won't need to do this work again for notification templates as well

[weisdd]

I'm also in favour of having this PR rewritten on top of changes introduced in #1765 and #1770.

[chenlujjj]

Sure, I'll do that.

[CarlosLanderas]

Wow!!! This is awesome @chenlujjj!

We really need this and I was actually thinking about opening an issue to ask if I can contribute it myself.

Awesome thanks!

[CarlosLanderas]

Any ETA for this nice feature to be released?

Many thanks!

[chenlujjj]

Hi @CarlosLanderas ,I'm waiting for #1770 to be merged, so that I can rewrite this PR on top of it.

[CarlosLanderas]

Perfect @chenlujjj , thanks!

[theSuess]

@chenlujjj we just merged #1770 so if you have the time to rebase ontop of that, that'd be great!

[Baarsgaard]

Hi @chenlujjj looks good overall.
I spotted a few places in the Reconcile function where you're doing more than necessary.
Some of the code can be deleted or replaced with a couple of helper functions.

The other comments are mostly minor changes!

I wrote these diffs in the Github UI, there is likely an error in one or more of them

[Baarsgaard]

@theSuess, @weisdd,
What do you think about this change where we can essentially remove the ResyncPeriodHasElapsed check?

[chenlujjj]

Thank you @Baarsgaard , I've updated the code based on your review suggestions and implementation of grafana folder controller.

[Baarsgaard]

Significantly less comments this time 😄 good work!

[chenlujjj]

@Baarsgaard Thanks for your patient review. I resolved all the comments.

I've got one question: in the following code

	if err != nil || len(instances) == 0 {
		setNoMatchingInstancesCondition(&notificationTemplate.Status.Conditions, notificationTemplate.Generation, err)
		meta.RemoveStatusCondition(&notificationTemplate.Status.Conditions, conditionNotificationTemplateSynchronized)
		notificationTemplate.Status.NoMatchingInstances = true
		controllerLog.Error(err, "could not find matching instances", "name", notificationTemplate.Name, "namespace", notificationTemplate.Namespace)
		return ctrl.Result{RequeueAfter: RequeueDelay}, err
	}

does it matter to return ctrl.Result{RequeueAfter: RequeueDelay}, err or return ctrl.Result{RequeueAfter: RequeueDelay}, nil ? I saw different implementations in different resource controllers.

[chenlujjj]

@Baarsgaard Could you please take another review, thank you.

[Baarsgaard]

Hi @chenlujjj sorry for the delay, here's another couple of comments 😅
Additionally, I'm very sorry for the PRs that I open that end up blocking yours 🙇‍♂️

[Baarsgaard]

@chenlujjj Both #1801 and #1804 were merged, you can now continue, and I will not be opening any more PRs that interfere with this!

[chenlujjj]

Thank you @Baarsgaard .
Hi @theSuess @weisdd Can anyone of you have a review ?

[weisdd]

Sorry for delays in reviews, during the holiday season many of maintainers are absent.

Overall, the code looks good to me, though we still need to make a few adjustments. Aside from the comments I've added, we also need to:

• add an e2e-test for the resource;
• add an example (check examples folder).

[weisdd]

I think we should not return an error here. - Having zero matching instances is a valid case, e.g. due to timing (the notification template gets reconciled earlier than Grafana), Grafana CR will be added later, etc. - In such scenario, we only need to update condition, but should not return errors in my opinion.

[Baarsgaard]

I took a deeper look at GetScopedMatchingInstances and the error is only defined when the client returns an error on listing grafana instances.
What do you think of:

if err != nil {
	return ctrl.Result{}, fmt.Errorf("could not find matching instances: %w", err)
} else {
	r.Log.Info("could not find matching instances")
	return ctrl.Result{Requeue: false}, nil
}

This way the log is still clear on what is happening and actual errors are logged.
Alternatively, we can change the function signature of GetScopedMatchingInstances to never return an error and only return empty arrays?
Then handle all the logging of errors within GetScopedMatchingInstances?

instances := GetScopedMatchingInstances(controllerLog, ctx, r.Client, notificationTemplate)
if len(instances) == 0 {
	setNoMatchingInstancesCondition(&notificationTemplate.Status.Conditions, notificationTemplate.Generation, err)
	meta.RemoveStatusCondition(&notificationTemplate.Status.Conditions, conditionNotificationTemplateSynchronized)
	r.Log.Info("could not find matching instances")
	return ctrl.Result{Requeue: false}, nil
}

[weisdd]

In my view:

• We should not silence errors. If the client hits an error, the operator should return the error and make another reconciliation attempt;
• We should not handle error- and zero length cases in one if-block, - even in an abstract case, I would put it as a code-style issue;
• Usage of if err != nil {} ... else is not encouraged in Go either. - Normally, you would have something like if err != nil { return err } and then, on a separate line, the next independent block.

I think notification policy controller offers a more or less optimal example:

	instances, err := GetMatchingInstances(ctx, r.Client, notificationPolicy.Spec.InstanceSelector)
	if err != nil {
		setNoMatchingInstance(&notificationPolicy.Status.Conditions, notificationPolicy.Generation, "ErrFetchingInstances", fmt.Sprintf("error occurred during fetching of instances: %s", err.Error()))
		meta.RemoveStatusCondition(&notificationPolicy.Status.Conditions, conditionNotificationPolicySynchronized)
		r.Log.Error(err, "could not find matching instances")
		return ctrl.Result{RequeueAfter: RequeueDelay}, err
	}

	if len(instances.Items) == 0 {
		meta.RemoveStatusCondition(&notificationPolicy.Status.Conditions, conditionNotificationPolicySynchronized)
		setNoMatchingInstance(&notificationPolicy.Status.Conditions, notificationPolicy.Generation, "EmptyAPIReply", "Instances could not be fetched, reconciliation will be retried")
		return ctrl.Result{}, nil
	}

Though, the last log line can be improved (we can do that later, in batch).

[Baarsgaard]

The idiomatic go code would then, in this case, be?:

instances, err := GetScopedMatchingInstances(controllerLog, ctx, r.Client, notificationTemplate)
if err != nil {
	setNoMatchingInstancesCondition(&notificationTemplate.Status.Conditions, notificationTemplate.Generation, err)
	meta.RemoveStatusCondition(&notificationTemplate.Status.Conditions, conditionNotificationTemplateSynchronized)
	return ctrl.Result{}, fmt.Errorf("could not find matching instances: %w", err)
}
if len(instances) == 0 {
	setNoMatchingInstancesCondition(&notificationTemplate.Status.Conditions, notificationTemplate.Generation, err)
	meta.RemoveStatusCondition(&notificationTemplate.Status.Conditions, conditionNotificationTemplateSynchronized)
	return ctrl.Result{RequeueAfter: RequeueDelay}, nil
}

[weisdd]

Yeah. As you can see, it's much cleaner and easier to follow :) I would only add a new line before if len(instances) == 0 for better readability.

[weisdd]

Suggested change

	_, err = r.getNotificationTemplateByName(ctx, instance, notificationTemplate)
	if err != nil {
	return fmt.Errorf("getting notification template by name: %w", err)
	}

If a template is absent, it should not be considered an error, otherwise we'll have a never-ending reconciliation. Grafana API returns 204 (No Content) when someone attempts to remove non-existing template, it's treated as success by the Grafana API client we're using.