fix: send notification on single container update#2357
fix: send notification on single container update#2357kmendell merged 2 commits intogetarcaneapp:mainfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| out.Duration = time.Since(start).String() | ||
|
|
||
| if s.notificationService != nil { | ||
| if notifErr := s.notificationService.SendContainerUpdateNotification(ctx, containerName, normalizedRef, inspectBefore.Image, s.normalizeRef(normalizedRef)); notifErr != nil { |
There was a problem hiding this comment.
Redundant double-normalization for
newDigest
normalizedRef is already s.normalizeRef(imageRef), so passing s.normalizeRef(normalizedRef) as newDigest applies the normalizer twice to the same string, making imageRef and newDigest identical in the notification. The bulk-update path intentionally passes the raw p.newRef as imageRef and s.normalizeRef(p.newRef) as newDigest so the two fields can differ (e.g. "nginx:latest" vs "docker.io/library/nginx:latest"). To match that pattern here, pass the pre-normalized imageRef as the second argument and normalizedRef as the last:
| if notifErr := s.notificationService.SendContainerUpdateNotification(ctx, containerName, normalizedRef, inspectBefore.Image, s.normalizeRef(normalizedRef)); notifErr != nil { | |
| if notifErr := s.notificationService.SendContainerUpdateNotification(ctx, containerName, imageRef, inspectBefore.Image, normalizedRef); notifErr != nil { |
The same applies to the standalone path at line 662.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/internal/services/updater_service.go
Line: 628
Comment:
**Redundant double-normalization for `newDigest`**
`normalizedRef` is already `s.normalizeRef(imageRef)`, so passing `s.normalizeRef(normalizedRef)` as `newDigest` applies the normalizer twice to the same string, making `imageRef` and `newDigest` identical in the notification. The bulk-update path intentionally passes the raw `p.newRef` as `imageRef` and `s.normalizeRef(p.newRef)` as `newDigest` so the two fields can differ (e.g. `"nginx:latest"` vs `"docker.io/library/nginx:latest"`). To match that pattern here, pass the pre-normalized `imageRef` as the second argument and `normalizedRef` as the last:
```suggestion
if notifErr := s.notificationService.SendContainerUpdateNotification(ctx, containerName, imageRef, inspectBefore.Image, normalizedRef); notifErr != nil {
```
The same applies to the standalone path at line 662.
How can I resolve this? If you propose a fix, please make it concise.
Fixes #1802.
What happened
The bulk Update Containers button sends Discord/Apprise notifications correctly. The per-container Update button (single container) was silently skipping them —
SendContainerUpdateNotificationwas never called in either the compose-project or standalone success branch ofUpdateSingleContainer.Fix
Added the notification call in both success paths inside
UpdateSingleContainer, mirroring exactly whatrestartContainersUsingOldIDsalready does for bulk updates.Testing
Trigger a single-container update via the container's Update button → notification fires. Bulk update → still fires. No-op update (digest unchanged) → no notification, same as before.
Disclaimer Greptiles Reviews use AI, make sure to check over its work.
To better help train Greptile on our codebase, if the comment is useful and valid Like the comment, if its not helpful or invalid Dislike
To have Greptile Re-Review the changes, mention
greptileai.Greptile Summary
Adds
SendContainerUpdateNotificationcalls to both success branches insideUpdateSingleContainer— the compose-project path and the standalone path — fixing a bug where single-container updates silently skipped Discord/Apprise notifications. The fix correctly mirrors the existing bulk-update implementation inrestartContainersUsingOldIDsand preserves the no-op-skip behavior (no notification when digest is unchanged).Confidence Score: 5/5
Safe to merge; the fix is correct and the only finding is a minor style point.
Both notification call sites use the correct function with semantically valid arguments. The only issue is a redundant double-normalization that is harmless at runtime. No P0 or P1 findings.
No files require special attention.
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix: single container update now pings y..." | Re-trigger Greptile