added support for `restart-policy` keys in v3 #666

surajnarwade · 2017-06-22T12:05:05Z

Resolves restart_policy from issue #644

cdrage · 2017-06-22T13:45:32Z

script/test/fixtures/v3/output-k8s-full-example.json

@@ -160,7 +160,7 @@
        }
      },
      "spec": {
-        "replicas": 1,
+        "replicas": 6,


neither should this..

but there are replicas=6 in docker-compose file, right ?

cdrage · 2017-06-22T13:45:33Z

script/test/fixtures/v3/output-env-subs.json

@@ -56,8 +56,7 @@
                "image": "foo/bar:latest",
                "env": [
                  {
-                    "name": "FOO",
-                    "value": "bar"
+                    "name": "FOO"


this test should not be changed

but results are like this, so I changed

cdrage · 2017-06-22T13:48:14Z

pkg/loader/compose/v3.go

 		// This is a bit messy since we use yaml.MemStringorInt
 		// TODO: Refactor yaml.MemStringorInt in kobject.go to int64
 		// Since Deploy.Resources.Limits does not initialize, we must check type Resources before continuing
 		if (composeServiceConfig.Deploy.Resources != types.Resources{}) {
 			serviceConfig.MemLimit = libcomposeyaml.MemStringorInt(composeServiceConfig.Deploy.Resources.Limits.MemoryBytes)
 		}
+		//Handling restart-policy
+		serviceConfig.Restart = composeServiceConfig.Deploy.RestartPolicy.Condition


Please add a small comment here in regards to deploy.

Example:

// Here we handle all Docker Compose Deploy keys

So we can better organize from the rest of all the keys :)

cdrage · 2017-06-23T12:38:24Z

script/test/fixtures/v3/output-k8s-full-example.json

@@ -363,13 +358,10 @@
                "stdin": true,
                "tty": true
              }
-            ],
-            "restartPolicy": "Always"


cdrage · 2017-06-23T12:38:27Z

script/test/fixtures/v3/output-k8s-full-example.json

-        "annotations": {
-          "com.example.description": "Accounting webapp",
-          "com.example.empty-label": "",
-          "com.example.number": "42"


Why are these being deleted?

@cdrage, they are not deleted, just order is misplaced

cdrage · 2017-06-23T12:38:28Z

script/test/fixtures/v3/output-k8s-full-example.json

          }
        },
-        "strategy": {
-          "type": "Recreate"
-        }


Same as here?

cdrage · 2017-06-23T12:39:01Z

script/test/fixtures/v3/output-os-full-example.json

