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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
internal/timeout: extract package for handling timeouts #2698
Conversation
@skriss I think this makes a lot of sense and is a great improvement. I'd like to do a full review next week. |
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.
Overall, LGTM, except for one naming thing.
989ba68
to
7fd56f3
Compare
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.
LGTM, nice work.
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.
I like this a lot.
internal/timeout/timeout_test.go
Outdated
got := Parse(tc.duration) | ||
if tc.want != got { | ||
t.Errorf("Wanted %v, got %v", tc.want, got) | ||
} |
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.
Arguably a matter of taste, but I'd be inclined to test this like this:
assert.Equal(Parse(""), DefaultSetting())
assert.Equal(Parse("0"), DefaultSetting())
I'm also OK if you want to leave this as it is.
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
8cd91d4
to
9ea9544
Compare
Rebased to address merge conflicts. |
Codecov Report
@@ Coverage Diff @@
## master #2698 +/- ##
==========================================
+ Coverage 76.85% 76.88% +0.03%
==========================================
Files 72 72
Lines 5702 5711 +9
==========================================
+ Hits 4382 4391 +9
Misses 1228 1228
Partials 92 92
|
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.
Looks great.
…ur#2698) Adds the internal/timeout package, which exposes a structured type for representing timeouts, and updates existing code to use this new package. Signed-off-by: Steve Kriss <krisss@vmware.com>
WIP, let me know if this makes sense. I got sick of keeping track of what -1, 0, "", and "infinity" meant between Contour and Envoy so added a proper type for it.
If this makes sense, then we can use the same package for processing the config file timeout values so the syntax is consistent everywhere.
Updates #2697