Propagate underscore into valid name #509

procrypt · 2017-03-24T04:27:12Z

Now we can provide service name with _ in docker-compose files and they will by converted as - in the generated artifacts by kompose eg, if the service name in docker-compose file is "foo_bar" then it will be converted into "foo-bar" in the generated artifacts.

docker-compose.yml containing _ in service name.

version: "2"
services:
  web_service:
    image: tuna/docker-counter23:latest
    ports:
      - "5000:5000"
    links:
      - redis_service
  redis_service:
    image: redis:latest
    ports:
      - "6379"

$ kompose up
INFO[0000] Service name in docker-compose has been changed from "redis_service" to "redis-service" 
INFO[0000] Service name in docker-compose has been changed from "web_service" to "web-service" 

We are going to create Kubernetes Deployments, Services and PersistentVolumeClaims for your Dockerized application. 
If you need different kind of resources, use the 'kompose convert' and 'kubectl create -f' commands instead. 

INFO[0000] Successfully created Service: redis-service  
INFO[0000] Successfully created Service: web-service    
INFO[0000] Successfully created Deployment: redis-service 
INFO[0000] Successfully created Deployment: web-service 

Your application has been deployed to Kubernetes. You can run 'kubectl get deployment,svc,pods,pvc' for details.

docker-compose.yml without _ in service name.

version: "2"
services:
  web:
    image: tuna/docker-counter23:latest
    ports:
      - "5000:5000"
    links:
      - redis
  redis:
    image: redis:latest
    ports:
      - "6379"

$ kompose up
We are going to create Kubernetes Deployments, Services and PersistentVolumeClaims for your Dockerized application. 
If you need different kind of resources, use the 'kompose convert' and 'kubectl create -f' commands instead. 

INFO[0000] Successfully created Service: redis          
INFO[0000] Successfully created Service: web            
INFO[0000] Successfully created Deployment: redis       
INFO[0000] Successfully created Deployment: web         

Your application has been deployed to Kubernetes. You can run 'kubectl get deployment,svc,pods,pvc' for details.

cc: @kadel @cdrage
Fixes: #420

kadel · 2017-03-27T13:44:24Z

