Failing when port is specified with labels #590

surajnarwade · 2017-05-08T08:18:16Z

Resolves #522
Kompose will give FATAL error if labels are given but ports are not defined

cdrage · 2017-05-08T11:09:01Z

pkg/loader/compose/compose.go

@@ -358,6 +358,12 @@ func (c *Compose) LoadFile(files []string) (kobject.KomposeObject, error) {
 			}
 		}

+		if len(serviceConfig.Port) == 0 {
+			if composeServiceConfig.Labels["kompose.service.type"] == "NodePort" || composeServiceConfig.Labels["kompose.service.type"] == "LoadBalancer" {


Why are there nested if's?

Shouldn't it be: if len(serviceConfig.Port) == 0 && composeServiceConfig.Labels["kompose.service.type"] == "NodePort" || composeServiceConfig.Labels["kompose.service.type"] == "LoadBalancer"

cdrage · 2017-05-08T12:21:29Z

pkg/loader/compose/compose.go

@@ -358,6 +358,11 @@ func (c *Compose) LoadFile(files []string) (kobject.KomposeObject, error) {
 			}
 		}

+		if len(serviceConfig.Port) == 0 && composeServiceConfig.Labels["kompose.service.type"] == "NodePort" || composeServiceConfig.Labels["kompose.service.type"] == "LoadBalancer" {
+			log.Fatal("Failed to proceed as Nodeport or loadbalancer is defined without ports")


Was just thinking again (sorry for asking for another change) that this could be better:

log.Fatal("%s defined with no ports present. Issues may occur when bringing up artifacts.", composeServiceConfig.Labels["kompose.service.type"])

And also change this log to warning instead? What do you think @kadel ?

log message is okay but giving warning instead of FATAL is of no use

@surajnarwade I think it should still convert.. Let's wait for @kadel 's opinion 👍

I think @kadel made it clear that we should fail here #522 (comment)

yes, that makes sense, we should fail, as it doesn't make sense to use NodePort or LoadBalancer without port defined.

Please update the messge though!

surajssd · 2017-05-09T11:44:32Z

pkg/loader/compose/compose.go

@@ -358,6 +358,11 @@ func (c *Compose) LoadFile(files []string) (kobject.KomposeObject, error) {
 			}
 		}

+		if len(serviceConfig.Port) == 0 && composeServiceConfig.Labels["kompose.service.type"] == "NodePort" || composeServiceConfig.Labels["kompose.service.type"] == "LoadBalancer" {
+			log.Fatal("Failed to proceed as Nodeport or loadbalancer is defined without ports")
+


nit: remove blank line

surajnarwade · 2017-05-15T09:14:47Z

cc @kadel @cdrage

surajssd · 2017-05-15T09:17:53Z

@surajnarwade tests and docs update

surajssd · 2017-05-15T09:20:59Z

pkg/loader/compose/compose.go

@@ -358,6 +358,10 @@ func (c *Compose) LoadFile(files []string) (kobject.KomposeObject, error) {
 			}
 		}

+		if len(serviceConfig.Port) == 0 && composeServiceConfig.Labels["kompose.service.type"] == "NodePort" || composeServiceConfig.Labels["kompose.service.type"] == "LoadBalancer" {
+			log.Fatalf("%s defined with no ports present. Issues may occur when bringing up artifacts.", composeServiceConfig.Labels["kompose.service.type"])


Also this one does not tell which docker-compose service has that label and no port defined together?

cdrage · 2017-05-15T14:59:35Z

pkg/loader/compose/compose.go

@@ -358,6 +358,10 @@ func (c *Compose) LoadFile(files []string) (kobject.KomposeObject, error) {
 			}
 		}

+		if len(serviceConfig.Port) == 0 && composeServiceConfig.Labels["kompose.service.type"] == "NodePort" || composeServiceConfig.Labels["kompose.service.type"] == "LoadBalancer" {
+			log.Fatalf("In %s service, %s defined with no ports present. Issues may occur when bringing up artifacts.", name, composeServiceConfig.Labels["kompose.service.type"])


In %s service isn't grammatically correct.

My previous comment may work a bit better:

log.Fatal("%s defined in service %s with no ports present. Issues may occur when bringing up artifacts.", composeServiceConfig.Labels["kompose.service.type"], name)

surajnarwade · 2017-05-16T08:37:29Z

@cdrage @surajssd needs review

cdrage · 2017-05-16T17:28:11Z

LGTM 👍

surajssd · 2017-05-17T07:01:56Z

@surajnarwade tests and docs update

gitlawr · 2017-05-17T08:39:00Z

Would it be better if it return error here and leave the Fatalf() work to upper layer?
I'm investigating #464 and I think logrus.Fatalf(...) may be a broker because it finally invokes os.Exit()
Anyway, returning an error consists with other parts of the codes:
https://github.com/surajnarwade/kompose/blob/8198246bd8363642800941be75a2b9bf414ee9b5/pkg/loader/compose/compose.go#L352

surajnarwade · 2017-05-17T09:20:57Z

@gitlawr , return error here, I didn't get you

gitlawr · 2017-05-17T09:42:41Z

@surajnarwade
I mean

return kobject.KomposeObject{}, errors.New("xxx defined in service xxx with no ports present. Issues may occur when bringing up artifacts.")

instead of

log.Fatalf(...)

concaf · 2017-05-17T13:58:24Z

pkg/loader/compose/compose.go

@@ -359,6 +359,10 @@ func (c *Compose) LoadFile(files []string) (kobject.KomposeObject, error) {
 			}
 		}

+		err = checkLabelsPorts(len(serviceConfig.Port), composeServiceConfig.Labels["kompose.service.type"], name)
+		if err != nil {
+			log.Fatal(err)


Let's not use log.Fatal, instead wrap the error and pass it up the call stack. Use errors.Wrap

what @containscafeine said ;-)

concaf · 2017-05-17T13:59:34Z

pkg/loader/compose/compose.go

+
+func checkLabelsPorts(noOfPort int, labels string, svcName string) error {
+	if noOfPort == 0 && labels == "NodePort" || labels == "LoadBalancer" {
+		return errors.Errorf("%s defined in service %s with no ports present. Issues may occur when bringing up artifacts.", labels, svcName)


Let's wrap it using errors.Wrap to maintain consistency throughout the code.

@surajnarwade if there is nothing to wrap, use errors.New()

@containscafeine , I have variable in my error message. so I used errors.Errorf() instead of errors.New()

Let's change message a bit. It is not possible to create Headless service with type NodePort or LoadBalancer.
The Message should be more definite.
For example something in those lines:
"kompose.service.type can't be set if service doesn't expose any ports."

added message

surajnarwade · 2017-05-29T07:58:16Z

@surajssd , needs review on this

surajssd · 2017-05-29T10:01:51Z

@surajnarwade bear with me, also please add functional tests! so when I meant tests == (unit tests and functional tests)

cdrage · 2017-05-29T14:02:10Z

pkg/loader/compose/compose.go

@@ -363,6 +363,10 @@ func (c *Compose) LoadFile(files []string) (kobject.KomposeObject, error) {
 			}
 		}

+		err1 := checkLabelsPorts(len(serviceConfig.Port), composeServiceConfig.Labels["kompose.service.type"], name)


err1? that's not a good name for a variable. Please use err.

cdrage · 2017-05-29T14:03:12Z

pkg/loader/compose/compose.go

@@ -363,6 +363,10 @@ func (c *Compose) LoadFile(files []string) (kobject.KomposeObject, error) {
 			}
 		}

+		err1 := checkLabelsPorts(len(serviceConfig.Port), composeServiceConfig.Labels["kompose.service.type"], name)
+		if err1 != nil {
+			return kobject.KomposeObject{}, errors.Wrap(err, err1.Error())


what's going on here? for errors.Wrap it should be `errors.Wrap(err, "Your error message here") or something along the lines.

surajnarwade · 2017-05-30T13:04:08Z

@cdrage , @surajssd I have updated PR with changes and added tests as well

kadel · 2017-06-02T09:28:37Z

pkg/loader/compose/compose.go

@@ -359,6 +359,10 @@ func (c *Compose) LoadFile(files []string) (kobject.KomposeObject, error) {
 			}
 		}

+		err = checkLabelsPorts(len(serviceConfig.Port), composeServiceConfig.Labels["kompose.service.type"], name)
+		if err != nil {
+			log.Fatal(err)


what @containscafeine said ;-)

kadel · 2017-06-02T10:34:46Z

pkg/loader/compose/compose.go

+
+func checkLabelsPorts(noOfPort int, labels string, svcName string) error {
+	if noOfPort == 0 && labels == "NodePort" || labels == "LoadBalancer" {
+		return errors.Errorf("%s defined in service %s with no ports present. Issues may occur when bringing up artifacts.", labels, svcName)


Let's change message a bit. It is not possible to create Headless service with type NodePort or LoadBalancer.
The Message should be more definite.
For example something in those lines:
"kompose.service.type can't be set if service doesn't expose any ports."

surajnarwade · 2017-06-06T06:15:40Z

@kadel @surajssd @containscafeine can you please test now

cdrage · 2017-06-13T14:01:38Z

docs/user-guide.md

+
+#### Warning about Labels
+
+`kompose.service.type` label should be defined with `ports` only, otherwise `kompose` will fail.


This should be added under the labels section, not at the end of the user-guide file, otherwise, PR LGTM.

surajnarwade · 2017-06-14T06:25:17Z

@cdrage review please :)

surajssd

LGTM!

cdrage · 2017-06-14T14:11:19Z

docs/user-guide.md

@@ -401,3 +402,5 @@ If the Docker Compose file has a volume specified for a service, the Deployment
 If the Docker Compose file has service name with `_` in it (eg.`web_service`), then it will be replaced by `-` and the service name will be renamed accordingly (eg.`web-service`). Kompose does this because "Kubernetes" doesn't allow `_` in object name.

 Please note that changing service name might break some `docker-compose` files.
+
+


Remove extra newlines, update PR, then go-ahead and merge!

Resolves kubernetes#522 Kompose will give FATAL error if labels are given but ports are not defined

surajnarwade · 2017-06-15T09:02:00Z

All green now, @cdrage @surajssd

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 8, 2017

cdrage reviewed May 8, 2017

View reviewed changes

surajnarwade force-pushed the label-fix branch from 9030394 to edd715a Compare May 8, 2017 12:12

cdrage requested changes May 8, 2017

View reviewed changes

surajssd reviewed May 9, 2017

View reviewed changes

surajnarwade force-pushed the label-fix branch from edd715a to 9ff231c Compare May 10, 2017 06:51

surajssd suggested changes May 15, 2017

View reviewed changes

surajnarwade force-pushed the label-fix branch from 9ff231c to c84d990 Compare May 15, 2017 14:31

cdrage reviewed May 15, 2017

View reviewed changes

surajnarwade force-pushed the label-fix branch 2 times, most recently from a825926 to 8198246 Compare May 15, 2017 16:25

cdrage approved these changes May 16, 2017

View reviewed changes

surajnarwade force-pushed the label-fix branch from 8198246 to 7ca3394 Compare May 17, 2017 10:31

concaf suggested changes May 17, 2017

View reviewed changes

surajnarwade force-pushed the label-fix branch 3 times, most recently from 6e07ead to 8a8ac81 Compare May 22, 2017 07:12

concaf approved these changes May 25, 2017

View reviewed changes

cdrage reviewed May 29, 2017

View reviewed changes

surajnarwade force-pushed the label-fix branch from 8a8ac81 to 6870fb4 Compare May 30, 2017 11:48

surajnarwade force-pushed the label-fix branch from 6870fb4 to 7308a22 Compare June 2, 2017 08:50

kadel requested changes Jun 2, 2017

View reviewed changes

surajnarwade force-pushed the label-fix branch from 7308a22 to 961e7d5 Compare June 5, 2017 06:34

surajnarwade force-pushed the label-fix branch from 961e7d5 to be95128 Compare June 13, 2017 06:26

cdrage requested changes Jun 13, 2017

View reviewed changes

surajnarwade force-pushed the label-fix branch 3 times, most recently from 642406a to 5ccc7cd Compare June 14, 2017 06:08

surajssd approved these changes Jun 14, 2017

View reviewed changes

kadel approved these changes Jun 14, 2017

View reviewed changes

cdrage approved these changes Jun 14, 2017

View reviewed changes

Failing when port is specified with labels

209cda2

Resolves kubernetes#522 Kompose will give FATAL error if labels are given but ports are not defined

surajnarwade force-pushed the label-fix branch from 5ccc7cd to 209cda2 Compare June 15, 2017 04:16

surajssd merged commit f823a22 into kubernetes:master Jun 15, 2017


		#### Warning about Labels

		`kompose.service.type` label should be defined with `ports` only, otherwise `kompose` will fail.

		@@ -401,3 +402,5 @@ If the Docker Compose file has a volume specified for a service, the Deployment
		If the Docker Compose file has service name with `_` in it (eg.`web_service`), then it will be replaced by `-` and the service name will be renamed accordingly (eg.`web-service`). Kompose does this because "Kubernetes" doesn't allow `_` in object name.

		Please note that changing service name might break some `docker-compose` files.

Failing when port is specified with labels #590

Failing when port is specified with labels #590

Conversation

surajnarwade commented May 8, 2017

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

surajnarwade commented May 15, 2017

surajssd commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajnarwade commented May 16, 2017

cdrage commented May 16, 2017

surajssd commented May 17, 2017

gitlawr commented May 17, 2017

surajnarwade commented May 17, 2017

gitlawr commented May 17, 2017

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

surajnarwade commented May 29, 2017

surajssd commented May 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajnarwade commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajnarwade commented Jun 6, 2017

Choose a reason for hiding this comment

surajnarwade commented Jun 14, 2017

surajssd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajnarwade commented Jun 15, 2017