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.

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

Add support for host:port:port #393

Merged
merged 1 commit into from
Feb 10, 2017

Conversation

cdrage
Copy link
Member

@cdrage cdrage commented Jan 25, 2017

This adds support for supplying for example:
"127.0.0.1:80:80/tcp" to docker-compose.yaml and converting it to it's
corresponding Kubernetes / OpenShift hostIP.

This commit also refactors the loadPorts function of compose.go

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 25, 2017
@cdrage cdrage force-pushed the host-container-protocol-bug branch 3 times, most recently from 576e2b3 to 750ce20 Compare January 25, 2017 18:12
@cdrage
Copy link
Member Author

cdrage commented Jan 30, 2017

1 test doesn't pass, going to fix that :)

@cdrage cdrage force-pushed the host-container-protocol-bug branch 2 times, most recently from 4a4cdd7 to 06c848d Compare January 30, 2017 14:54
@cdrage
Copy link
Member Author

cdrage commented Jan 30, 2017

There was a typo with a test parameter, all good now 👍

Ready for review! @surajssd @procrypt @kadel @ngtuna

@cdrage
Copy link
Member Author

cdrage commented Feb 2, 2017

Ping @surajssd @containscafeine @procrypt @kadel @rtnpro for review!

@kadel
Copy link
Member

kadel commented Feb 5, 2017

Can this be covered by unit tests?

otherwise LGTM

@procrypt
Copy link

procrypt commented Feb 6, 2017

@cdrage LGTM

@procrypt
Copy link

procrypt commented Feb 6, 2017

@kadel @cdrage Can I write the unit tests for this.

@cdrage
Copy link
Member Author

cdrage commented Feb 6, 2017

@procrypt Sure why not! :)

@cdrage cdrage force-pushed the host-container-protocol-bug branch from 06c848d to 6bcc49d Compare February 6, 2017 20:11
@cdrage
Copy link
Member Author

cdrage commented Feb 6, 2017

Rebased ^^

@surajssd
Copy link
Member

surajssd commented Feb 9, 2017

@cdrage fix in tests and docs with all the supported ports type will help! code LGTM 👍

@cdrage cdrage force-pushed the host-container-protocol-bug branch 2 times, most recently from 30b2532 to 89548ea Compare February 9, 2017 13:54
@cdrage
Copy link
Member Author

cdrage commented Feb 9, 2017

@surajssd @kadel Tests should pass this time (was related to my TCP PR being merged)

Regards for docs, like my other PR, I'm going to push a 1-1 document regarding what we convert between docker-compose => kubernetes/openshift all-in-one-go. Rather than including a tiny doc in here :)

@cdrage
Copy link
Member Author

cdrage commented Feb 9, 2017

All green 🎉

@kadel
Copy link
Member

kadel commented Feb 9, 2017

I meant if you can add go unit tests, instead of shell test to this pr 👼

I think is is better to cover as much as we can using proper go tests

@cdrage
Copy link
Member Author

cdrage commented Feb 9, 2017

@kadel Np, I'll update that :)

This adds support for supplying for example:
"127.0.0.1:80:80/tcp" to docker-compose.yaml and converting it to it's
corresponding Kubernetes / OpenShift hostIP.

This commit also refactors the loadPorts function of compose.go

Closes kubernetes#335
@cdrage cdrage force-pushed the host-container-protocol-bug branch from 89548ea to 438088f Compare February 9, 2017 17:21
@cdrage
Copy link
Member Author

cdrage commented Feb 9, 2017

@kadel Your wish is granted! Unit tests added.

@kadel
Copy link
Member

kadel commented Feb 10, 2017

Sweet 👍 thank you.

"Coverage increased (+2.3%) to 51.987%" 🎉 🥇

@kadel kadel merged commit 1712634 into kubernetes:master Feb 10, 2017
@cdrage cdrage deleted the host-container-protocol-bug branch February 10, 2017 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants