adding support for port ranges on --expose by brahmaroutu · Pull Request #8167 · moby/moby · GitHub

Addresses #1834

Signed-off-by: Srini Brahmaroutu srbrahma@us.ibm.com

LGTM, thanks for the patch!

wouldn't it make sense to add this to -p as well, or what would be the use case for exposing the range without publishing?

@jfrazelle I thought about it but I only wanted to add code that originally intended by @jhorey. My thinking is the feature can be used with -P (which I tested) and certainly a new issue should be opened to add support for -p.

gotcha

LGTM

This string is set from the flag code in runconfig/parse.go (and is updated using a script from the output of docker)

so needs to be set there. Can you also mention in that text the port-port format - I'm not sure we've used a - separator anywhere else, so a non-read the docs reminder would be good.

@SvenDowideit I have added the port range to the flag description. I went with a example for port range and hope that works.

Maybe instead of else do

for i := 0; 1 < len(l.Ports); {
    p := l.Ports[i]
    j := nextContiguous(l.Ports, p.Int(), i)
    if j > i+1 {
        env = append(env, fmt.Sprintf("%s_PORT_%s_%s_START=%s://%s:%s", alias, p.Port(), strings.ToUpper(p.Proto()), p.Proto(), l.ChildIP, p.Port()))
        env = append(env, fmt.Sprintf("%s_PORT_%s_%s_ADDR=%s", alias, p.Port(), strings.ToUpper(p.Proto()), l.ChildIP))
        env = append(env, fmt.Sprintf("%s_PORT_%s_%s_PROTO=%s", alias, p.Port(), strings.ToUpper(p.Proto()), p.Proto()))
        env = append(env, fmt.Sprintf("%s_PORT_%s_%s_PORT_START=%s", alias, p.Port(), strings.ToUpper(p.Proto()), p.Port()))

        q := l.Ports[j]
        env = append(env, fmt.Sprintf("%s_PORT_%s_%s_END=%s://%s:%s", alias, p.Port(), strings.ToUpper(q.Proto()), q.Proto(), l.ChildIP, q.Port()))
        env = append(env, fmt.Sprintf("%s_PORT_%s_%s_PORT_END=%s", alias, p.Port(), strings.ToUpper(q.Proto()), q.Port()))

        i = j + 1
        continue
    }

    env = append(env, fmt.Sprintf("%s_PORT_%s_%s=%s://%s:%s", alias, p.Port(), strings.ToUpper(p.Proto()), p.Proto(), l.ChildIP, p.Port()))
    env = append(env, fmt.Sprintf("%s_PORT_%s_%s_ADDR=%s", alias, p.Port(), strings.ToUpper(p.Proto()), l.ChildIP))
    env = append(env, fmt.Sprintf("%s_PORT_%s_%s_PORT=%s", alias, p.Port(), strings.ToUpper(p.Proto()), p.Port()))
    env = append(env, fmt.Sprintf("%s_PORT_%s_%s_PROTO=%s", alias, p.Port(), strings.ToUpper(p.Proto()), p.Proto()))
    i++
}

Just my personal preference.

Changed code as above, it is lot cleaner.

The commandline flags documented in cli.md and docker-run.man1.md should be identical to that in the code.

and can you squash your commits when you rebase?

@SvenDowideit I squashed my commits and Please see the change to --expose flag, is that what you refer to?

This is the text that should be used in docker-run.1.md, cli.md and run.md - as the text written should look identical to what the user sees when they type docker run ---help.

I have a script that will update it for you (though you need to ignore some of its changes) - docs/docs-update.py ./bundles/1.2.0-dev/binary/docker - this will use your modified docker binary to update the documentation. :)

thanks @SvenDowideit, that is a cool tool. I did selectively updated the docs using the tool.

this needs a rebase and I think we are just waiting on docs approval?

Thanks, I just did a rebase

Please let me know if there is anything missing? I am still waiting for this merge. @SvenDowideit are the docs look good?

Docs LGTM - @jamtur01 @fredlf @ostezer

Docs LGTM

@jfrazelle @crosbymichael , Docs are happy :)

Rebased and I hope this can be merged soon.

@brahmaroutu this needs a rebase and then it seems like we should be good to merge sorry for the delay

also so GH automatically closes the issue can you change the "Addresses" in your first comment to Closes

Closes moby#1834 Signed-off-by: Srini Brahmaroutu <srbrahma@us.ibm.com>

I addressed both the comments.

adding support for port ranges on --expose

It doesn't look like there is corresponding support for publishing a range of ports, just for exposing them. Is there an open issue for that? I couldn't find one.

In most cases, people are doing to want to do a verbatim mapping instead of doing --publish-all, since these port ranges are often used in cases where the server has a well-known port (or multiple) and hands out port assignments as part of a protocol.

Yes I am aware of that, it was commented earlier about this issue. I can work on that as this is merged.

That's great. If there's an open issue, I'd like to subscribe to it. I can create one otherwise.

Thanks!

Please open an issue.

Is it possible to also do this from the Dockerfile?

EXPOSE 3300-3310

@dkirrane yes, you can do EXPOSE 3300-3310 from Dockerfile

brahmaroutu force-pushed the expose_ports_1834 branch 2 times, most recently from 687a342 to 652bd7b Compare September 22, 2014 17:53

jessfraz added the /project/doc label Sep 22, 2014

SvenDowideit reviewed Sep 22, 2014
View reviewed changes

