improve: facelift samples to use ReconcileUtils #3135
Conversation
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
openshift-merge-robot
added
the
needs-rebase
csviri
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
| .inNamespace(ns) | ||
| .resource(desiredHtmlConfigMap) | ||
| .serverSideApply(); | ||
| ReconcileUtils.serverSideApply(context, desiredHtmlConfigMap); |
There was a problem hiding this comment.
This makes me think that maybe we should remove ReconcileUtils altogether and put its method on Context instead.
There was a problem hiding this comment.
ReconcileUtils now contains all kind of other methods, see also PR #3130
So we would need a Reconcile utils anyways.
For adding those methods into Context I have mixed feelings: it would be easier to discover for sure, on the other hand having basically utility methods there does not feel right to me. Also I'm a little bit affraid that would make too much noise/pollute the Context, so would be harder to understand the details of context for the user.
Also, I assume some more such methods might come in the future.
So with design wise I would formulate it like: have minimal stuff exposed absolutely necessary through the Context, and putting everything else, helper methods into the ReconcileUtils.
There was a problem hiding this comment.
But up to discussion.
There was a problem hiding this comment.
It makes sense but from a design-perspective, I'm always suspicious of static functions that all take the same object parameter. It's usually an indication that this function should probably be a method of that object.
Regarding that other PR, it doesn't seem like these new methods are particularly related to reconciliation, which weakens the unity of ReconcileUtils, imo. I mean I get not wanting to create lots of utility classes for discovery purposes but, on the same token, putting too many loosely coupled functions in a single object doesn't help discovery either (and yes, that applies to Context as you correctly pointed out).
There was a problem hiding this comment.
TBH, I can see both points but I'm more inclined to put these methods on context because less visual noise for the user and easier discoverability. I rarely scroll down all available methods of an object. I think most people will read the docs first and understand how to call everything. So to me it's between static method vs the object already in params, and the second case wins.
There was a problem hiding this comment.
the thing is that the param in that Reconcile utils is not the primary resource but a resource that user explicitly creates. Let's have a separate issues for this, dicsuss it on community meeting.
There was a problem hiding this comment.
let's continue here, will merge this and the other PR, and we can still refactor before the release:
#3136
openshift-merge-robot
removed
the
needs-rebase
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
4 participants
No description provided.