Added support for `group_add` key #739

surajnarwade · 2017-08-02T10:09:41Z

No description provided.

kompose-bot · 2017-08-02T10:09:43Z

@surajnarwade, thank you for the pull request! We'll ping some people to review your PR. @surajssd and @containscafeine, please review this.

cdrage · 2017-08-02T14:12:38Z

pkg/loader/compose/v1v2.go

+		for _, i := range composeServiceConfig.GroupAdd {
+			j, err := strconv.Atoi(i)
+			if err != nil {
+				panic(err)


needs to use errors.new for this

cdrage · 2017-08-02T14:13:36Z

pkg/transformer/kubernetes/kubernetes.go

@@ -103,8 +103,11 @@ func (k *Kubernetes) CheckUnsupportedKey(komposeObject *kobject.KomposeObject, u
 }

 // InitPodSpec creates the pod specification
-func (k *Kubernetes) InitPodSpec(name string, image string) api.PodSpec {
+func (k *Kubernetes) InitPodSpec(name string, image string, groupadd []int64) api.PodSpec {


no need to add groupadd here, we can add this to k8sutils.go, having it in kubernetes.go / openshift.go will make InitPodSpec messy with multiple input variables.

Have a look at how we transform the Kubernetes artifacts in k8sutils.go

cdrage · 2017-08-02T14:14:20Z

script/test/fixtures/buildargs/output-os-template.json

@@ -113,7 +113,8 @@
                "resources": {}
              }
            ],
-            "restartPolicy": "Always"
+            "restartPolicy": "Always",
+	    "securityContext": {}


all of the tests add securityContext, we should have an if statement in the code that ONLY adds securityContext if it's been supplied. no need for blank resources like this.

surajnarwade · 2017-08-07T05:37:29Z

I have updated PR with suggested changes, @cdrage @surajssd needs review on it

concaf

A couple of nits, and maybe add a failing test for both providers, and this is good to go.
Great work, @surajnarwade 🎉

concaf · 2017-08-07T07:25:22Z

pkg/loader/compose/v1v2.go

+		for _, i := range composeServiceConfig.GroupAdd {
+			j, err := strconv.Atoi(i)
+			if err != nil {
+				return kobject.KomposeObject{}, errors.Wrap(err, "Unable to get group_add key")


nit: start the error with lowercase

concaf · 2017-08-07T07:26:29Z

pkg/loader/compose/v1v2.go

@@ -264,6 +264,16 @@ func libComposeToKomposeMapping(composeObject *project.Project) (kobject.Kompose
 		serviceConfig.MemLimit = composeServiceConfig.MemLimit
 		serviceConfig.TmpFs = composeServiceConfig.Tmpfs
 		serviceConfig.StopGracePeriod = composeServiceConfig.StopGracePeriod
+		var t2 []int64


nit: what does t2 mean? How about a more explanatory variable name?

concaf · 2017-08-07T07:29:55Z

script/test/cmd/tests.sh

+# kubernetes test
+convert::expect_success "kompose -f $KOMPOSE_ROOT/script/test/fixtures/group-add/docker-compose.yml convert --stdout -j" "$KOMPOSE_ROOT/script/test/fixtures/group-add/output-k8s.json"
+# openshift test
+convert::expect_success "kompose --provider openshift -f $KOMPOSE_ROOT/script/test/fixtures/group-add/docker-compose.yml convert --stdout -j" "$KOMPOSE_ROOT/script/test/fixtures/group-add/output-os.json"



Can we add a failing test for both the providers? Makes the tests more robust.

concaf

LGTM

cdrage

Forget v3 support though, they've added a new key called config that configured uid / guid...

Other than two comments that suggest some changes, LGTM.

cdrage · 2017-08-08T14:03:30Z

script/test/fixtures/gitlab/docker-compose.yml

@@ -4,7 +4,7 @@ services:
  postgresql:
    image: swordphilic/postgresql
    ports:
-      - "$DB_PORT"
+      - $DB_PORT


Why is this changed? Please revert back to with quotes.

cdrage · 2017-08-08T14:04:36Z

pkg/loader/compose/v1v2.go

@@ -264,6 +264,16 @@ func libComposeToKomposeMapping(composeObject *project.Project) (kobject.Kompose
 		serviceConfig.MemLimit = composeServiceConfig.MemLimit
 		serviceConfig.TmpFs = composeServiceConfig.Tmpfs
 		serviceConfig.StopGracePeriod = composeServiceConfig.StopGracePeriod
+		var groupAdd []int64


Please add spacing, newline here as well as a newline below (after serviceConfig.GroupAdd = groupAdd).

Since this is a range, perhaps it may be better to make this into a separate function?

Also add a small comment please :)

cdrage · 2017-08-08T14:13:56Z

Please add a description to your commit as well as update this original PR description.

This PR will add support for `group_add` key which will map to supplemental group in pod security context.

surajnarwade · 2017-08-10T06:25:25Z

@cdrage updated with suggested changes

cdrage · 2017-08-10T15:02:43Z

Awesome! This is great. Changes are good. Thanks @surajnarwade for the hard work.

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

kompose-bot requested review from concaf and surajssd August 2, 2017 10:09

surajnarwade changed the title ~~Added support for group_add key~~ [WIP] Added support for group_add key Aug 2, 2017

cdrage requested changes Aug 2, 2017

View reviewed changes

surajnarwade force-pushed the group_add branch 2 times, most recently from a321210 to aad3b25 Compare August 4, 2017 05:32

surajnarwade changed the title ~~[WIP] Added support for group_add key~~ Added support for group_add key Aug 4, 2017

concaf suggested changes Aug 7, 2017

View reviewed changes

surajnarwade force-pushed the group_add branch from aad3b25 to e1f4380 Compare August 7, 2017 09:15

concaf approved these changes Aug 8, 2017

View reviewed changes

cdrage requested changes Aug 8, 2017

View reviewed changes

surajnarwade force-pushed the group_add branch 3 times, most recently from 483b750 to d2d76cc Compare August 10, 2017 06:23

Added support for group_add key

641f8f8

This PR will add support for `group_add` key which will map to supplemental group in pod security context.

surajnarwade force-pushed the group_add branch from d2d76cc to 641f8f8 Compare August 10, 2017 06:24

cdrage approved these changes Aug 10, 2017

View reviewed changes

cdrage merged commit 214cb48 into kubernetes:master Aug 11, 2017

This was referenced Aug 11, 2017

Revert " Added support for group_add key" #758

Closed

Support for: group_add #437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for `group_add` key #739

Added support for `group_add` key #739

surajnarwade commented Aug 2, 2017

kompose-bot commented Aug 2, 2017

cdrage Aug 2, 2017

cdrage Aug 2, 2017 •

edited

Loading

cdrage Aug 2, 2017

surajnarwade commented Aug 7, 2017

concaf left a comment

concaf Aug 7, 2017

concaf Aug 7, 2017

concaf Aug 7, 2017

concaf left a comment

cdrage left a comment

cdrage Aug 8, 2017

cdrage Aug 8, 2017

cdrage commented Aug 8, 2017

surajnarwade commented Aug 10, 2017

cdrage commented Aug 10, 2017

Added support for group_add key #739

Added support for group_add key #739

Conversation

surajnarwade commented Aug 2, 2017

kompose-bot commented Aug 2, 2017

cdrage Aug 2, 2017

Choose a reason for hiding this comment

cdrage Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

cdrage Aug 2, 2017

Choose a reason for hiding this comment

surajnarwade commented Aug 7, 2017

concaf left a comment

Choose a reason for hiding this comment

concaf Aug 7, 2017

Choose a reason for hiding this comment

concaf Aug 7, 2017

Choose a reason for hiding this comment

concaf Aug 7, 2017

Choose a reason for hiding this comment

concaf left a comment

Choose a reason for hiding this comment

cdrage left a comment

Choose a reason for hiding this comment

cdrage Aug 8, 2017

Choose a reason for hiding this comment

cdrage Aug 8, 2017

Choose a reason for hiding this comment

cdrage commented Aug 8, 2017

surajnarwade commented Aug 10, 2017

cdrage commented Aug 10, 2017

Added support for `group_add` key #739

Added support for `group_add` key #739

cdrage Aug 2, 2017 •

edited

Loading