brahmaroutu force-pushed the expose_ports_1834 branch from 652bd7b to c428c70 Compare September 23, 2014 05:35

cpuguy83 reviewed Sep 23, 2014
View reviewed changes

brahmaroutu force-pushed the expose_ports_1834 branch 2 times, most recently from b9b4f9b to 5f03176 Compare September 24, 2014 15:55

brahmaroutu force-pushed the expose_ports_1834 branch from 5f03176 to da287e4 Compare September 25, 2014 16:22

SvenDowideit reviewed Sep 26, 2014
View reviewed changes

brahmaroutu force-pushed the expose_ports_1834 branch from da287e4 to 0fc75d5 Compare September 26, 2014 16:10

brahmaroutu force-pushed the expose_ports_1834 branch from 0fc75d5 to d59a854 Compare September 29, 2014 19:01

brahmaroutu force-pushed the expose_ports_1834 branch from d59a854 to 822b10d Compare October 1, 2014 22:55

brahmaroutu force-pushed the expose_ports_1834 branch from 822b10d to e846ae2 Compare October 6, 2014 20:20

brahmaroutu force-pushed the expose_ports_1834 branch from e846ae2 to 989d7ca Compare October 7, 2014 17:24

brahmaroutu force-pushed the expose_ports_1834 branch 2 times, most recently from 164cdc8 to 673446f Compare October 14, 2014 18:57

brahmaroutu force-pushed the expose_ports_1834 branch 2 times, most recently from 7e35933 to ab97622 Compare October 20, 2014 18:04

brahmaroutu force-pushed the expose_ports_1834 branch from ab97622 to a41c5b6 Compare October 31, 2014 23:06

adding support for port ranges on --expose

fd774a8

Closes moby#1834 Signed-off-by: Srini Brahmaroutu <srbrahma@us.ibm.com>

brahmaroutu force-pushed the expose_ports_1834 branch from a41c5b6 to fd774a8 Compare October 31, 2014 23:07

jessfraz pushed a commit that referenced this pull request Oct 31, 2014

Merge pull request #8167 from brahmaroutu/expose_ports_1834

92bb497

adding support for port ranges on --expose

jessfraz merged commit 92bb497 into moby:master Oct 31, 2014

jessfraz mentioned this pull request Oct 31, 2014

Allow specifying a range of ports to EXPOSE #1834

Closed

md5 mentioned this pull request Nov 1, 2014

Add support for publishing ranges of ports #8899

Closed

fredlf mentioned this pull request Nov 5, 2014

Learning to use Pulls tool, just ignore #8967

Closed

SvenDowideit mentioned this pull request Nov 6, 2014

[DOC] post-1.3.1-docs-update-1 #8991

Closed

SvenDowideit mentioned this pull request Jan 6, 2015

Environment variables for published ports changed when using --link #9900

Closed

brahmaroutu deleted the expose_ports_1834 branch January 23, 2015 22:47

adding support for port ranges on --expose by brahmaroutu · Pull Request #8167 · moby/moby · GitHub

Uh oh!

adding support for port ranges on --expose - #8167

adding support for port ranges on --expose#8167jessfraz merged 1 commit intomoby:mastermoby/moby:masterfrom brahmaroutu:expose_ports_1834brahmaroutu/docker:expose_ports_1834Copy head branch name to clipboard

Conversation

brahmaroutu commented Sep 22, 2014

Uh oh!

jessfraz commented Sep 22, 2014

Uh oh!

jessfraz commented Sep 22, 2014

Uh oh!

brahmaroutu commented Sep 22, 2014

Uh oh!

jessfraz commented Sep 22, 2014

Uh oh!

vieux commented Sep 22, 2014

Uh oh!

SvenDowideit Sep 22, 2014

Choose a reason for hiding this comment

Uh oh!

brahmaroutu commented Sep 23, 2014

Uh oh!

cpuguy83 Sep 23, 2014

Choose a reason for hiding this comment

Uh oh!

brahmaroutu Sep 23, 2014

Choose a reason for hiding this comment

Uh oh!

SvenDowideit commented Sep 25, 2014

Uh oh!

brahmaroutu commented Sep 25, 2014

Uh oh!

SvenDowideit Sep 26, 2014

Choose a reason for hiding this comment

Uh oh!

brahmaroutu commented Sep 26, 2014

Uh oh!

jessfraz commented Sep 29, 2014

Uh oh!

brahmaroutu commented Sep 29, 2014

Uh oh!

brahmaroutu commented Oct 2, 2014

Uh oh!

SvenDowideit commented Oct 7, 2014

Uh oh!

jamtur01 commented Oct 7, 2014

Uh oh!

ghost commented Oct 7, 2014

Uh oh!

SvenDowideit commented Oct 8, 2014

Uh oh!

brahmaroutu commented Oct 14, 2014

Uh oh!

jessfraz commented Oct 31, 2014

Uh oh!

jessfraz commented Oct 31, 2014

Uh oh!

brahmaroutu commented Oct 31, 2014

Uh oh!

md5 commented Oct 31, 2014

Uh oh!

brahmaroutu commented Oct 31, 2014

Uh oh!

md5 commented Oct 31, 2014

Uh oh!

brahmaroutu commented Nov 1, 2014

Uh oh!

dkirrane commented Feb 25, 2015

Uh oh!

coolljt0725 commented Feb 25, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

adding support for port ranges on --expose#8167
jessfraz merged 1 commit into
moby:masterfrom
brahmaroutu:expose_ports_1834