Change getExposedPorts to return port w/ protocol #349
Conversation
commands/start-container.ts
Outdated
| @@ -29,7 +29,7 @@ export async function startContainer(context?: ImageNode, interactive?: boolean) | |||
| docker.getExposedPorts(imageToStart.Id).then((ports: string[]) => { | |||
| let options = `--rm ${interactive ? '-it' : '-d'}`; | |||
| if (ports.length) { | |||
| const portMappings = ports.map((port) => `-p ${port}:${port}`); | |||
| const portMappings = ports.map((port) => `-p ${port.split("/")[0]}:${port}`); | |||
There was a problem hiding this comment.
Please comment with what you're expecting and the output you're going for.
There was a problem hiding this comment.
current output:
-p 8080:8080
output I'm going for:
-p 8080:8080/tcp , -p 3010:3010/tcp
Notice the difference in 8080/tcp. Earlier, we stripped the protocol when returning the function (see getExposedPorts).
The earlier command was:
docker run --rm -d -p 8080:8080 node-todo:latest
Now with the change:
docker run --rm -d -p 8080:8080/tcp node-todo:latest
There was a problem hiding this comment.
I wanted code comments. If you need to explain it to me, you need to explain it to code maintainers.
There was a problem hiding this comment.
Ah, my bad. Added comment
commands/start-container.ts
Outdated
| @@ -29,7 +29,7 @@ export async function startContainer(context?: ImageNode, interactive?: boolean) | |||
| docker.getExposedPorts(imageToStart.Id).then((ports: string[]) => { | |||
| let options = `--rm ${interactive ? '-it' : '-d'}`; | |||
| if (ports.length) { | |||
| const portMappings = ports.map((port) => `-p ${port}:${port}`); | |||
| const portMappings = ports.map((port) => `-p ${port.split("/")[0]}:${port}`); | |||
There was a problem hiding this comment.
Are there are testcases?
Fixes #284