- Notifications
You must be signed in to change notification settings - Fork927
fix(coderd): humanize duration on notifications#14333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Almost there, please address CI issues.
coderd/util/time/duration.go Outdated
| func Humanize(d time.Duration) string { | ||
| endTime := time.Now().Add(d) | ||
| return humanize.Time(endTime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Given this is only used in two places, I'm not convinced we need any indirection in front ofhumanize.Time; we should just use it directly.
If we were using this very broadly, I could see some value is hiding this behind our own interface so we could swap out the underlying implementation, but I don't think we're there yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Happy you noticed this to reduce abstraction!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
100%, I was just too lazy to remove the first abstraction I added. My bad 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Couple minor thoughts but LGTM!
| @@ -323,14 +324,15 @@ func (e *Executor) runOnce(t time.Time) Stats { | |||
| } | |||
| } | |||
| if shouldNotifyDormancy { | |||
| dormantTime := time.Now().Add(time.Duration(tmpl.TimeTilDormant)) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
One thing to check: is the dormancy value in the database in UTC? If so, the timezone of the control plane might make things weird and we may want to add a.UTC() call in here too or usedbtime.Now()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good to know
| @@ -691,7 +691,7 @@ func TestNotificationTemplatesCanRender(t *testing.T) { | |||
| "reason": "breached the template's threshold for inactivity", | |||
| "initiator": "autobuild", | |||
| "dormancyHours": "24", | |||
| "timeTilDormant": "24h", | |||
| "timeTilDormant": "24 hours", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A test which includes not such a round number would also be helpful for illustrative purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Added
10327fb intomainUh oh!
There was an error while loading.Please reload this page.
Close#14325
I added tests for the Humanize function, but we might want to consider adding snapshot tests for the notifications in the near future.
Since these tests would require a database (as the templates are stored there), it can be challenging to update the golden files locally with our current test setup. I’m wondering if we should start considering the use of an embedded database for some tests. 🤔