Repository navigation
Conversation
vkhoroz
reviewed
Oct 22, 2026
vkhoroz
left a comment
vkhoroz
left a comment
Member
There was a problem hiding this comment.
LGTM
I only have one mental teaser to think of.
| if err := json.Unmarshal(target.Custom, &custom); err != nil || !slices.Contains(custom.Tags, u.Tag) { | ||
| continue | ||
| } | ||
| version, _ := strconv.Atoi(custom.Version) |
Member
There was a problem hiding this comment.
Need to check for invalid version. I guess that's an error.
|
|
||
| // updateTargets returns the targets an update installs: the latest version carrying the update's tag, per | ||
| // hardware id. It returns nil when the update has no targets metadata. | ||
| func (s Storage) updateTargets(name string) (map[string]bool, error) { |
Member
There was a problem hiding this comment.
iiuc it is latestUpdateTargets, right?
Comment on lines
+101
to
+104
| // A device reports other targets too, like the one it rolls back to after this update fails | ||
| if targets != nil && len(status.TargetName) > 0 && !targets[status.TargetName] { | ||
| continue | ||
| } |
Member
There was a problem hiding this comment.
This seems to be correct in a static environment.
I also believe that it is correct in a dynamic environment too:
- A rollout log only contains logs for a specific update.
- But, all logs from a device which is assigned to this update are coming into it; even if the device was effectively updated to some different update (e.g. from a previous rollout).
- So, what this thing does - it filters out non-update related events; only keeping update related events.
Is this how it works?
I am inclined to think this is what we want.
But, it might hide problems, or seem confusing, as we may show an event which wasn't the last as the last one.
Possibly, we need to somehow show both events (for each device participating in an update):
- the last event related to a given update;
- the absolute last event.
A device also reports the target it goes back to when an update fails: the previous one, reinstalled after an app fails to start or synced after the boot loader falls back. That success landed last in the update's rollout log and the summary counted the failed update as succeeded. Signed-off-by: Andre Detsch <andre.detsch@foundries.io> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
detsch
force-pushed
the
detsch/rollout-summary-target
branch
from
October 24, 2026 14:35
ddcc328 to
5745c39
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When an update fails, the device also reports the target it goes back to, under the same update. That happens in two cases:
Either way, the previous target's
Installation completed; succeededis the device's last line in the update's rollout log. The summary keeps only each device's last status, so the failed update counted as a success. Lab runs caught this on the rollout summary of a failed app update.The summary and state-for queries now count only events for the update's own target: the latest version with the update's tag, per hardware id, from its
targets.json. Events without a target name still count. So do all events of an update without targets metadata or without a matching target, as before.Tested with
go test ./...(all packages pass) and golangci-lint onstorage/. The newTestUpdateSummaryCountsOnlyTheUpdateTargetcovers both rollback patterns, and it fails without the change (3 succeeded instead of 2 failed and 1 succeeded).🤖 Generated with Claude Code