-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
Codecov Report
@@ Coverage Diff @@
## master #186 +/- ##
==========================================
- Coverage 78.23% 77.95% -0.28%
==========================================
Files 86 87 +1
Lines 3542 3570 +28
==========================================
+ Hits 2771 2783 +12
- Misses 703 718 +15
- Partials 68 69 +1 Continue to review full report at Codecov.
|
@@ -0,0 +1,97 @@ | |||
// Copyright (c) 2018 Uber Technologies, Inc. |
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.
nit: change the package name to listenadress
PortType Resolver `yaml:"portType" validate:"nonzero"` | ||
|
||
// Value is the config specified port if using config port type. | ||
Value *string `yaml:"value"` |
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.
Could you make this an int
?
} | ||
p := c.Port | ||
|
||
var port string |
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.
nit: make this an int
err := fmt.Errorf("missing port env var value using: resolver=%s, name=%s", | ||
string(p.PortType), *p.EnvVarName) | ||
return "", err | ||
} |
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.
ensure the port is a valid int (do the Atoi) after getting it here.
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 w/ a few nits
hostname = "0.0.0.0" | ||
) | ||
|
||
func TestListenAddressResolver(t *testing.T) { |
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.
nit: Can you convert these to tabled tests since they're hitting the same function?
) | ||
|
||
var ( | ||
port = "9000" |
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.
Add in a test for invalid (<0,>2^16) and non-alpha ports?
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 +2
No description provided.