support both : and = as compose envvar separators #197

ngtuna · 2016-10-10T15:43:38Z

fix #196 #173

ngtuna · 2016-10-13T13:34:03Z

could you check it? @surajssd

kadel

I've added few comments, but those can be addressed is separate PRs.

LGTM

kadel · 2016-10-13T14:59:16Z

pkg/loader/compose/compose.go

-	for k, v := range e {
+	var character string
+	for _, e := range envars {
+		//FIXME:(tuna) if envvar string contains both = and :, then = will be first pick up. Consider the case: URL=http://examples.com


One suggestion how to solve this problem. As both : and = are invalid characters for env variable names, we can detect which one is first in string and consider it to be separator.

That's correct 👍 . I will update accordingly.

kadel · 2016-10-13T15:02:20Z

pkg/loader/compose/compose.go

+		} else if strings.Contains(e, ":") {
+			character = ":"
+		} else {
+			logrus.Errorf("Invalid environment variable format, only : and = separators are supported")


just variable name without : or ; is also valid see: https://docs.docker.com/compose/compose-file/#/environment
This is something that we should also consider.

added ec3aa3d

kadel

But there is one think that this PR should include, and that is test for it.

ngtuna · 2016-10-18T07:57:44Z

But there is one think that this PR should include, and that is test for it.

Yes. I will add follow-up commit

surajssd · 2016-10-19T03:02:44Z

@ngtuna I have problem building this branch, can you guys tell me what am I missing?

$ git remote show tuna
* remote tuna
  Fetch URL: https://github.com/ngtuna/kompose
  Push  URL: https://github.com/ngtuna/kompose
  HEAD branch: master
  Remote branches:
    clean-code        tracked
    envvar_colon      tracked
    global-flag       tracked
[SNIP]
    userguide         tracked
  Local branches configured for 'git pull':
    envvar_colon    merges with remote envvar_colon
    project-context merges with remote project-context
    update-objects  merges with remote update-objects
  Local refs configured for 'git push':
    envvar_colon    pushes to envvar_colon    (up to date)
    master          pushes to master          (fast-forwardable)
    project-context pushes to project-context (up to date)
    update-objects  pushes to update-objects  (up to date)

$ git fetch tuna

$ git checkout envvar_colon 
Switched to branch 'envvar_colon'
Your branch is up-to-date with 'tuna/envvar_colon'.

$ git pull
Already up-to-date.

$ build-kompose 
+ cd /home/hummer/go/src/github.com/kubernetes-incubator/kompose
+ go build -tags experimental -o kompose ./cli/main/
# github.com/kubernetes-incubator/kompose/pkg/loader/bundle
pkg/loader/bundle/bundle.go:95: cannot use p (type "github.com/kubernetes-incubator/kompose/vendor/k8s.io/kubernetes/pkg/api".Protocol) as type "github.com/skippbox/kompose/vendor/k8s.io/kubernetes/pkg/api".Protocol in field value
# github.com/kubernetes-incubator/kompose/pkg/loader/compose
pkg/loader/compose/compose.go:101: cannot use proto (type "github.com/kubernetes-incubator/kompose/vendor/k8s.io/kubernetes/pkg/api".Protocol) as type "github.com/skippbox/kompose/vendor/k8s.io/kubernetes/pkg/api".Protocol in field value
pkg/loader/compose/compose.go:110: cannot use proto (type "github.com/kubernetes-incubator/kompose/vendor/k8s.io/kubernetes/pkg/api".Protocol) as type "github.com/skippbox/kompose/vendor/k8s.io/kubernetes/pkg/api".Protocol in field value
# github.com/kubernetes-incubator/kompose/pkg/transformer/kubernetes
pkg/transformer/kubernetes/k8sutils.go:248: cannot use service (type "github.com/skippbox/kompose/pkg/kobject".ServiceConfig) as type "github.com/kubernetes-incubator/kompose/pkg/kobject".ServiceConfig in argument to transformer.ConfigAnnotations
pkg/transformer/kubernetes/k8sutils.go:274: cannot use service (type "github.com/skippbox/kompose/pkg/kobject".ServiceConfig) as type "github.com/kubernetes-incubator/kompose/pkg/kobject".ServiceConfig in argument to transformer.ConfigAnnotations
pkg/transformer/kubernetes/kubernetes.go:188: cannot use port.Protocol (type "github.com/skippbox/kompose/vendor/k8s.io/kubernetes/pkg/api".Protocol) as type "github.com/kubernetes-incubator/kompose/vendor/k8s.io/kubernetes/pkg/api".Protocol in field value
pkg/transformer/kubernetes/kubernetes.go:207: cannot use port.Protocol (type "github.com/skippbox/kompose/vendor/k8s.io/kubernetes/pkg/api".Protocol) as type "github.com/kubernetes-incubator/kompose/vendor/k8s.io/kubernetes/pkg/api".Protocol in field value
# github.com/skippbox/kompose/pkg/transformer/kubernetes
../../skippbox/kompose/pkg/transformer/kubernetes/k8sutils.go:248: cannot use service (type "github.com/kubernetes-incubator/kompose/pkg/kobject".ServiceConfig) as type "github.com/skippbox/kompose/pkg/kobject".ServiceConfig in argument to transformer.ConfigAnnotations
../../skippbox/kompose/pkg/transformer/kubernetes/k8sutils.go:274: cannot use service (type "github.com/kubernetes-incubator/kompose/pkg/kobject".ServiceConfig) as type "github.com/skippbox/kompose/pkg/kobject".ServiceConfig in argument to transformer.ConfigAnnotations
../../skippbox/kompose/pkg/transformer/kubernetes/kubernetes.go:188: cannot use port.Protocol (type "github.com/kubernetes-incubator/kompose/vendor/k8s.io/kubernetes/pkg/api".Protocol) as type "github.com/skippbox/kompose/vendor/k8s.io/kubernetes/pkg/api".Protocol in field value
../../skippbox/kompose/pkg/transformer/kubernetes/kubernetes.go:207: cannot use port.Protocol (type "github.com/kubernetes-incubator/kompose/vendor/k8s.io/kubernetes/pkg/api".Protocol) as type "github.com/skippbox/kompose/vendor/k8s.io/kubernetes/pkg/api".Protocol in field value

kadel · 2016-10-19T09:16:46Z

@surajssd this is because @ngtuna's branch is not yet rebased to current master. It still uses old import path github.com/skippbox/kompose

surajssd · 2016-10-19T09:50:16Z

oh okay

ngtuna · 2016-10-20T20:49:42Z

Hey @kadel @surajssd I made rebase and added test. Please take a look.

surajssd · 2016-10-21T04:22:13Z

@ngtuna I tried pulling and building it and it LGTM

kadel

LGTM

ngtuna · 2016-10-21T13:06:28Z

merging

ngtuna added the review needed label Oct 10, 2016

kadel approved these changes Oct 13, 2016

View reviewed changes

kadel requested changes Oct 13, 2016

View reviewed changes

kadel added the rebase needed label Oct 19, 2016

support both : and = as envvar separators

1fdaffe

ngtuna force-pushed the envvar_colon branch from ec3aa3d to eb1f350 Compare October 20, 2016 20:06

choose separator which happens first

df533a2

ngtuna force-pushed the envvar_colon branch from eb1f350 to df533a2 Compare October 20, 2016 20:13

add test

07f1635

ngtuna removed the rebase needed label Oct 20, 2016

kadel approved these changes Oct 21, 2016

View reviewed changes

kadel added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2016

ngtuna merged commit 019f90c into kubernetes:master Oct 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support both : and = as compose envvar separators #197

support both : and = as compose envvar separators #197

ngtuna commented Oct 10, 2016

ngtuna commented Oct 13, 2016

kadel left a comment

kadel Oct 13, 2016

ngtuna Oct 18, 2016

kadel Oct 13, 2016

ngtuna Oct 18, 2016

kadel left a comment

ngtuna commented Oct 18, 2016

surajssd commented Oct 19, 2016

kadel commented Oct 19, 2016

surajssd commented Oct 19, 2016

ngtuna commented Oct 20, 2016

surajssd commented Oct 21, 2016

kadel left a comment

ngtuna commented Oct 21, 2016

support both : and = as compose envvar separators #197

support both : and = as compose envvar separators #197

Conversation

ngtuna commented Oct 10, 2016

ngtuna commented Oct 13, 2016

kadel left a comment

Choose a reason for hiding this comment

kadel Oct 13, 2016

Choose a reason for hiding this comment

ngtuna Oct 18, 2016

Choose a reason for hiding this comment

kadel Oct 13, 2016

Choose a reason for hiding this comment

ngtuna Oct 18, 2016

Choose a reason for hiding this comment

kadel left a comment

Choose a reason for hiding this comment

ngtuna commented Oct 18, 2016

surajssd commented Oct 19, 2016

kadel commented Oct 19, 2016

surajssd commented Oct 19, 2016

ngtuna commented Oct 20, 2016

surajssd commented Oct 21, 2016

kadel left a comment

Choose a reason for hiding this comment

ngtuna commented Oct 21, 2016