-
Notifications
You must be signed in to change notification settings - Fork 7
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
Implement fake client builder #273
Conversation
03ebb20
to
96a73c3
Compare
// Note: PodObserver that subscribe to update events should implement a mechanism for | ||
// avoiding an update loop. They can for instance check the object is in a particular state | ||
// before updating. Additionally, they can unsubscribe from further updates. | ||
type PodObserver func(ObjectEvent, *corev1.Pod) (*corev1.Pod, bool, error) |
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.
I'm not 100% sold on the name observer
, I think it has read-only connotations for me. As the concept is somewhat similar to mutation webhooks, what about using mutator
?
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.
I also but mutator
has the problem as mutating the object is optional. What about PodEventHook
? It's more neutral.
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.
EventHook
seems a bit too long I think... Let's keep Observer then, while not great it seems to be the one we dislike the less.
continue | ||
} | ||
objectEvent := ObjectEvent(watcherEvent.Type) | ||
if event == ObjectEventAll || objectEvent == event { |
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.
I'd suggest to invert this if and continue
, ad oppose to increase indentation for the test of the function.
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.
I generally try to minimize indentation, but in this case, I found the condition becomes less readeable.
if event != ObjetEventAll && objectEvent != event {
continue
}
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.
🤔 I don't know, it does not seem that unreadable to me. I'd still prefer it.
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.
I understand that many indentation levels are not a good practice in general. But I think it is a guideline, not a hard requirement. In this case, it is only one indentation and If I have to trade between this condition with two negations and one indentation, I would prefer the indentations. It seems to me a matter of preferences, don't you think?
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.
I believe most PR discussions are (and should be) a matter of preference, but that does not make them less valid. If we could coalesce the entirety of our intuition and experience in hard requirements, we could skip them altogether and just use linters :)
But I digress. Non-negated (pun intended) conditions are indeed more readable, and although I think returning early is preferred, I'm not adamant on it.
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.
I believe most PR discussions are (and should be) a matter of preference
And I'm convinced they should not be about preferences. PR reviews should not focus not in making the code more to the reviewer's taste but to ensure we are not infringing damage to our future selves.
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.
I just realized I explained very poorly my point in my previous comments when I said it was about preferences on indentation over the alternative if condition.
What I meant to say is that in this context, my criteria was that double negation has a larger cognitive load than a single indentation. So, in reality, I wasn't talking about preferences but about perceptions, that are still subjective but not the same.
// Note: PodObserver that subscribe to update events should implement a mechanism for | ||
// avoiding an update loop. They can for instance check the object is in a particular state | ||
// before updating. Additionally, they can unsubscribe from further updates. | ||
type PodObserver func(ObjectEvent, *corev1.Pod) (*corev1.Pod, bool, error) |
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.
I find the signature of the observer function a bit confusing:
- When I register an observer I already supply an
ObjectEvent
. Does the function need to receive it again when it is called? - Returning nil as an indication of not doing anything is a valid option, but I think it is more explicit and readable if the observer function actively chooses to not do anything. This could be done, for example, by having the observer function receive as an argument a function to edit the pod, which then the observer decides whether to call or not.
- Returning both a boolean and an error seems a bit redundant, specially since both do the same thing: Breaking the observer loop. I'd suggest to return a special error instead and use
error.Is
in the loop to decide whether the error should be logged. This is a common pattern in the stdlib, e.g.WalkDirFunc
orSplitFunc
.
To exemplify the suggestions below, the signature could be:
var ErrStopWatching = errors.New("stop watching")
// PodMutatorFunc is a function that is called when a pod is added, deleted,
// or modified, and that can react to this event by mutating the pod by calling
// `mutate` with a the modified pod as an argument.
// PodMutatorFunc may return ErrStopWatching if they do not wish to continue being
// called for any other pod event.
type PodMutatorFunc func(pod *corev1.Pod, mutate func(*corev1.Pod) error) error
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.
When I register an observer I already supply an ObjectEvent. Does the function need to receive it again when it is called?
Yes, because there is the option to subscribe to all.
by having the observer function receive as an argument a function to edit the pod
I find this option overcomplicated and, as you said, "returning nil is a valid option". I prefer to keep it simple. if we later find there are use cases that are not well covered by just retuning nil, we can consider this approach.
Returning both a boolean and an error seems a bit redundant,
No, they are not redundant because the boolean indicates the observer doesn't;t want to continue observing and the error means there was an error processing the event, two very different situations. I find returning an error to signal a non-error situation and making the caller has to use check if the "error" is really an error a lot more complex than returning a simple boolean.
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.
I don't see how passing a function is overcomplicated, neither I agree that checking error types is complex. The standard library is doing (thus, by example, encouraging) this pattern in multiple instances.
I wouldn't like to block the PR over this though, specially being a very internal API. Let's keep it as it is if you think it's cleaner.
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.
I don't see how passing a function is overcomplicated
Because there's a simpler option that is retuning a value.
neither I agree that checking error types is complex
it is not complex if the error is an error, but it is when errors are abused to signal non-error situations.
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
70fd444
to
b1e05f0
Compare
Description
Implements a builder for fake clients that facilitates common tasks.
In particular, it allows attaching observers for object events (added, modified, deleted) that can react to those events and eventually update the objects. This simplifies tests that require reacting to events (for example, changing the status of a container once it is created) by:
Checklist:
make lint
) and all checks pass.make test
) and all tests pass.make e2e-xxx
foragent
,disruptors
,kubernetes
orcluster
related changes)