Investigate getting rid of CompositePredicate

[kaovilai]

There's a lot of code in this repo around composite predicate and switching predicates based on object triggering event.

I think its cause we just happen to use EventFilter that applies to all Fors, Watches.

It may be better to get rid of all those complicated logic if you could contain a predicate to within a type of watch.

Perhaps something like below. If we could get rid of at least 10 files out of this would be good.

oadp-non-admin/internal/controller/nonadmindownloadrequest_controller.go

Lines 84 to 109 in 0df85a8

	For(&nacv1alpha1.NonAdminDownloadRequest{}, builder.WithPredicates(ctrlPredicate.Funcs{
	// TODO: DeleteFunc: , delete velero download requests?
	// Process creates
	CreateFunc: func(_ event.TypedCreateEvent[client.Object]) bool { return true },
	// TODO: UpdateFunc: , potentially don't process updates?
	// TODO: GenericFunc: , what to do with generic events?
	})).
	Named("nonadmindownloadrequest").
	Watches(&velerov1.DownloadRequest{}, handler.Funcs{
	UpdateFunc: func(ctx context.Context, tue event.TypedUpdateEvent[client.Object], rli workqueue.RateLimitingInterface) {
	if dr, ok := tue.ObjectNew.(*velerov1.DownloadRequest); ok && dr.Status.DownloadURL != "" {
	log := function.GetLogger(ctx, dr, "VeleroDownloadRequestHandler")
	log.V(1).Info("DownloadRequest populated with url")
	// on update, we need to reconcile NonAdminDownloadRequests to update
	rli.Add(reconcile.Request{
	NamespacedName: types.NamespacedName{
	Namespace: constant.NadrOriginNamespaceAnnotation,
	Name: constant.NadrOriginNameAnnotation,
	},
	})
	}
	},
	// DeleteFunc: , TODO: if velero DownloadRequests gets cleaned up, delete this?
	},
	).

[kaovilai]

For lurking boss, I'm talking about nuking at least half the code in https://github.com/migtools/oadp-non-admin/tree/master/internal/predicate

[weshayutin]

well sir, it's true I lurk :) I can't say I know enough to know enough yet though. I do like getting rid of code though :)
Probably a great topic for you to use your voice and microphone on a scrum call and raise your concern to the group for a discussion.

[kaovilai]

It's a cleanup/ease of maintenance sort of issue. Not breaking nor urgent.

[mpryc]

@kaovilai it's worth considering refactoring this part, but does the current implementation actually have any impact on performance or the way how the application works or just for the readability of the code? If just the second I would argue.

The Secret watch in the NaBSL PR is a great example of clear separation, It integrates cleanly with the controller, requiring only four additional lines while encapsulating logic within well-defined interfaces. Each Watch can also have different handler arguments, e.g., the Secret watch needs a Client, while NonAdminBackupStorageLocationRequest does not:

https://github.com/migtools/oadp-non-admin/pull/244/files

With the current approach we have:

• Granular control – The three-level filtering (Watch → Predicate → Handler) allows precise event handling.
• Allow to easily see which object is reacting on which event in clean way
• Consistency – This structure is already used across multiple controllers, making the code easier to follow.
• Flexibility – Different object types can have different handler arguments, as seen in the Secret watch in the NaBSL PR (#244).

In case of downloadrequest it maybe does not make sense to keep composite predicates and handlers as you don't watch too many objects, however across entire project using composite predicates in my opinion actually avoids unnecessary complexity and allows better readability.

[kaovilai]

I don't view consistency across the code base as readability.

I view, can I parse the code and understand what it does in a single pane of glass as readability.

If you want to keep current structure I suggest making some structs more descriptive or more comments.

[kaovilai]

Different object types can already use different handlers without composite predicate

[mpryc]

@kaovilai if you could propose one change to one object then we can decide which way to go, possibly backup as it has most logic in composite and different events involves different predicates within NAB.