We shouldn't do this in every place where name is used.
This should be fixed when docker-compose file is loaded into KObject. (https://github.com/kubernetes-incubator/kompose/blob/ec897ef50facdd1c2518cd074bf3b5fcc56bae7c/pkg/loader/compose/compose.go#L377)

Than you have to do it only once, and you can be sure that it will be correct in every place after that.

procrypt · 2017-03-27T19:13:34Z

@kadel @surajssd Rebased and updated.

cdrage · 2017-03-27T20:05:09Z

pkg/transformer/kubernetes/kubernetes.go

@@ -637,7 +637,7 @@ func (k *Kubernetes) Deploy(komposeObject kobject.KomposeObject, opt kobject.Con
 	if !opt.EmptyVols {
 		pvcStr = " and PersistentVolumeClaims "
 	}
-	fmt.Println("We are going to create Kubernetes Deployments, Services" + pvcStr + "for your Dockerized application. \n" +
+	fmt.Println("\nWe are going to create Kubernetes Deployments, Services" + pvcStr + "for your Dockerized application. \n" +


Remove the \n

This may be a good opportunity to change this into a log.Info instead of a Println.

cdrage · 2017-03-27T20:05:51Z

pkg/loader/compose/compose.go

@@ -374,7 +374,10 @@ func (c *Compose) LoadFile(files []string) (kobject.KomposeObject, error) {
 		serviceConfig.Tty = composeServiceConfig.Tty
 		serviceConfig.MemLimit = composeServiceConfig.MemLimit
 		serviceConfig.TmpFs = composeServiceConfig.Tmpfs
-		komposeObject.ServiceConfigs[name] = serviceConfig
+		komposeObject.ServiceConfigs[normalizeServiceNames(name)] = serviceConfig
+		if !reflect.DeepEqual(komposeObject.ServiceConfigs[normalizeServiceNames(name)], komposeObject.ServiceConfigs[name]) {


Should @procrypt add a test for this @kadel ?

@cdrage Test are already there, the function normalizeServiceNames() was written by @surajssd but we never used it.
Tests for normalizeServiceNames().

@procrypt ah, makes sense!

That reflect seems little bit too much :-(
Can same think be done a bit easier without reflect and more readable?
Why compere whole struct? Comparing just names should be enough no?

surajssd · 2017-03-28T07:42:05Z

pkg/loader/compose/compose.go

-		komposeObject.ServiceConfigs[name] = serviceConfig
+		komposeObject.ServiceConfigs[normalizeServiceNames(name)] = serviceConfig
+		if !reflect.DeepEqual(komposeObject.ServiceConfigs[normalizeServiceNames(name)], komposeObject.ServiceConfigs[name]) {
+			log.Infof("Service name in docker-compose has been changed from %c%s%c to %c%s%c", '"', name, '"', '"', normalizeServiceNames(name), '"')


if you want to print " in the output then you can use format specifier %q and get the same result, so you don't need to do the hack currently done!

This might help https://golang.org/pkg/fmt/

@surajssd Thanks 👍

cdrage

LGTM

procrypt · 2017-03-29T03:11:10Z

@kadel Updated 😉

kadel · 2017-03-30T08:20:24Z

pkg/loader/compose/compose.go

@@ -374,7 +374,10 @@ func (c *Compose) LoadFile(files []string) (kobject.KomposeObject, error) {
 		serviceConfig.Tty = composeServiceConfig.Tty
 		serviceConfig.MemLimit = composeServiceConfig.MemLimit
 		serviceConfig.TmpFs = composeServiceConfig.Tmpfs
-		komposeObject.ServiceConfigs[name] = serviceConfig
+		komposeObject.ServiceConfigs[normalizeServiceNames(name)] = serviceConfig
+		if komposeObject.ServiceConfigs[normalizeServiceNames(name)].Image != komposeObject.ServiceConfigs[name].Image {


This confuses me a lot.
Why are you comparing Images?

@kadel I didn't see that sorry, I have updated the PR.

Now we can provide service name with "_" in docker-compose files and they will by converted as "-" in the generated artifacts by kompose eg, if the service name in docker-compose file is "foo_bar" then it will be converted into "foo-bar" in the generated artifacts

sylus · 2017-04-03T16:53:19Z

Excited for this to get in so can properly use kompose :) Thanks so much!

kadel · 2017-04-03T17:40:07Z

Excited for this to get in so can properly use kompose :) Thanks so much!

Just beware that in some cases this also renames dns name that points to renamed service. In some cases this can break communication between services :-(

surajssd · 2017-04-03T20:46:18Z

pkg/transformer/kubernetes/kubernetes.go

@@ -637,7 +637,7 @@ func (k *Kubernetes) Deploy(komposeObject kobject.KomposeObject, opt kobject.Con
 	if !opt.EmptyVols {
 		pvcStr = " and PersistentVolumeClaims "
 	}
-	fmt.Println("We are going to create Kubernetes Deployments, Services" + pvcStr + "for your Dockerized application. \n" +
+	log.Info("We are going to create Kubernetes Deployments, Services" + pvcStr + "for your Dockerized application. " +


IIRC It was purposefully kept as Println

@surajssd Why?

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

procrypt force-pushed the propagate_underscore branch 2 times, most recently from 8d81b48 to afa0240 Compare March 24, 2017 04:30

surajssd added the rebase needed label Mar 27, 2017

procrypt force-pushed the propagate_underscore branch 6 times, most recently from b244494 to 94e6c7f Compare March 27, 2017 18:43

procrypt force-pushed the propagate_underscore branch from 94e6c7f to dea681c Compare March 27, 2017 19:36

cdrage requested changes Mar 27, 2017

View reviewed changes

procrypt force-pushed the propagate_underscore branch 2 times, most recently from 4354b4f to 8d1f4bd Compare March 28, 2017 03:52

surajssd reviewed Mar 28, 2017

View reviewed changes

procrypt force-pushed the propagate_underscore branch from 8d1f4bd to d5aa4c8 Compare March 28, 2017 08:09

cdrage approved these changes Mar 28, 2017

View reviewed changes

idcrook mentioned this pull request Mar 28, 2017

underscores get propagated into invalid names #420

Closed

procrypt force-pushed the propagate_underscore branch 2 times, most recently from faa8091 to fdb4878 Compare March 29, 2017 03:10

kadel reviewed Mar 30, 2017

View reviewed changes

procrypt force-pushed the propagate_underscore branch from fdb4878 to 6e0b934 Compare April 3, 2017 07:36

cdrage approved these changes Apr 3, 2017

View reviewed changes

kadel mentioned this pull request Apr 3, 2017

add documentation about renaming service #538

Closed

kadel approved these changes Apr 3, 2017

View reviewed changes

kadel merged commit a24084d into kubernetes:master Apr 3, 2017

surajssd reviewed Apr 3, 2017

View reviewed changes

surajssd mentioned this pull request Apr 21, 2017

fix driver:local in prefixing volumes with current dir name #557

Merged

rcmorano mentioned this pull request Aug 11, 2018

dots get propagated into invalid names #1062

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate underscore into valid name #509

Propagate underscore into valid name #509

procrypt commented Mar 24, 2017 •

edited

Loading

kadel commented Mar 27, 2017

procrypt commented Mar 27, 2017

cdrage Mar 27, 2017

cdrage Mar 27, 2017

procrypt Mar 28, 2017

cdrage Mar 28, 2017

kadel Mar 28, 2017

surajssd Mar 28, 2017

surajssd Mar 28, 2017

procrypt Mar 28, 2017

cdrage left a comment

procrypt commented Mar 29, 2017

kadel Mar 30, 2017

procrypt Apr 3, 2017

sylus commented Apr 3, 2017

kadel commented Apr 3, 2017

surajssd Apr 3, 2017

cdrage Apr 4, 2017

Propagate underscore into valid name #509

Propagate underscore into valid name #509

Conversation

procrypt commented Mar 24, 2017 • edited Loading

kadel commented Mar 27, 2017

procrypt commented Mar 27, 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

cdrage left a comment

Choose a reason for hiding this comment

procrypt commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylus commented Apr 3, 2017

kadel commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

procrypt commented Mar 24, 2017 •

edited

Loading