Refactor BIOSSettingsSetReconciler and improve watch setup
#571
Conversation
afritzler
force-pushed
the
enh/biossettingsset
branch
from
0a6858b to
7a606bd
Compare
Nuckal777
left a comment
Nuckal777
left a comment
There was a problem hiding this comment.
Thanks, just some nitpicks from my side.
| } | ||
| return reqs | ||
|
|
||
| result := make([]ctrl.Request, 0, 1) |
There was a problem hiding this comment.
Capacity should probably be len(reqs).
internal/controller/helper.go
Outdated
| func handleIgnoreAnnotationPropagation(ctx context.Context, log logr.Logger, c client.Client, parentObj client.Object, ownedObjects client.ObjectList) error { | ||
| var errs []error | ||
| _ = meta.EachListItem(ownedObjects, func(obj runtime.Object) error { | ||
| if err := meta.EachListItem(ownedObjects, func(obj runtime.Object) error { |
There was a problem hiding this comment.
Taking the error from meta.EachListItem changes the semantics. Previously the whole list of items was iterated and the patching was always tried for each object. Now, remaining items after an item caused an error would be ignored.
internal/controller/helper.go
Outdated
| return nil | ||
| }) | ||
| }); errs != nil { | ||
| errs = append(errs, err) |
There was a problem hiding this comment.
If we use the error from meta.EachListItem() the errs slice isn't needed anymore.
internal/controller/helper.go
Outdated
| func handleRetryAnnotationPropagation(ctx context.Context, log logr.Logger, c client.Client, parentObj client.Object, ownedObjects client.ObjectList) error { | ||
| var errs []error | ||
| _ = meta.EachListItem(ownedObjects, func(obj runtime.Object) error { | ||
| if err := meta.EachListItem(ownedObjects, func(obj runtime.Object) error { |
There was a problem hiding this comment.
Taking the error from meta.EachListItem changes the semantics. Previously the whole list of items was iterated and the patching was always tried for each object. Now, remaining items after an item caused an error would be ignored.
internal/controller/helper.go
Outdated
| return nil | ||
| }) | ||
| }); errs != nil { | ||
| errs = append(errs, err) |
There was a problem hiding this comment.
If we use the error from meta.EachListItem() the errs slice isn't needed anymore.
|
Thanks a lot @Nuckal777 for the feedback. I addressed your comments and rolled back the error handling in the propagation methods. Lets deal with that in a separate PR. |
Proposed Changes
Refactor
BIOSSettingsSetReconcilerand improve watch setup.