Fix docker run --expose with an invalid port does not error out #10856
Conversation
Signed-off-by: Lei Jitang <leijitang@huawei.com>
|
Shoud I add test for this? |
|
Nice simplification! A test to validate error on invalid port would be great. |
|
We ❤️ tests :D |
|
update, add a test for expose a invalid port @jfrazelle |
| if err != nil && !strings.Contains(out, "Invalid range format for --expose") { | ||
| t.Fatalf("failed to run container, output: %q", out) | ||
| } | ||
| if !strings.Contains(out, "Invalid range format for --expose") { |
There was a problem hiding this comment.
Failing test would never reach this point since you are testing for this above @ 2726.
There was a problem hiding this comment.
052e3bc to
234fd5e
Compare
|
@cpuguy83 @jfrazelle the test has updated |
| // test docker run expose a invalid port | ||
| func TestRunExposePort(t *testing.T) { | ||
| runCmd := exec.Command(dockerBinary, "run", "--expose", "80000", "busybox") | ||
| _, _, err := runCommandWithOutput(runCmd) |
There was a problem hiding this comment.
Sorry, it's often extremely helpful to get the actual output text here as part of the test's error message, and then we can do a !strings.Contains(<some error string here>) to make sure we go the error we thought we should.
|
LGTM after nits with the test. |
|
thanks for the test! looks good ping me after the changes for the merge :D |
Signed-off-by: Lei Jitang <leijitang@huawei.com>
234fd5e to
34b7c10
Compare
|
update the test, ping @jfrazelle |
|
LGTM |
|
thanks @coolljt0725 |
Fix docker run --expose with an invalid port does not error out
Signed-off-by: Lei Jitang leijitang@huawei.com
Fixes #10836
when
docker run --exposewith an invalid port, there is no error out.This patch fix this and have some refactor to previous code to make the code
more simplified. Since function
parsers.ParsePortRangewill dostrings.Contains(e, "-")and if the port is not a range but a specified one, the returnstart portandend portare the same. So there is no need to doif strings.Contains(e, "-")in
parse.go