Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Jan 20, 2015
Merged

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

merged 1 commit into from
Jan 20, 2015

Conversation

brahmaroutu
Copy link
Contributor

... for port ranges, regression from #1834

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

@jessfraz
Copy link
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

LK4D4
LK4D4 reviewed Jan 13, 2015
View reviewed changes
links/links.go
}

}
for i := 0; i < len(l.Ports); {
Copy link
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
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
Contributor

LGTM

@crosbymichael
Copy link
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
Contributor

LK4D4 commented Jan 14, 2015

Yup, +1 to tests.

@brahmaroutu
Copy link
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

@brahmaroutu
Env Variables created for each of the ports in addition to env variab…
611a23a 
…les for port ranges, regression from moby#1834

Closes moby#9900
Signed-off-by: Srini Brahmaroutu <srbrahma@us.ibm.com>
@brahmaroutu
Copy link
Contributor Author

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

@icecrime
Copy link
Contributor

LGTM

@icecrime
Copy link
Contributor

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

@crosbymichael
Copy link
Contributor

Thanks for the tests!

crosbymichael added a commit that referenced this pull request Jan 20, 2015
@crosbymichael
Merge pull request #10061 from brahmaroutu/env_ports_9900
f49fb25 
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
@SvenDowideit SvenDowideit mentioned this pull request Jan 27, 2015
Merged
@thaJeztah thaJeztah mentioned this pull request Nov 25, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.

Environment variables for published ports changed when using --link
6 participants
@brahmaroutu @jessfraz @crosbymichael @LK4D4 @icecrime @tiborvass