-
Notifications
You must be signed in to change notification settings - Fork 2
chore: separate actions from the controller service #203
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
Conversation
| } | ||
|
|
||
| func (h ActionHandlers) Close() error { | ||
| return h[reflect.TypeOf(&castai.ActionCreateEvent{})].(*CreateEventHandler).Close() |
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 can panic, right?
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.
not really when used with the constructor, but this is mostly copy & paste from the existing place
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.
ok, maybe we should make it not panic even in this case, minor change
| for k := range s.actionHandlers { | ||
| s.actionHandlers[k] = handler | ||
| testActionHandlers := map[reflect.Type]actions.ActionHandler{ | ||
| reflect.TypeOf(&castai.ActionDeleteNode{}): handler, |
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.
We used to initialize them all with mock handler, correct? Does this mean we only test these 3?
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.
correct, only 3 action types are actually tested here. I'd argue if only fake actions should be used instead as it's mostly to test if the controller implementation works correctly
the controller constructor takes dependencies which are only used to initialize the default actions even if those are not used anywhere else in the implementation. Extract the actions to simplify the controller constructor and inject them instead.