-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Extend 'addFixedExposedPort' method with InternetProtocol argument #586
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -644,7 +644,7 @@ public SELF withExposedPorts(Integer... ports) { | |
| } | ||
|
|
||
| /** | ||
| * Add a container port that should be bound to a fixed port on the docker host. | ||
| * Add a TCP container port that should be bound to a fixed port on the docker host. | ||
| * <p> | ||
| * Note that this method is protected scope to discourage use, as clashes or instability are more likely when | ||
| * using fixed port mappings. If you need to use this method from a test, please use {@link FixedHostPortGenericContainer} | ||
|
|
@@ -654,7 +654,22 @@ public SELF withExposedPorts(Integer... ports) { | |
| * @param containerPort | ||
| */ | ||
| protected void addFixedExposedPort(int hostPort, int containerPort) { | ||
| portBindings.add(String.format("%d:%d", hostPort, containerPort)); | ||
| addFixedExposedPort(hostPort, containerPort, InternetProtocol.TCP); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIR if the protocol is not specified, it will use both, no? If so, this is a breaking change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No @bsideup by default it is TCP. Proof: https://docs.docker.com/config/containers/container-networking/
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
| } | ||
|
|
||
| /** | ||
| * Add a container port that should be bound to a fixed port on the docker host. | ||
| * <p> | ||
| * Note that this method is protected scope to discourage use, as clashes or instability are more likely when | ||
| * using fixed port mappings. If you need to use this method from a test, please use {@link FixedHostPortGenericContainer} | ||
| * instead of GenericContainer. | ||
| * | ||
| * @param hostPort | ||
| * @param containerPort | ||
| * @param protocol | ||
| */ | ||
| protected void addFixedExposedPort(int hostPort, int containerPort, InternetProtocol protocol) { | ||
| portBindings.add(String.format("%d:%d/%s", hostPort, containerPort, protocol.toDockerNotation())); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package org.testcontainers.containers; | ||
|
|
||
| /** | ||
| * The IP protocols supported by Docker. | ||
| */ | ||
| public enum InternetProtocol { | ||
|
|
||
| TCP, | ||
| UDP; | ||
|
|
||
| public String toDockerNotation() { | ||
| return name().toLowerCase(); | ||
| } | ||
|
|
||
| public static InternetProtocol fromDockerNotation(String protocol) { | ||
| return valueOf(protocol.toUpperCase()); | ||
| } | ||
| } |
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'd rather not leak docker-java implementation details into the public API. Instead I think we should use our own Enums 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.
Good point @kiview
Fixed.