PVC name changes if volumes containes .(dot) in it #603

procrypt · 2017-05-16T09:59:00Z

If the docker-compose.yml has volumes defined like this

   volumes:
     - .:/code

Then pvc generated by kompose will be like .-persistentvolumeclaim.yaml. This PR handles the .(dot) in the pvc name.

docker-compose.yml file used

web:
  image: flask_web      
  command: python app.py
  ports:
   - "5000:5000"
  volumes:
   - .:/code
  links:
   - redis
redis:
  image: redis

Master output

$ kompose convert -f volume.yml -o sample/
INFO file "sample/redis-service.yaml" created     
INFO file "sample/web-service.yaml" created       
INFO file "sample/redis-deployment.yaml" created  
INFO file "sample/web-deployment.yaml" created    
INFO file "sample/.-persistentvolumeclaim.yaml" created

This PR output.

$ kompose convert -f volume.yml -o config/
INFO file "config/redis-service.yaml" created     
INFO file "config/web-service.yaml" created       
INFO file "config/redis-deployment.yaml" created  
INFO file "config/web-deployment.yaml" created    
INFO file "config/web-claim0-persistentvolumeclaim.yaml" created

Fixes: #584

cc: @surajssd @cdrage @surajnarwade

cdrage · 2017-05-17T15:06:31Z

pkg/loader/compose/compose.go

