refactor(update): clean up actions/update #1895
Conversation
- removes unwieldy SkipUpdate return value in favor of errors.Is - generalizes the code for all four phases - allows timeout to be defined for all phases - enables explicit unit in timeout label values (in addition to implicit minutes)
renames the container.Stale field to what it's actually used for, as staleness is not the only factor used to decide whether a container should be updated anymore also hides the private field along with linkedToRestarting
- move common arguments to a shared struct - remove unused fields - fix outdated names - improve logging/error handling
piksel
force-pushed
the
refactor-update
branch
from
a482680
to
a42eb28
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1895 +/- ##
==========================================
- Coverage 70.55% 69.97% -0.59%
==========================================
Files 26 27 +1
Lines 2493 2498 +5
==========================================
- Hits 1759 1748 -11
- Misses 633 648 +15
- Partials 101 102 +1 ☔ View full report in Codecov by Sentry. |
| @@ -208,7 +208,7 @@ func (c Container) Links() []string { | |||
| // ToRestart return whether the container should be restarted, either because | |||
| // is stale or linked to another stale container. | |||
There was a problem hiding this comment.
The comment describing the function is out of date now with the revised logic
| func stopStaleContainer(container types.Container, client container.Client, params types.UpdateParams) error { | ||
| if container.IsWatchtower() { | ||
| log.Debugf("This is the watchtower container %s", container.Name()) | ||
| func (us *updateSession) stopContainer(c types.Container) error { |
There was a problem hiding this comment.
Consider keeping this variable named "container" rather than "c" to help in readability and to match other functions like restartContainer
There was a problem hiding this comment.
Can't, since that is the name of an imported package. This is the common go "solution". :/
| failed[c.ID()] = err | ||
| } else if c.IsStale() { | ||
| } else if c.IsMarkedForUpdate() { | ||
| // Only add (previously) stale containers' images to cleanup | ||
| cleanupImageIDs[c.ImageID()] = true |
There was a problem hiding this comment.
To me, rather than at line 89, this is a better place to call
us.progress.MarkForUpdate(c.ID())
That would avoid setting a container's state to UpdatedState before it's actually updated. That would also avoid an item going from UpdatedState to FailedSate in cases where something fails (or if something later breaks with properly setting FailedState, misrepresenting the item as having been updated if it wasn't). This is probably not a big deal but I mention it because it was confusing to me initially why we're setting something to (past-tense) UpdatedState before any action is completed.
| } | ||
|
|
||
| UpdateImplicitRestart(containers) | ||
|
|
||
| var containersToUpdate []types.Container | ||
| for _, c := range containers { | ||
| if !c.IsMonitorOnly(params) { | ||
| if c.ToRestart() { |
There was a problem hiding this comment.
In testing my latest changes that assume this PR will be coming in, I found that I needed to change this:
if c.ToRestart() {
to
if c.ToRestart() && !c.IsMonitorOnly(params) {
In order to avoid the monitor only test cases failing. Consider whether to make the same change here in your code or just to incorporate the !c.IsMonitorOnly(params) test within the definition of ToRestart()
There was a problem hiding this comment.
It's not needed anymore, since shouldUpdate (and later markedForUpdate) will never be true for monitorOnly.
|
Hi @piksel . Just curious if there was any movement on this PR? Would love to update my other PR for deferred updates to match this code and get that resolved too whenever you're ready. LMK if there's anything else I can do to help here! |
|
It would be nice to finish this work |
This PR tries to refactor the update code flow to make it easier to understand, maintain and extend.
actions/update:
updateSession)lifecycle:
container:
StaletoMarkedForUpdaterenames the
container.Stalefield to what it's actually used for, as stalenessis not the only factor used to decide whether a container should be updated anymore