Update `docker/cli` with upstream changes #244

vdemeester · 2018-06-25T14:58:45Z

This is still depending on a fork, but with way less differents
commits (several changes have been already upstreamed) and with an
up-to-date docker/cli.

One big change to notice is the enabled becomes
x-enabled (using extras field of the composefile format), and thus
does not require any change to the composefile struct.

Depends on docker/cli#1128
Includes docker/cli#1126 and docker/cli#1123

The only non-upstream(ed) commit yet is vdemeester/docker-cli@13760a1

Signed-off-by: Vincent Demeester [email protected]

codecov · 2018-06-25T15:10:44Z

Codecov Report

Merging #244 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #244   +/-   ##
=======================================
  Coverage   49.32%   49.32%           
=======================================
  Files          40       40           
  Lines        2966     2966           
=======================================
  Hits         1463     1463           
  Misses       1295     1295           
  Partials      208      208

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33a86eb...89348a1. Read the comment docs.

mat007

Some nits…

mat007 · 2018-06-25T15:22:00Z

internal/renderer/render.go

+	delimiter, delimiter, substitution, substitution,
+)
+
+var pattern = regexp.MustCompile(patternString)


nit:

var ( delimiter = "\\$" substitution = "[_a-z][._a-z0-9]*(?::?[-?][^}]*)?" pattern = regexp.MustCompile(delimiter + "(?i:(?P<escaped>" + delimiter + ")|(?P<named>" + substitution + ")|{(?P<braced>" + substitution + ")}|(?P<invalid>))") )

?

mat007 · 2018-06-25T15:23:07Z

internal/renderer/render.go

+			}
+		}
+		services = append(services, service)
+


nit: empty line

mat007 · 2018-06-25T15:24:26Z

internal/renderer/render.go