@@ -337,7 +337,12 @@ func (c *Compose) LoadFile(files []string) (kobject.KomposeObject, error) {
 		if composeServiceConfig.Volumes != nil {
 			for _, volume := range composeServiceConfig.Volumes.Volumes {
 				v := normalizeServiceNames(volume.String())
-				serviceConfig.Volumes = append(serviceConfig.Volumes, v)
+				if strings.Contains(v, ".") {


Instead of putting this if statement here (making compose.go even larger). Why not put the if / else statement inside of func normalizeVolumeNames ?

surajssd

We don't need to do all of these things, check my comment at:
#584 (comment)

surajssd · 2017-05-23T13:48:06Z

script/test/fixtures/change-in-volume/docker-compose.yml

@@ -0,0 +1,11 @@
+web:


let's do version 2 please?

surajssd · 2017-05-23T13:48:29Z

script/test/cmd/tests.sh

@@ -218,6 +218,13 @@ cd "$KOMPOSE_ROOT/script/test/fixtures/nginx-node-redis/node"
 convert::expect_success_and_warning "kompose convert  --provider openshift --stdout -j -f ../docker-compose.yml" "/tmp/output-os.json" "$warning"
 cd $CURRENT_DIR

+####
+# Test related to change in volume-name


a better line which mentions what this test is about?

procrypt · 2017-05-24T13:24:52Z

All tests are passing locally.

cdrage · 2017-05-24T14:40:16Z

@procrypt Nope, they're not. Travis still says failing.

surajssd · 2017-05-29T07:10:40Z

@procrypt update the fixtures with the latest code changes, I mean regenerate them again, it seems that it is taking name of fixture as ''.".

surajssd · 2017-05-29T09:45:06Z

@procrypt openshift output matches but k8s does not?

procrypt · 2017-05-29T10:15:40Z

@surajssd Test are passing locally.

cdrage · 2017-05-29T14:00:41Z

@procrypt Doesn't matter if tests are passing locally or not, it has to pass on Travis in order to get approval. You may have to run these tests manually and troubleshoot to figure out what may be the issue.

procrypt · 2017-05-30T05:49:18Z

@cdrage I have no idea why kubernetes test is failing on travis :(
@kadel can you please take a look at it.

kadel · 2017-06-07T14:12:16Z

Yeh, this will be quite hard to debug.
It might be time look at #604 it will make it easier to debug issues like this

ashetty1 · 2017-06-08T10:18:48Z

@procrypt tests are failing locally for me with kubernetes. I see that the patch fixes the bug; so it could be a case where the output file you are comparing against is incorrect?

procrypt · 2017-06-13T05:36:02Z

@surajssd @cdrage @kadel @ashetty1 Ping all tests are passing now.

ashetty1 · 2017-06-13T06:47:38Z

LGTM

cdrage

other than small change for port, this code LGTM.

cdrage · 2017-06-13T14:04:34Z

script/test/fixtures/multiple-compose-files/docker-os.yml

@@ -16,7 +16,7 @@ services:
  etherpad:
    image: centos/etherpad
    ports:
-      - "80:9001"
+      - "81:9001"


why 81? please change back to 80 :(

surajssd · 2017-06-14T06:19:29Z

The commit message needs to framed better also the extra tab space makes it look more weird

surajssd · 2017-06-14T06:39:37Z

tried running this locally, it fails for some reason:

The test fails:

$ kompose version
0.6.0 (6faa732)

$ make test-cmd 
./script/test/cmd/tests.sh
...
===> Starting test <===
convert::expect_success_and_warning: Running: 'kompose convert -f
/home/hummer/go/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/change-in-volume/docker-compose.yml
--stdout -j' expected_output:
'/home/hummer/go/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/change-in-volume/output-k8s.json'
expected_warning: 'Volume mount on the host "." isn't supported - ignoring path on the host'
FAIL: converted output does not match
...

So when I tried re-generating the output-k8s.json it is not generating labels.

regenerate:

✔ ~/go/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/change-in-volume [pr_603 L|✔] 
12:02 $ kompose convert --stdout -j > ./output-k8s.json 
WARN Volume mount on the host "." isn't supported - ignoring path on the host 
✔ ~/go/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/change-in-volume [pr_603 L|✚ 1]

diff:

$ git diff
diff --git a/script/test/fixtures/change-in-volume/output-k8s.json b/script/test/fixtures/change-in-volume/output-k8s.json
index ea27825..6ee4489 100644
--- a/script/test/fixtures/change-in-volume/output-k8s.json
+++ b/script/test/fixtures/change-in-volume/output-k8s.json
@@ -61,10 +61,7 @@
       "apiVersion": "extensions/v1beta1",
       "metadata": {
         "name": "redis",
-        "creationTimestamp": null,
-        "labels": {
-          "io.kompose.service": "redis"
-        }
+        "creationTimestamp": null
       },
       "spec": {
         "replicas": 1,
@@ -95,10 +92,7 @@
       "apiVersion": "extensions/v1beta1",
       "metadata": {
         "name": "web",
-        "creationTimestamp": null,
-        "labels": {
-          "io.kompose.service": "web"
-        }
+        "creationTimestamp": null
       },
       "spec": {
         "replicas": 1,

if volumes in docker file is defined as volumes: - .:/code Then the PVC created by kompose will containes .(dot) in it. .-persistentvolumeclaim.yaml

procrypt · 2017-06-14T09:12:50Z

@surajssd can you review it again when you get some time, I have updated the PR.

surajssd

LGTM

surajssd · 2017-06-16T04:17:06Z

waiting on @cdrage to approve and merge!

procrypt · 2017-06-16T07:44:14Z

Awesome :)

surajnarwade · 2017-06-16T10:26:35Z

LGTM 👍

surajssd · 2017-06-17T05:52:44Z

@cdrage you need to approve to merge this!

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

procrypt changed the title ~~PVC name changes if volumes container .(dot) in it.~~ PVC name changes if volumes containes .(dot) in it. May 16, 2017

procrypt changed the title ~~PVC name changes if volumes containes .(dot) in it.~~ PVC name changes if volumes containes .(dot) in it May 16, 2017

procrypt force-pushed the error_with_volume_name branch from 49a108e to 072d7a2 Compare May 16, 2017 10:01

cdrage reviewed May 17, 2017

View reviewed changes

surajssd suggested changes May 18, 2017

View reviewed changes

procrypt force-pushed the error_with_volume_name branch from 072d7a2 to 370ebb4 Compare May 23, 2017 06:23

surajssd reviewed May 23, 2017

View reviewed changes

procrypt force-pushed the error_with_volume_name branch from 370ebb4 to 50f7771 Compare May 24, 2017 13:12

procrypt force-pushed the error_with_volume_name branch from 50f7771 to 15a1372 Compare May 24, 2017 13:33

procrypt force-pushed the error_with_volume_name branch 2 times, most recently from a7e18e4 to 6f0da4d Compare May 29, 2017 09:11

procrypt force-pushed the error_with_volume_name branch 2 times, most recently from c39b1b2 to bbf901d Compare May 29, 2017 10:33

procrypt force-pushed the error_with_volume_name branch 2 times, most recently from 054271f to 39a0ca4 Compare June 12, 2017 18:59

cdrage requested changes Jun 13, 2017

View reviewed changes

procrypt force-pushed the error_with_volume_name branch from 39a0ca4 to 6faa732 Compare June 14, 2017 06:17

procrypt force-pushed the error_with_volume_name branch from 6faa732 to 4baf4fe Compare June 14, 2017 08:19

change volume name with generated deployment file

c40bb4b

if volumes in docker file is defined as volumes: - .:/code Then the PVC created by kompose will containes .(dot) in it. .-persistentvolumeclaim.yaml

procrypt force-pushed the error_with_volume_name branch from 4baf4fe to c40bb4b Compare June 14, 2017 09:01

surajssd approved these changes Jun 16, 2017

View reviewed changes

procrypt closed this Jun 16, 2017

procrypt reopened this Jun 16, 2017

cdrage approved these changes Jun 19, 2017

View reviewed changes

cdrage merged commit ee79612 into kubernetes:master Jun 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PVC name changes if volumes containes .(dot) in it #603

PVC name changes if volumes containes .(dot) in it #603

procrypt commented May 16, 2017 •

edited

Loading

cdrage May 17, 2017

surajssd left a comment

surajssd May 23, 2017

surajssd May 23, 2017

procrypt commented May 24, 2017

cdrage commented May 24, 2017

surajssd commented May 29, 2017

surajssd commented May 29, 2017

procrypt commented May 29, 2017

cdrage commented May 29, 2017 •

edited

Loading

procrypt commented May 30, 2017 •

edited

Loading

kadel commented Jun 7, 2017

ashetty1 commented Jun 8, 2017

procrypt commented Jun 13, 2017

ashetty1 commented Jun 13, 2017

cdrage left a comment

cdrage Jun 13, 2017

surajssd commented Jun 14, 2017

surajssd commented Jun 14, 2017

procrypt commented Jun 14, 2017

surajssd left a comment

surajssd commented Jun 16, 2017

procrypt commented Jun 16, 2017

surajnarwade commented Jun 16, 2017

surajssd commented Jun 17, 2017

PVC name changes if volumes containes .(dot) in it #603

PVC name changes if volumes containes .(dot) in it #603

Conversation

procrypt commented May 16, 2017 • edited Loading

cdrage May 17, 2017

Choose a reason for hiding this comment

surajssd left a comment

Choose a reason for hiding this comment

surajssd May 23, 2017

Choose a reason for hiding this comment

surajssd May 23, 2017

Choose a reason for hiding this comment

procrypt commented May 24, 2017

cdrage commented May 24, 2017

surajssd commented May 29, 2017

surajssd commented May 29, 2017

procrypt commented May 29, 2017

cdrage commented May 29, 2017 • edited Loading

procrypt commented May 30, 2017 • edited Loading

kadel commented Jun 7, 2017

ashetty1 commented Jun 8, 2017

procrypt commented Jun 13, 2017

ashetty1 commented Jun 13, 2017

cdrage left a comment

Choose a reason for hiding this comment

cdrage Jun 13, 2017

Choose a reason for hiding this comment

surajssd commented Jun 14, 2017

surajssd commented Jun 14, 2017

procrypt commented Jun 14, 2017

surajssd left a comment

Choose a reason for hiding this comment

surajssd commented Jun 16, 2017

procrypt commented Jun 16, 2017

surajnarwade commented Jun 16, 2017

surajssd commented Jun 17, 2017

procrypt commented May 16, 2017 •

edited

Loading

cdrage commented May 29, 2017 •

edited

Loading

procrypt commented May 30, 2017 •

edited

Loading