Skip to content

Env Variables created for each of the ports in addition to env variables...#10061

Merged
crosbymichael merged 1 commit intomoby:masterfrom
brahmaroutu:env_ports_9900
Jan 20, 2015
Merged

Env Variables created for each of the ports in addition to env variables...#10061
crosbymichael merged 1 commit intomoby:masterfrom
brahmaroutu:env_ports_9900

Conversation

@brahmaroutu
Copy link
Copy Markdown
Contributor

... for port ranges, regression from #1834

Closes #9900
Signed-off-by: Srini Brahmaroutu [email protected]

@jessfraz
Copy link
Copy Markdown
Contributor

Can you please sign your commits following these rules:

https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work

The easiest way to do this is to amend the last commit:

git commit --amend -s --no-edit && git push -f

Comment thread links/links.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just for _, p := range l.Ports?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes no reason now, just a copy of old code. I changes it now.

@tiborvass tiborvass added this to the 1.5.0 milestone Jan 14, 2015
@crosbymichael
Copy link
Copy Markdown
Contributor

LGTM

@crosbymichael
Copy link
Copy Markdown
Contributor

We should probably add a few integration tests to enforce the environment variables for a container or it maybe pretty easy to write a unit test for this method.

@LK4D4
Copy link
Copy Markdown
Contributor

LK4D4 commented Jan 14, 2015

Yup, +1 to tests.

@brahmaroutu
Copy link
Copy Markdown
Contributor Author

@crosbymichael @LK4D4 Thanks for the review. I will add tests shortly. One scenario that I am not very convinced about is that env for range may show differently, for example
docker run --name backend -p 7101:7101 -p 7102:7102 -p 7103-7105:7103-7105
<we get normal ENV variables for all 5 ports with this fix but the range env variables will look like below since we detect the continuity in allocation>
BACKEND_PORT_7101_TCP_PORT_END=7105
BACKEND_PORT_7101_TCP_PORT_START=7101

…les for port ranges, regression from moby#1834

Closes moby#9900
Signed-off-by: Srini Brahmaroutu <[email protected]>
@brahmaroutu
Copy link
Copy Markdown
Contributor Author

Please see that I added a unit test for check all env variables.

@icecrime
Copy link
Copy Markdown
Contributor

LGTM

@icecrime
Copy link
Copy Markdown
Contributor

@LK4D4 Anything else to add to this or can we merge?

@crosbymichael
Copy link
Copy Markdown
Contributor

Thanks for the tests!

crosbymichael added a commit that referenced this pull request Jan 20, 2015
Env Variables created for each of the ports in addition to env variables...
@crosbymichael crosbymichael merged commit f49fb25 into moby:master Jan 20, 2015
@brahmaroutu brahmaroutu deleted the env_ports_9900 branch January 23, 2015 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Environment variables for published ports changed when using --link

6 participants