Pod controller change stop signature #2056
Conversation
|
Thanks for submitting this pull request 🎉. The team will review it soon and get back to you. If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document. |
e-sumin
force-pushed
the
pod_controller_change_stop_signature
branch
from
2a444c2
to
4b89667
Compare
| @@ -40,9 +40,9 @@ var ( | |||
| type PodController interface { | |||
| PodName() string | |||
| Pod() *corev1.Pod | |||
| StartPod(ctx context.Context, stopTimeout time.Duration) error | |||
| StartPod(ctx context.Context) error | |||
There was a problem hiding this comment.
Removing stopTimeout is good idea. I was thinking would it be a good idea if StartPod accepts another parameter that decides if StartPod wold also wait for the pod to be Running.
But I see we have another method that does that.
I am not sure if removing WaitForPodReady method and having that handled in StartPod would be better idea but it's worth thinking about.
There was a problem hiding this comment.
Originally it was written in a way you are describing, but I think that generic interface should provide granular methods, and consumers are able to combine them as they would like. Also that makes tests easier.
Change Overview
Since PodController is a general purpose interface, it makes no sense to pass timeout to StartPod method and keep it internally. It's much better to pass it to StopPod.
For the same reason it makes no sense to StopPod when WaitForPodReady failed. Interface user might want to do something before stopping pod.
Pull request type
Please check the type of change your PR introduces:
Test Plan