restartmanager: Remove RestartManager interface, and unused error return #42264
Conversation
thaJeztah
added
status/2-code-review
area/daemon
kind/refactor
thaJeztah
changed the title
|
I can remove the interface as there's only one implementation |
thaJeztah
force-pushed
the
update_the_update
branch
from
8cd4ab7
to
e6712c2
Compare
thaJeztah
force-pushed
the
update_the_update
branch
from
e6712c2
to
eb6f642
Compare
thaJeztah
force-pushed
the
update_the_update
branch
2 times, most recently
from
132dd1c
to
40cc95f
Compare
thaJeztah
changed the title
| ShouldRestart(exitCode uint32, hasBeenManuallyStopped bool, executionDuration time.Duration) (bool, chan error, error) | ||
| } | ||
|
|
||
| type restartManager struct { |
There was a problem hiding this comment.
I'm guessing it was using an interface (exported) and restartManager type (not exported) to enforce that it had to be initialised through .New(). Not sure if that's worth the extra complexity though (but open to feedback)
There was a problem hiding this comment.
Hmmm do any of the linters we're using have a way to help enforce that? I guess maybe we're probably not even using this in enough places to really worry about it?
There was a problem hiding this comment.
Yeah, I don't think we should worry about this. It's (somewhat) inherit to how Go work, and it not being "really" OO; I guess we have any types that are only functional / usable when using a constructor (or being sure all fields are set), and nothing really prevents someone from just foo := SomeType{} without setting those fields.
I think the change in this PR outweighs that theoretical "issue".
thaJeztah
force-pushed
the
update_the_update
branch
from
40cc95f
to
4e970d5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
thaJeztah
force-pushed
the
update_the_update
branch
from
4e970d5
to
7628a1b
Compare
thaJeztah
force-pushed
the
update_the_update
branch
from
7628a1b
to
d3f2717
Compare
This function would never return an error, and no code was handling errors. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It only had a single implementation, so we may as well remove the added complexity of defining it as an interface. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
update_the_update
branch
from
d3f2717
to
c5d4b6b
Compare
This function would never return an error, and no code was handling errors.
container.UpdateMonitor(): don't instance restartmanager if not needed When updating a container to set a "no" restart policy, and the container does not have a restart-policy set, there should be no need to instance a restartmanager.(removed)- A picture of a cute animal (not mandatory but encouraged)