Emptiness reconcile now properly backs off #664
Conversation
|
✔️ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: 8ad0d83 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/6139265b3f7b5100071a76ad |
pkg/utils/functional/functional.go
Outdated
| for _, duration := range durations { | ||
| if duration > max { | ||
| max = duration | ||
| if (duration < min && duration != 0) || (duration != 0 && min == 0) { |
There was a problem hiding this comment.
if you initialize min to the first duration, you can simplify this check by removing all the 0s
There was a problem hiding this comment.
IIUC, if the first duration is 0, we still run into the problem.
There was a problem hiding this comment.
oh you want to return the smallest non-zero, ok. I'm guessing it's fine to return 0 if all the durations are 0 then?
pkg/utils/functional/functional.go
Outdated
| for _, duration := range durations { | ||
| if duration > max { | ||
| max = duration | ||
| if (duration < min && duration != 0) || (duration != 0 && min == 0) { |
There was a problem hiding this comment.
Just for safety, clarity, can you use https://pkg.go.dev/time#Time.IsZero ?
There was a problem hiding this comment.
Since we're comparing time.Duration and not time.Time, this kind of comparison should be okay.
From their code
type Duration int64
There was a problem hiding this comment.
Yeah, I realize it works either way, but I think IsZero is preferable from a readability standpoint (eg as a reader of the code I don't have to think as much :-))
There was a problem hiding this comment.
So since there's not a time.Duration.IsZero() function, are you suggesting I add it?
1. Issue, if available:
#663
2. Description of changes:
3. Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.