+	value, found := mapping(substitution)
+	if !found {
+		return "", true, &composetemplate.InvalidTemplateError{
+			Template: fmt.Sprintf("required variable %s is missing a value", substitution),


nit:

Template: "required variable " + substitution + " is missing a value",

silvin-lubecki · 2018-06-26T09:28:16Z

Gopkg.toml

@@ -1,25 +1,3 @@
-
-# Gopkg.toml example
-#


You should keep this part, I always need it when I update the vendoring 😅

silvin-lubecki · 2018-06-26T09:29:11Z

cmd/docker-app/render_test.go

+	"testing"
+)
+
+func TestRender(t *testing.T) {}


silvin-lubecki · 2018-06-26T09:45:24Z

internal/renderer/render.go

+	case e == "", e == "0", e == "false":
+		return false
+	case strings.HasPrefix(e, "!"):
+		return isEnabled(e[1:])


I think it should inverse the result no?

silvin-lubecki · 2018-06-26T14:09:33Z

internal/renderer/render.go

+	e = strings.ToLower(e)
+	switch {
+	case e == "", e == "0", e == "false":
+		return false


I don't like the case e == "", it means if someone uses x-enabled with empty value, it will be disabled:

node: x-enabled:

-> disabled
I think we should error out here. Enabling/Disabling a node should be explicit.

Linking to #261

silvin-lubecki · 2018-06-26T14:17:52Z

internal/renderer/render.go

+	return value, true, nil
+}
+
+func processEnabled(config *composetypes.Config) {


By the way I don't see any unit test on this "x-enabled" feature 😅

vdemeester · 2018-06-29T12:40:34Z

Gopkg.toml

@@ -30,14 +29,9 @@ required = ["github.com/wadey/gocovmerge"]
  name = "gopkg.in/yaml.v2"
  version = "2.2.1"

-[[override]]
+[[constraint]]


And it's now depending on docker/cli master 😛

silvin-lubecki · 2018-07-02T09:50:30Z

cmd/docker-app/deploy.go

-		Namespace: opts.deployNamespace,
-		Config:    opts.deployKubeConfig,
+	return stack.RunDeploy(dockerCli, flags, rendered, deployOrchestrator, options.Deploy{
+		Composefiles: []string{"-"},


Maybe add a // FIXME or // TODO so we can try to fix it in a follow up?

docker/cli#1172

silvin-lubecki · 2018-07-02T09:59:22Z

e2e/render/envvariables.dockerapp/expected.txt

@@ -6,3 +6,7 @@ services:
    - -c
    - cat bar bam
    image: alpine:latest
+networks: {}


silvin-lubecki · 2018-07-02T09:59:28Z

e2e/render/template-gotemplate.yml/expected.txt

@@ -10,3 +10,7 @@ services:
    image: alpine:latest
  other:
    image: nginx
+networks: {}


silvin-lubecki · 2018-07-02T09:59:34Z

e2e/render/template-mustache.dockerapp/expected.txt

@@ -10,3 +10,7 @@ services:
    image: alpine:latest
  other:
    image: nginx
+networks: {}


silvin-lubecki · 2018-07-02T10:06:19Z

internal/renderer/helm.go

@@ -273,34 +274,30 @@ func makeStack(appname string, targetDir string, data []byte, stackVersion strin
 	}
 	os.Mkdir(filepath.Join(targetDir, "templates"), 0755)
 	var stack interface{}
+	typeMeta := metav1.TypeMeta{


nit: Seams to be replicated in helmRender, it should be factorized 😄

silvin-lubecki · 2018-07-02T12:59:36Z

internal/renderer/helm.go

+		Name:      internal.AppNameFromDir(appname),
+		Namespace: "default", // FIXME
+	}
+


nit: empty line

silvin-lubecki · 2018-07-02T13:08:35Z

internal/renderer/helm.go

+func objectMeta(appname string) metav1.ObjectMeta {
+	return metav1.ObjectMeta{
+		Name:      internal.AppNameFromDir(appname),
+		Namespace: "default", // FIXME


I think it should be removed, as the "default" namespace is unlikely to be the user's default namespace.

silvin-lubecki

LGTM 🐒

This is still depending on a fork, but with way less differents commits (several changes have been already upstreamed) and with an up-to-date `docker/cli`. One big change to notice is the `enabled` becomes `x-enabled` (using *extras* field of the composefile format), and thus does not require any change to the composefile struct. Signed-off-by: Vincent Demeester <[email protected]>

silvin-lubecki · 2018-07-02T13:47:25Z

🎉

GordonTheTurtle added the status/0-triage label Jun 25, 2018

vdemeester added status/2-code-review and removed status/0-triage labels Jun 25, 2018

mat007 reviewed Jun 25, 2018

View reviewed changes

vdemeester force-pushed the rewrite-cli-integration branch from c391c36 to 9a5b7ad Compare June 26, 2018 10:52

silvin-lubecki suggested changes Jun 26, 2018

View reviewed changes

vdemeester force-pushed the rewrite-cli-integration branch 6 times, most recently from 647690a to 17036b6 Compare June 29, 2018 12:39

vdemeester commented Jun 29, 2018

View reviewed changes

vdemeester force-pushed the rewrite-cli-integration branch from 17036b6 to 7e0415a Compare June 29, 2018 12:43

vdemeester mentioned this pull request Jul 2, 2018

Update dependencies #265

Merged

mat007 approved these changes Jul 2, 2018

View reviewed changes

vdemeester force-pushed the rewrite-cli-integration branch from 7e0415a to afa27ab Compare July 2, 2018 10:01

silvin-lubecki reviewed Jul 2, 2018

View reviewed changes

vdemeester force-pushed the rewrite-cli-integration branch 3 times, most recently from 7d4a901 to 6de9091 Compare July 2, 2018 12:53

silvin-lubecki reviewed Jul 2, 2018

View reviewed changes

vdemeester force-pushed the rewrite-cli-integration branch 2 times, most recently from 7b1a982 to de108fd Compare July 2, 2018 13:12

silvin-lubecki approved these changes Jul 2, 2018

View reviewed changes

vdemeester force-pushed the rewrite-cli-integration branch from de108fd to 90bc232 Compare July 2, 2018 13:23

vdemeester force-pushed the rewrite-cli-integration branch from 90bc232 to 89348a1 Compare July 2, 2018 13:29

silvin-lubecki merged commit a558ab7 into docker:master Jul 2, 2018

vdemeester deleted the rewrite-cli-integration branch July 2, 2018 13:47

chris-crone mentioned this pull request Aug 21, 2018

Disabling a node in YAML should be explicit #261

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `docker/cli` with upstream changes #244

Update `docker/cli` with upstream changes #244

vdemeester commented Jun 25, 2018 •

edited

Loading

codecov bot commented Jun 25, 2018 •

edited

Loading

mat007 left a comment

mat007 Jun 25, 2018

mat007 Jun 25, 2018

mat007 Jun 25, 2018

silvin-lubecki Jun 26, 2018

silvin-lubecki Jun 26, 2018

silvin-lubecki Jun 26, 2018

silvin-lubecki Jun 26, 2018

silvin-lubecki Jun 29, 2018

silvin-lubecki Jun 26, 2018

vdemeester Jun 29, 2018

silvin-lubecki Jul 2, 2018

silvin-lubecki Jul 2, 2018

vdemeester Jul 2, 2018

silvin-lubecki Jul 2, 2018

silvin-lubecki Jul 2, 2018

silvin-lubecki Jul 2, 2018

silvin-lubecki Jul 2, 2018

silvin-lubecki Jul 2, 2018

silvin-lubecki Jul 2, 2018

silvin-lubecki left a comment

silvin-lubecki commented Jul 2, 2018

Update docker/cli with upstream changes #244

Update docker/cli with upstream changes #244

Conversation

vdemeester commented Jun 25, 2018 • edited Loading

codecov bot commented Jun 25, 2018 • edited Loading

Codecov Report

mat007 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

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

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

silvin-lubecki left a comment

Choose a reason for hiding this comment

silvin-lubecki commented Jul 2, 2018

Update `docker/cli` with upstream changes #244

Update `docker/cli` with upstream changes #244

vdemeester commented Jun 25, 2018 •

edited

Loading

codecov bot commented Jun 25, 2018 •

edited

Loading