expose service to outside, fix #140 #285

concaf · 2016-11-16T05:51:18Z

No description provided.

cdrage · 2016-11-16T14:15:10Z

i think this needs rebasing, you've got some README.md changes in here.

concaf · 2016-11-17T09:06:20Z

@cdrage yeah, it's a WIP right now, things are going to get worse before it gets any better :)

surajssd · 2016-11-17T14:00:15Z

pkg/transformer/openshift/openshift.go

@@ -160,6 +184,10 @@ func (o *OpenShift) Transform(komposeObject kobject.KomposeObject, opt kobject.C
 			objects = append(objects, svc)
 		}

+		if service.ExposeService != "" {
+			objects = append(objects, o.initRoute(name, service))
+		}


Also we can expose a service only if we can create a service in first place. So this if block should be inside if o.PortsExist(name, service).

rtnpro · 2016-11-28T17:26:15Z

@containscafeine I tested the example you have created and it seems to work correctly. I also had a look at the code. It looks good, overall.

coveralls · 2016-12-07T19:37:36Z

Coverage decreased (-0.1%) to 34.872% when pulling f140667 on containscafeine:expose_service into 862419b on kubernetes-incubator:master.

coveralls · 2016-12-08T15:54:48Z

Coverage decreased (-2.0%) to 33.024% when pulling 9a52fa6 on containscafeine:expose_service into 862419b on kubernetes-incubator:master.

coveralls · 2016-12-08T17:36:28Z

Coverage decreased (-2.0%) to 33.024% when pulling 1a6946a on containscafeine:expose_service into 04a3131 on kubernetes-incubator:master.

coveralls · 2016-12-08T20:08:04Z

Coverage decreased (-1.4%) to 33.617% when pulling 179c1af on containscafeine:expose_service into 04a3131 on kubernetes-incubator:master.

coveralls · 2016-12-09T14:23:15Z

Coverage decreased (-1.4%) to 33.617% when pulling 4c22466 on containscafeine:expose_service into 04a3131 on kubernetes-incubator:master.

coveralls · 2016-12-12T18:48:56Z

Coverage decreased (-2.2%) to 32.707% when pulling 87f8884 on containscafeine:expose_service into 65e19e3 on kubernetes-incubator:master.

coveralls · 2016-12-14T11:52:42Z

Coverage increased (+3.6%) to 38.576% when pulling d40fd99 on containscafeine:expose_service into 65e19e3 on kubernetes-incubator:master.

coveralls · 2016-12-14T12:06:53Z

Coverage increased (+3.6%) to 38.576% when pulling 35302ef on containscafeine:expose_service into 65e19e3 on kubernetes-incubator:master.

surajssd · 2016-12-14T19:39:39Z

pkg/transformer/openshift/openshift.go

+		},
+	}
+
+	//if exposeSlice := strings.Split(service.ExposeService, ":"); len(exposeSlice) == 2 {


are you planning to remove this?

Yes, and will add some comments in the code in the next commit.

surajssd · 2016-12-14T19:41:46Z

pkg/transformer/kubernetes/kubernetes.go

+		},
+	}
+
+	if service.ExposeService != "true" {


is there a way that we enforce user to provide either "true" or a valid url?

i am asking because what if user provides value as false

In that case, false gets treated as a hostname. If the user does not want to expose, he would rather not write (or comment) this in the compose file.

Even I am divided on this. Should we enforce, (true, false, hostname), or just (true, hostname)?

So went ahead with (true, hostname)

surajssd · 2016-12-15T06:05:41Z

@containscafeine please send docs with this, we don't want user to miss out on this cool feature due to lack of docs

coveralls · 2016-12-20T12:52:07Z

Coverage increased (+2.7%) to 41.155% when pulling 190cf79 on containscafeine:expose_service into c485a87 on kubernetes-incubator:master.

coveralls · 2016-12-20T12:53:56Z

Coverage increased (+2.7%) to 41.155% when pulling 190cf79 on containscafeine:expose_service into c485a87 on kubernetes-incubator:master.

coveralls · 2016-12-20T13:40:11Z

Coverage increased (+1.7%) to 40.157% when pulling 0c1fd72 on containscafeine:expose_service into c485a87 on kubernetes-incubator:master.

cdrage · 2016-12-20T13:49:11Z

@containscafeine tests seem to fail since glide-vc --only-code --no-tests needs to be ran again in the directory for vendoring. missing one odd file.

coveralls · 2016-12-20T14:24:16Z

Coverage increased (+1.7%) to 40.157% when pulling 861f016 on containscafeine:expose_service into c485a87 on kubernetes-incubator:master.

cdrage · 2016-12-20T15:29:55Z

commits need to be squashed and then LGTM from me :)

coveralls · 2016-12-21T10:02:25Z

Coverage increased (+1.7%) to 40.157% when pulling 667f359 on containscafeine:expose_service into c485a87 on kubernetes-incubator:master.

