feat: add eligible_at timestamp to cooldown deferral logs and notifications - #1698
Merged
Conversation
…stem - Add `EligibleAt` field to `CooldownError` struct to store when container becomes eligible for update - Extend `buildCooldownError` to accept precomputed `eligibleAt` time from remaining duration calculation - Add `cooldownEligibleAt` field to `ContainerStatus` with `CooldownEligibleAt()` getter method - Update `SetCooldownInfo` signature in both `ContainerStatus` and `Progress` to include `eligibleAt` parameter - Include `eligible_at` in notification template data (formatted as RFC1123) and log output (RFC3339) - Add `RFC1123` template function for formatting timestamps in notification templates - Update all callers to pass `eligibleAt` through the cooldown information flow
…estamp in cooldown defaults test - Correct indentation of cooldown notification template to align with adjacent template branches - Verify `CooldownEligibleAt()` returns zero time by default in `TestContainerStatus_CooldownDefaults`
2 tasks
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR extends the cooldown error reporting pipeline to include an eligibility timestamp. The change adds an ChangesCooldown eligibility timestamp propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1698 +/- ##
==========================================
- Coverage 74.79% 74.61% -0.18%
==========================================
Files 59 60 +1
Lines 9949 10052 +103
==========================================
+ Hits 7441 7500 +59
- Misses 2243 2284 +41
- Partials 265 268 +3
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/session/progress_test.go (1)
1305-1352:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing assertion for
CooldownEligibleAt().The
wantcases now populatecooldownEligibleAt(Lines 1192, 1217, 1262), but this verification loop only checksCooldownPassed,CooldownAge,CooldownDelay, andCooldownRemaining. The eligibility timestamp—the field this PR adds—is never asserted, so propagation throughProgress.SetCooldownInfois not actually validated.💚 Proposed fix to assert the eligible-at timestamp
if gotStatus.CooldownRemaining() != wantStatus.cooldownRemaining { t.Errorf( "Progress.SetCooldownInfo() CooldownRemaining for %v = %v, want %v", id, gotStatus.CooldownRemaining(), wantStatus.cooldownRemaining, ) } + + if gotStatus.CooldownEligibleAt() != wantStatus.cooldownEligibleAt { + t.Errorf( + "Progress.SetCooldownInfo() CooldownEligibleAt for %v = %v, want %v", + id, + gotStatus.CooldownEligibleAt(), + wantStatus.cooldownEligibleAt, + ) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/session/progress_test.go` around lines 1305 - 1352, Add an assertion in the verification loop to check that the eligibility timestamp is propagated: call gotStatus.CooldownEligibleAt() and compare it to wantStatus.cooldownEligibleAt, and emit a t.Errorf (matching the existing style) when they differ; place this check alongside the other Cooldown* assertions in the loop that validates Progress.SetCooldownInfo so the new cooldownEligibleAt field is actually tested.
🧹 Nitpick comments (1)
pkg/notifications/templates/funcs.go (1)
47-56: 💤 Low valueConsider logging parse failures for consistency and observability.
The
formatRFC1123function silently returns the original string when parsing fails. For consistency with thetoJSONhelper (lines 34-45), which logs marshaling errors withlogrus.Warn, consider logging parse failures to aid debugging of notification templates.📊 Suggested enhancement for error logging
func formatRFC1123(s string) string { t, err := time.Parse(time.RFC3339, s) if err != nil { + logrus.WithError(err).WithFields(logrus.Fields{ + "value": s, + }).Warn("Failed to parse RFC3339 timestamp in notification template") return s } return t.Format(time.RFC1123) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/notifications/templates/funcs.go` around lines 47 - 56, The formatRFC1123 function currently swallows parse errors; update it to log parse failures similarly to toJSON by calling logrus.Warnf (or the project's logger) when time.Parse returns an error, including the input string and the parse error, then continue returning the original string; this change should be made in the formatRFC1123 function so failures are observable while preserving the existing fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/session/progress_test.go`:
- Around line 1305-1352: Add an assertion in the verification loop to check that
the eligibility timestamp is propagated: call gotStatus.CooldownEligibleAt() and
compare it to wantStatus.cooldownEligibleAt, and emit a t.Errorf (matching the
existing style) when they differ; place this check alongside the other Cooldown*
assertions in the loop that validates Progress.SetCooldownInfo so the new
cooldownEligibleAt field is actually tested.
---
Nitpick comments:
In `@pkg/notifications/templates/funcs.go`:
- Around line 47-56: The formatRFC1123 function currently swallows parse errors;
update it to log parse failures similarly to toJSON by calling logrus.Warnf (or
the project's logger) when time.Parse returns an error, including the input
string and the parse error, then continue returning the original string; this
change should be made in the formatRFC1123 function so failures are observable
while preserving the existing fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc6ecd2c-8d32-4c85-92f3-70f6d1cc3082
📒 Files selected for processing (9)
internal/actions/actions.gointernal/actions/update.gopkg/container/cooldown.gopkg/notifications/common_templates.gopkg/notifications/templates/funcs.gopkg/session/container_status.gopkg/session/container_status_test.gopkg/session/progress.gopkg/session/progress_test.go
…mprove formatRFC1123 error logging - Verify `CooldownEligibleAt()` matches expected value in `TestProgress_SetCooldownInfo` - Rename `formatRFC1123` parameter from `s` to `value` for clarity - Add structured warning log with `logrus` when RFC3339 parsing fails in `formatRFC1123`
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
1 participant
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.
This PR introduces an
eligible_attimestamp field to cooldown deferral logs and notifications, showing the exact time a container becomes eligible for update.Problem
The cooldown deferral log message only showed a human-readable duration (
eligible_in, e.g. "11 hours 18 minutes 21 seconds"), forcing users to manually calculate the actual time when a container becomes eligible for update.Solution
Compute the exact eligible-at timestamp once in
evalImageAge, store it inCooldownError.EligibleAt, and thread it throughProgressandContainerStatusso it can be displayed as an RFC3339 string in logs and RFC1123 in notifications.Changes
EligibleAt time.TimetoCooldownError; computed once inevalImageAgeand passed intobuildCooldownErrorcooldownEligibleAt time.Timefield andCooldownEligibleAt()getter toContainerStatus; updatedSetCooldownInfosignatureProgress.SetCooldownInfoto thread througheligibleAt time.TimecooldownErr.EligibleAtfromCooldownErrorthrough to progress inupdate.goeligible_at(RFC3339) to the cooldown deferral log entry inactions.goRFC1123template function for human-friendly notification formatting{{RFC1123 .}}eligibleAtparameter and assertionsSummary by CodeRabbit