-
Notifications
You must be signed in to change notification settings - Fork 599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor: Pass client.Object types by reference #4040
Conversation
The client.Object interface is implemented only by the pointer type. Subtle bugs can arise from passing a value when you intended to pass a reference. See: 96132b8 See: https://go.dev/wiki/MethodSets
7123613
to
34cdcbf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Per your comment on the PR (should commits 2-4 wait): I think getting them in early doesn't increase our complexity, so seems fine to merge in now, yeah?
d5a3e49
to
bb37061
Compare
"sigs.k8s.io/controller-runtime/pkg/reconcile" | ||
) | ||
|
||
// Requests converts objects to a slice of [reconcile.Request]. | ||
func Requests[T client.Object](objects ...T) []reconcile.Request { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has convinced me that "slice of plain objects" pays dividends, even now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less code ++
I'm confident about the first commit. The others switch from List types, which seem like a Kubernetes API concern, to slices of "plain old objects." That seems better positioned for generics and
slices
, but I didn't look for those opportunities here. 🤔 Perhaps those commits should wait until then.