coveralls · 2016-12-21T11:12:46Z

Coverage increased (+1.7%) to 40.157% when pulling d14802f on containscafeine:expose_service into c485a87 on kubernetes-incubator:master.

coveralls · 2016-12-21T11:17:04Z

Coverage increased (+1.7%) to 40.157% when pulling d14802f on containscafeine:expose_service into c485a87 on kubernetes-incubator:master.

concaf · 2016-12-21T11:28:25Z

@cdrage @surajssd - added docs, squashed the commits, refactored the ingress unit test since last review. Should be good to go now.

coveralls · 2016-12-21T11:34:31Z

Coverage increased (+1.7%) to 40.157% when pulling 9654f28 on containscafeine:expose_service into c485a87 on kubernetes-incubator:master.

cdrage · 2016-12-21T13:30:35Z

@containscafeine just conflicting right now :)

Implements a kompose specific docker compose label "kompose.service.expose" which can be used to expose the specified services externally. The accepted values are of type string. If the value is set to "true", the provider sets the endpoint automatically, and for any other value, the value is set as the hostname. If multiple ports are defined in a service, the first one is chosen to be the exposed. Unit tests, functional tests, glide updates and docs have also been added in this commit for the related feature.

coveralls · 2016-12-21T14:39:03Z

Coverage increased (+1.2%) to 42.083% when pulling 7e378cd on containscafeine:expose_service into 48e3ba8 on kubernetes-incubator:master.

concaf · 2016-12-21T15:32:02Z

@cdrage fixed

surajssd · 2016-12-22T11:42:54Z

LGTM!

rtnpro force-pushed the expose_service branch from 18abca6 to 18b8e68 Compare November 16, 2016 08:11

surajssd added the rebase needed label Nov 16, 2016

concaf force-pushed the expose_service branch 2 times, most recently from ccbf4ef to 75e851f Compare November 17, 2016 09:26

surajssd reviewed Nov 17, 2016

View reviewed changes

surajssd removed the rebase needed label Nov 17, 2016

concaf force-pushed the expose_service branch from 75e851f to 7cb5f97 Compare November 22, 2016 09:29

concaf force-pushed the expose_service branch from 7cb5f97 to f140667 Compare December 7, 2016 19:06

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 7, 2016

concaf force-pushed the expose_service branch from 9a52fa6 to 1a6946a Compare December 8, 2016 16:55

concaf force-pushed the expose_service branch from 4c22466 to 87f8884 Compare December 12, 2016 18:41

concaf changed the title ~~[WIP] expose service to outside, fix #140~~ expose service to outside, fix #140 Dec 14, 2016

surajssd mentioned this pull request Dec 14, 2016

RFE: choosing Service type #154

Closed

surajssd added the review needed label Dec 14, 2016

surajssd reviewed Dec 14, 2016

View reviewed changes

surajssd added the rebase needed label Dec 19, 2016

concaf force-pushed the expose_service branch from 35302ef to 2a6effe Compare December 20, 2016 12:44

concaf force-pushed the expose_service branch from 2a6effe to 190cf79 Compare December 20, 2016 12:46

concaf force-pushed the expose_service branch from 9a4fbee to d14802f Compare December 21, 2016 11:09

concaf force-pushed the expose_service branch from d14802f to 9654f28 Compare December 21, 2016 11:27

concaf force-pushed the expose_service branch from 9654f28 to 7e378cd Compare December 21, 2016 14:31

surajssd removed the rebase needed label Dec 22, 2016

surajssd approved these changes Dec 22, 2016

View reviewed changes

surajssd merged commit 240f150 into kubernetes:master Dec 22, 2016

expose service to outside, fix #140 #285

expose service to outside, fix #140 #285

Conversation

concaf commented Nov 16, 2016

cdrage commented Nov 16, 2016

concaf commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtnpro commented Nov 28, 2016

coveralls commented Dec 7, 2016

coveralls commented Dec 8, 2016

coveralls commented Dec 8, 2016

coveralls commented Dec 8, 2016

coveralls commented Dec 9, 2016

coveralls commented Dec 12, 2016

coveralls commented Dec 14, 2016

coveralls commented Dec 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajssd commented Dec 15, 2016

coveralls commented Dec 20, 2016

coveralls commented Dec 20, 2016

coveralls commented Dec 20, 2016

cdrage commented Dec 20, 2016

coveralls commented Dec 20, 2016

cdrage commented Dec 20, 2016

coveralls commented Dec 21, 2016

coveralls commented Dec 21, 2016

coveralls commented Dec 21, 2016

concaf commented Dec 21, 2016

coveralls commented Dec 21, 2016

cdrage commented Dec 21, 2016

coveralls commented Dec 21, 2016

concaf commented Dec 21, 2016

surajssd commented Dec 22, 2016