Test/ run-opts-configurable-host #3293
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: intojhanurag The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
bot
added
the
size/L
knative-prow
bot
added
size/M
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3293 +/- ##
==========================================
- Coverage 54.74% 54.71% -0.03%
==========================================
Files 168 168
Lines 19605 19602 -3
==========================================
- Hits 10732 10726 -6
- Misses 7807 7811 +4
+ Partials 1066 1065 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/ok-to-test |
knative-prow
bot
added
the
ok-to-test
|
Hey @gauron99 , Please take a look on it . I added the suggestions of the preivous PR ( because that PR was messed up ). |
gauron99
left a comment
gauron99
left a comment
There was a problem hiding this comment.
Thanks! just a few naming nits. See below
pkg/functions/runner.go
Outdated
| if val == "" { | ||
| val = "localhost" | ||
| } | ||
| return val, "8080" |
There was a problem hiding this comment.
use the default values defined as variables for host and port. Also the TODO at the top of the file // TODO allow to be altered via a runOpt can be removed since we are adding this option to edit the host
pkg/functions/runner_test.go
Outdated
| name string | ||
| val string | ||
| host string | ||
| port string |
There was a problem hiding this comment.
can we name this something clearer like expectedHost and expectedPort? so its clear that those values are an expected result. Or the other way around; specify input or inputString instead of the ambiguous val
pkg/functions/runner.go
Outdated
| err io.Writer | ||
| } | ||
|
|
||
| func ParseAddressFlag(val string) (string, string) { |
There was a problem hiding this comment.
| func ParseAddressFlag(val string) (string, string) { | |
| func ParseAddress(val string) (string, string) { |
pkg/functions/runner_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func TestParseAddressFlag(t *testing.T) { |
There was a problem hiding this comment.
| func TestParseAddressFlag(t *testing.T) { | |
| func TestParseAddress(t *testing.T) { |
knative-prow
bot
added
size/L
|
Hey @gauron99 , Now is it good to go ? |
gauron99
left a comment
gauron99
left a comment
There was a problem hiding this comment.
Thanks for the cleanups. I reviewed the functionality and have some suggestions. I tested the functionality a little locally and I think my suggestions should cover all the cases. If you discover something Im missing please let me know!
| } | ||
| } | ||
|
|
||
| func TestParseAddress(t *testing.T) { |
There was a problem hiding this comment.
every case which has implicit defaulting should expect the defaultX variable value instead of hardcoded string so the test doesnt have to be re-written if the default changes
pkg/functions/runner.go
Outdated
| } | ||
| } | ||
| host, port := ParseAddress(address) | ||
| explicitPort := address != "" && strings.Contains(address, ":") |
There was a problem hiding this comment.
this : check wont work. IPv6 addresses contain the symbol and also for IPv4 you can specify host like this 127.0.0.1: - SplitHostPort accepts this just fine. I would add a return value to ParseAddress which specifies if port was or was not given. This way we can decide when we know if we are returning defaultPort or port because it was specified in the address flag. Check my comment for the ParseAddress function.
pkg/functions/runner.go
Outdated
| func ParseAddress(val string) (string, string) { | ||
| if h, p, err := net.SplitHostPort(val); err == nil { | ||
| return h, p | ||
| } | ||
|
|
||
| if val == "" { | ||
| return defaultRunHost, defaultRunPort | ||
| } | ||
| return val, defaultRunPort | ||
| } |
There was a problem hiding this comment.
I would leverage the net library and rely on what it accepts - adhering to the standards that are already used in this default, widely used package. The one exception is the last return which returns the given value val as host and adds the default Port. I think this should cover all the cases?
The suggestion below is a pseudo-code.
| func ParseAddress(val string) (string, string) { | |
| if h, p, err := net.SplitHostPort(val); err == nil { | |
| return h, p | |
| } | |
| if val == "" { | |
| return defaultRunHost, defaultRunPort | |
| } | |
| return val, defaultRunPort | |
| } | |
| func ParseAddress(val string) (host, port string, explicitPort bool) { | |
| if val == "" { | |
| return defaultRunHost, defaultRunPort | |
| // explicit port false | |
| } | |
| if h, p, err := net.SplitHostPort(val); err == nil { | |
| if h == "" { | |
| // set default host to return value | |
| } | |
| if p == "" { | |
| // set default port | |
| // explicitPort is false | |
| } else { | |
| // set explicit port to true | |
| } | |
| return | |
| } | |
| return val, defaultRunPort | |
| // explicit port is false | |
| } |
|
Hey @gauron99, PTAL now , Good eye btw :) |
2 participants
Follow up: #3275