-        "annotations": {
-          "com.example.description": "Accounting webapp",
-          "com.example.empty-label": "",
-          "com.example.number": "42"


Annotations should still work ^^

surajnarwade · 2017-06-27T09:39:49Z

@cdrage , there was typo ( on_failure >> on-failure) in example due to which output was varying, can you please review now.

cdrage · 2017-06-27T12:51:03Z

pkg/loader/compose/v3.go

 		// This is a bit messy since we use yaml.MemStringorInt
 		// TODO: Refactor yaml.MemStringorInt in kobject.go to int64
 		// Since Deploy.Resources.Limits does not initialize, we must check type Resources before continuing
 		if (composeServiceConfig.Deploy.Resources != types.Resources{}) {
 			serviceConfig.MemLimit = libcomposeyaml.MemStringorInt(composeServiceConfig.Deploy.Resources.Limits.MemoryBytes)
 		}
-
+		//Here we handle all Docker Compose Deploy keys
+		//Handling restart-policy


please add spaces after //

cdrage · 2017-06-27T12:51:29Z

pkg/loader/compose/v3.go

-
+		//Here we handle all Docker Compose Deploy keys
+		//Handling restart-policy
+		if composeServiceConfig.Deploy.RestartPolicy != nil {


no need for != nil, just put: if composeServiceConfig.Deploy.RestartPolicy {

cdrage · 2017-06-27T12:53:54Z

script/test/fixtures/v3/docker-compose-full-example.yaml

@@ -39,7 +39,7 @@ services:
          cpus: '0.0001'
          memory: 20M
      restart_policy:
-        condition: on_failure
+        condition: on-failure


unfortunatley, docker says that it works with either _ or -. 👎 but keep this change in!

fixed: docker/cli#262

cdrage · 2017-06-27T12:55:35Z

pkg/transformer/kubernetes/k8sutils.go

 			template.Spec.RestartPolicy = api.RestartPolicyAlways
-		case "no":
+		case "no", "none":
 			template.Spec.RestartPolicy = api.RestartPolicyNever
 		case "on-failure":


will actually have to add on_failure as well to here!

but https://docs.docker.com/compose/compose-file/#restart_policy shows on-failure

on-failure case is already there ?

I know, but on_failure is still supported. I can't remember where in docker/cli is it, but it's there. It automatically converts. Unfortunately, some examples use on_failure.

surajnarwade · 2017-06-29T06:43:25Z

@cdrage , on_failure case added, final review please and ready for merge :)

cdrage

LGTM after my small comment (remove on_failure since it was fixed here: docker/cli#262)

Thanks man!!

cdrage · 2017-06-29T15:12:40Z

pkg/transformer/kubernetes/k8sutils.go

 			template.Spec.RestartPolicy = api.RestartPolicyNever
-		case "on-failure":
+		case "on-failure", "on_failure":


so found out it was error upstream. it's fixed now :( so we can remove on_failure. Sorry for confusion!

docker/cli#262

@surajnarwade gotta remove on_failure and we're good 👍

cdrage · 2017-06-29T15:12:52Z

script/test/fixtures/v3/docker-compose-full-example.yaml

@@ -39,7 +39,7 @@ services:
          cpus: '0.0001'
          memory: 20M
      restart_policy:
-        condition: on_failure
+        condition: on-failure


fixed: docker/cli#262

surajnarwade · 2017-06-30T12:37:08Z

@kadel @surajssd needed one more +1 here :)

surajnarwade · 2017-07-04T05:36:26Z

@cdrage , removed on_failure, we are good to go

cdrage · 2017-07-04T13:59:58Z

@surajnarwade thanks for helping me in terms of the confusion! this LGTM

surajnarwade · 2017-07-04T14:21:06Z

@surajssd , needs +1 to get it merged

Resolves `restart_policy` from issue kubernetes#644

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

surajnarwade force-pushed the add-restart-policy-v3 branch 6 times, most recently from 4b20ac5 to 72a94d8 Compare June 22, 2017 13:44

cdrage requested changes Jun 22, 2017

View reviewed changes

surajnarwade changed the title ~~added support for restart-policy keys in v3~~ [WIP]added support for restart-policy keys in v3 Jun 23, 2017

surajnarwade force-pushed the add-restart-policy-v3 branch from 72a94d8 to 9b6450b Compare June 23, 2017 12:28

cdrage requested changes Jun 23, 2017

View reviewed changes

surajnarwade force-pushed the add-restart-policy-v3 branch from 9b6450b to 992cbff Compare June 27, 2017 09:28

cdrage requested changes Jun 27, 2017

View reviewed changes

surajnarwade changed the title ~~[WIP]added support for restart-policy keys in v3~~ added support for restart-policy keys in v3 Jun 27, 2017

surajnarwade force-pushed the add-restart-policy-v3 branch 3 times, most recently from 3cfe312 to 5b97b04 Compare June 29, 2017 06:07

cdrage approved these changes Jun 29, 2017

View reviewed changes

surajnarwade force-pushed the add-restart-policy-v3 branch from 5b97b04 to 7d64c92 Compare July 4, 2017 05:13

surajssd added the rebase needed label Jul 6, 2017

surajnarwade force-pushed the add-restart-policy-v3 branch 3 times, most recently from fcda771 to 8cc8473 Compare July 6, 2017 11:56

added support for restart-policy keys in v3

9dcb2bf

Resolves `restart_policy` from issue kubernetes#644

surajnarwade force-pushed the add-restart-policy-v3 branch from 8cc8473 to 9dcb2bf Compare July 6, 2017 12:14

surajssd merged commit 14dbb30 into kubernetes:master Jul 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added support for `restart-policy` keys in v3 #666

added support for `restart-policy` keys in v3 #666

surajnarwade commented Jun 22, 2017

cdrage Jun 22, 2017

surajnarwade Jun 22, 2017

cdrage Jun 22, 2017

surajnarwade Jun 23, 2017

cdrage Jun 22, 2017

cdrage Jun 23, 2017

cdrage Jun 23, 2017

surajnarwade Jun 27, 2017 •

edited

Loading

cdrage Jun 23, 2017

cdrage Jun 23, 2017

surajnarwade Jun 27, 2017

surajnarwade commented Jun 27, 2017

cdrage Jun 27, 2017

cdrage Jun 27, 2017

cdrage Jun 27, 2017

cdrage Jun 29, 2017

cdrage Jun 27, 2017

surajnarwade Jun 27, 2017

surajnarwade Jun 28, 2017

cdrage Jun 28, 2017

surajnarwade commented Jun 29, 2017

cdrage left a comment

cdrage Jun 29, 2017

cdrage Jun 30, 2017

cdrage Jun 29, 2017

surajnarwade commented Jun 30, 2017

surajnarwade commented Jul 4, 2017

cdrage commented Jul 4, 2017

surajnarwade commented Jul 4, 2017

added support for restart-policy keys in v3 #666

added support for restart-policy keys in v3 #666

Conversation

surajnarwade commented Jun 22, 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

surajnarwade Jun 27, 2017 • edited Loading

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 Jun 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

surajnarwade commented Jun 29, 2017

cdrage left a 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 Jun 30, 2017

surajnarwade commented Jul 4, 2017

cdrage commented Jul 4, 2017

surajnarwade commented Jul 4, 2017

added support for `restart-policy` keys in v3 #666

added support for `restart-policy` keys in v3 #666

surajnarwade Jun 27, 2017 •

edited

Loading