Add support for multiple composefile when deploying #569

vdemeester · 2017-09-29T10:13:46Z

Using a custom version of mergo to manage special cases.

Fix vendor (right now it's a locally modified version of mergo)
~~Handling docker-compose.override.yml per default~~
More tests on edge cases compared to docker-compose

Should fix moby/moby#30127

/cc @shin- too for edge case in the merge that I would have missed

as of now, it is supposed to be red (i.e. vendor should not validate)

Signed-off-by: Vincent Demeester vincent@sbr.pm

ksouf · 2017-09-29T12:31:05Z

cli/command/stack/deploy.go

@@ -63,9 +63,9 @@ func runDeploy(dockerCli command.Cli, opts deployOptions) error {
 	}

 	switch {
-	case opts.bundlefile == "" && opts.composefile == "":
+	case opts.bundlefile == "" && len(opts.composefile) == 0:


i think we should keep the symmetry :)
i mean on checking the emptiness of a string always using len function instead of '== "" '

I really dislike checking empty string using len(string) == 0. Sure it works, but it's a lot less obvious when reading the code. I think most people say "empty string", not "a string of zero bytes", so variable == "" matches this phrase.

I'm in favour of keeping it as it is now.

ksouf · 2017-09-29T12:31:25Z

cli/command/stack/deploy.go

 		return errors.Errorf("Please specify either a bundle file (with --bundle-file) or a Compose file (with --compose-file).")
-	case opts.bundlefile != "" && opts.composefile != "":
+	case opts.bundlefile != "" && len(opts.composefile) != 0:


same thing :)

ksouf · 2017-09-29T12:34:49Z

cli/compose/loader/loader_multiple_test.go

+	_, err := Load(configDetails)
+	require.Error(t, err)
+}
+*/


i think we should remove commented code :)

@ksouf oh right, I kept this one to discuss it with @dnephin and @shin- if we do support merging 2 different version on docker-compose or not.

@vdemeester all right :)

oh but it doesn't really make sense in the PR because it's not the one implementing parsing of v1/v2 compose files 😅

It is not supported, it's an error to combine different versions.

ksouf · 2017-09-29T12:35:37Z

cli/compose/loader/loader_multiple_test.go

+		"networks": map[string]interface{}{},
+		"secrets":  map[string]interface{}{},
+		"configs":  map[string]interface{}{},
+	}


mmmm can we extract something ? :)

I'm going to write a builder for that once it's in code-review yes 😉

ksouf · 2017-09-29T12:37:11Z

cli/compose/loader/loader_multiple_test.go

+		"networks": map[string]interface{}{},
+		"secrets":  map[string]interface{}{},
+		"configs":  map[string]interface{}{},
+	}


same here i think we can extract it

silvin-lubecki

LGTM

silvin-lubecki · 2017-10-02T09:10:39Z

cli/command/stack/deploy.go

@@ -22,7 +22,7 @@ const (

 type deployOptions struct {
 	bundlefile       string
-	composefile      string
+	composefile      []string


Rename to composefiles ?

silvin-lubecki · 2017-10-02T09:15:34Z

cli/compose/loader/loader.go

+
+		if services, ok := configDict["services"]; ok {
+			if servicesDict, ok := services.(map[string]interface{}); ok {
+				forbidden := getProperties(servicesDict, types.ForbiddenProperties)


if forbidden := getProperties(servicesDict, types.ForbiddenProperties); len(forbidden) > 0{ return nil, &ForbiddenPropertiesError{Properties: forbidden} }

👍 yes, I think that'd be cleaner

silvin-lubecki · 2017-10-02T09:17:09Z

cli/compose/loader/loader.go

+			return nil, err
+		}
+
+		cfg.Configs, err = LoadConfigObjs(config["configs"], configDetails.WorkingDir)


Why don't you check err here?

Oups, I forgot 😓

silvin-lubecki · 2017-10-02T09:21:14Z

cli/compose/loader/merge.go

+}
+
+func (s *specials) Transformer(t reflect.Type) func(dst, src reflect.Value) error {
+	if fn, ok := s.m[t]; ok {


Why don't you just return s.m[t]?

func (s *specials) Transformer(t reflect.Type) func(dst, src reflect.Value) error { return s.m[t] }

Isn't it equivalent?

It would be 👍

silvin-lubecki · 2017-10-02T09:24:01Z

cli/compose/loader/merge.go

+}
+
+func merge(configs []*types.Config) (*types.Config, error) {
+	base := configs[0]


Check configs length before access to the first element?

At this point, it's guaranteed that you have a least one config (and as it's not exported.. I think we can "not check" but open to change that)

codecov-io · 2017-10-02T13:06:57Z

Codecov Report

Merging #569 into master will decrease coverage by 1%.
The diff coverage is 73.83%.

@@            Coverage Diff             @@
##           master     #569      +/-   ##
==========================================
- Coverage   52.95%   51.95%   -1.01%     
==========================================
  Files         244      245       +1     
  Lines       15839    15996     +157     
==========================================
- Hits         8387     8310      -77     
- Misses       6898     7125     +227     
- Partials      554      561       +7

dnephin · 2017-10-02T20:02:28Z

Hmm, jenkins build didn't trigger (or didn't update the github PR) for some reason. I've seen that happen occasionally.

Handling docker-compose.override.yml per default

I don't think we should read this by default. This is similar to the .env file support, which is specific to the tool docker-compose not the format (which is shared).

More tests on edge cases compared to docker-compose

Haven't looked at existing tests yet, but something that we need to make sure is tested is the case where we don't actually merge structs but have one struct override the other (ex: build).

dnephin

Design LGTM

Finding all the necessary test cases will be difficult. There are a ton in docker-compose. If we can find all the places that don't fully merge structs that would be a good thing to test.

thaJeztah · 2017-10-30T17:34:24Z

Design LGTM as well; want to give this a bit of a try; ideally we'd also have a way to print the merged stack before deploying (similar to docker-compose config, could be, e.g. docker stack print -c file -c file -c file, so that I can check what will be deployed and if the merged results are what I expect them to be.

dnephin · 2017-11-10T16:27:40Z

cli/compose/loader/loader.go

-	}
+	for _, file := range configDetails.ConfigFiles {
+		configDict := file.Config
+		configDetails.Version = schema.Version(configDict)


I think this should only set the version from the first config, and error if any other config does not match that version.

dnephin · 2017-11-10T16:34:04Z

cli/compose/loader/merge.go

+	for name, overrideService := range overrideServices {
+		if baseService, ok := baseServices[name]; ok {
+			if err := mergo.Merge(&baseService, &overrideService, mergo.WithOverride, mergo.WithTransformers(specials)); err != nil {
+				// if err := mergo.MergeWithOverwriteAndTransform(&baseService, &overrideService, specials); err != nil {


commented code

dnephin · 2017-11-10T16:39:43Z

cli/compose/loader/merge.go

+		// merge services
+		base.Services, err = mergeServices(base.Services, override.Services)
+		if err != nil {
+			return base, errors.Wrap(err, "cannot merge services")


Errors here might be different to debug when there are many files because there is no mention of which file failed to merge. Maybe types.Config needs a Filename field so that it can be included here?

dnephin · 2017-11-10T16:40:59Z

cli/compose/loader/merge.go

+		if baseService, ok := baseServices[name]; ok {
+			if err := mergo.Merge(&baseService, &overrideService, mergo.WithOverride, mergo.WithTransformers(specials)); err != nil {
+				// if err := mergo.MergeWithOverwriteAndTransform(&baseService, &overrideService, specials); err != nil {
+				return base, errors.Wrap(err, "cannot merge compose file")


This message seems more appropriate as a wrap around the error returned by merge(). At this level I think the detail that needs to be included is the service name.

dnephin · 2017-11-10T16:51:59Z

cli/compose/loader/merge.go

+	overrideServices := mapByName(override)
+	specials := &specials{
+		m: map[reflect.Type]func(dst, src reflect.Value) error{
+			reflect.TypeOf(types.BuildConfig{}): func(dst, src reflect.Value) error {


I thought that build was a non-recursive merge, but looking at the docker-compose code again it seems like maybe it does recursive?

logging looks like a special case, where it changes if the drier changes. I think driver options are a case where they don't get merged but overridden. I don't have a full list but I think I remember there being a bunch of them.

hum 🤔 what should we do here ? 😛
logging is a special case but build isn't ?
cc @shin-

Regarding logging, the idea is that options are driver-specific, thus an override of the driver resets the options dictionary. Unless the base driver is null or unset, in which case it's assumed the user is actively attempting to use inheritance, and base options are merged with the override. Same thing if the base has specified a driver but not the override.

These tests codify the behavior on the docker-compose side: https://github.com/docker/compose/blob/master/tests/unit/config/config_test.py#L1929-L2127

I'm not sure what the question is about build?

darccio · 2018-01-04T10:51:48Z

Sorry, I didn't see this before. I'll check it by this weekend.

vdemeester · 2018-01-15T15:00:00Z

Rebase and updated.. I need to take your comments into account @dnephin 😛

thaJeztah · 2018-01-23T02:11:16Z

Some testing (will post more);

file1.yml:

version: "3.5"
services:
  web:
    image: nginx:alpine
    ports:
      - "8080:80"

file2.yml:

version: "3.5"
services:
  web:
    image: nginx:alpine
    ports:
      - target: 80
        published: 8080

Updating service multi_web (id: l4irmgy0znyncwv1t2ehtbgbd)
failed to update service multi_web: Error response from daemon: rpc error: code = InvalidArgument desc = EndpointSpec: duplicate published ports provided

Should the client here detect the duplicate definition? This is what's sent by the CLI:

  "EndpointSpec": {
    "Ports": [
      {
        "PublishedPort": 8080,
        "TargetPort": 80
      },
      {
        "PublishedPort": 8080,
        "TargetPort": 80
      }
    ]
  },

vdemeester · 2018-01-29T18:35:59Z

@dnephin @thaJeztah should be ready for another round of review 👼

dnephin

LGTM

dnephin · 2018-01-30T00:44:45Z

cli/compose/loader/merge.go

+func sliceToMap(tomap tomapFn, v reflect.Value) (map[interface{}]interface{}, error) {
+	// check if valid
+	if !v.IsValid() {
+		return nil, errors.Errorf("invalid..") // FIXME


🤦‍♂️

Signed-off-by: Vincent Demeester <vincent@sbr.pm>

thaJeztah

LGTM 🎉

thaJeztah · 2018-02-06T08:41:18Z

As a follow up, we'll need;

updates to the completion scripts(?)
updates for the CLI reference
possibly other parts of the docs?

vlyubin · 2018-03-16T00:33:16Z

@thaJeztah so what exactly is the CLI syntax? Just docker stack deploy blah -c file1.yml -c file2.yml?

Thanks!

Nevermind, just saw the milestone! I don't have it yet.

MetalArend · 2018-09-28T10:45:09Z

Thanks for implementing this, so glad this finally got to swarm.

I have an edge case that keeps throwign an error. The two files are:

file A:

version: '3.7'
volumes:
  app:
    driver: local
    driver_opts:
      o: bind
      type: none
      device: $PWD

file B:

version: '3.7'
volumes:
  app:

If file A is overwritten by file B, everything is fine, but if file B gets overwritten by file A, the following error gets thrown:

panic: reflect: reflect.Value.Set using unaddressable value

goroutine 1 [running]:
reflect.flag.mustBeAssignable(0x95)
        /usr/local/go/src/reflect/value.go:234 +0x15c
reflect.Value.Set(0x51b9aa0, 0xc420a449d0, 0x95, 0x51b9aa0, 0xc4206b7b30, 0x15)
        /usr/local/go/src/reflect/value.go:1367 +0x2f
github.com/docker/cli/vendor/github.com/imdario/mergo.deepMerge(0x51b9aa0, 0xc420a449d0, 0x95, 0x51b9aa0, 0xc420a44980, 0x95, 0xc420a1f5b0, 0x2, 0xc420568920, 0x0, ...)
        /go/src/github.com/docker/cli/vendor/github.com/imdario/mergo/merge.go:82 +0xcc0
github.com/docker/cli/vendor/github.com/imdario/mergo.deepMerge(0x52f6540, 0xc420a449b0, 0x99, 0x52f6540, 0xc420a44960, 0x99, 0xc420a1f5b0, 0x1, 0xc420568920, 0x0, ...)
        /go/src/github.com/docker/cli/vendor/github.com/imdario/mergo/merge.go:71 +0x1836
github.com/docker/cli/vendor/github.com/imdario/mergo.deepMerge(0x51b7b20, 0xc420374198, 0x195, 0x51b7b20, 0xc4203741a0, 0x195, 0xc420a1f5b0, 0x0, 0xc420568920, 0x195, ...)
        /go/src/github.com/docker/cli/vendor/github.com/imdario/mergo/merge.go:114 +0x6ec
github.com/docker/cli/vendor/github.com/imdario/mergo._map(0x50f0520, 0xc420374198, 0x50f0520, 0xc4203741a0, 0xc420a1f7b0, 0x1, 0x1, 0x1, 0xc4203741a0)
        /go/src/github.com/docker/cli/vendor/github.com/imdario/mergo/map.go:159 +0x38b
github.com/docker/cli/vendor/github.com/imdario/mergo.Map(0x50f0520, 0xc420374198, 0x50f0520, 0xc4203741a0, 0xc420a1f7b0, 0x1, 0x1, 0x1, 0x0)
        /go/src/github.com/docker/cli/vendor/github.com/imdario/mergo/map.go:132 +0x71
github.com/docker/cli/cli/compose/loader.mergeVolumes(0xc420606690, 0xc4206b7410, 0x8, 0xc420063400, 0x5)
        /go/src/github.com/docker/cli/cli/compose/loader/merge.go:216 +0xd1
github.com/docker/cli/cli/compose/loader.merge(0xc4205f8250, 0x2, 0x2, 0x1, 0x2, 0xc4205f8250)
        /go/src/github.com/docker/cli/cli/compose/loader/merge.go:31 +0x14d
github.com/docker/cli/cli/compose/loader.Load(0xc420601f68, 0x3, 0xc42035cbc0, 0x32, 0xc420552cc0, 0x2, 0x2, 0xc420552cf0, 0x0, 0x0, ...)
        /go/src/github.com/docker/cli/cli/compose/loader/loader.go:111 +0x65f
github.com/docker/cli/cli/command/stack/loader.LoadComposefile(0x5587760, 0xc420167ce0, 0x0, 0x0, 0xc42025b4e0, 0x2, 0x2, 0x7fff5fbffc29, 0x3, 0x53c9fbd, ...)
        /go/src/github.com/docker/cli/cli/command/stack/loader/loader.go:28 +0x18c
github.com/docker/cli/cli/command/stack.newDeployCommand.func1(0xc420566c80, 0xc420369c20, 0x1, 0x6, 0x0, 0x0)
        /go/src/github.com/docker/cli/cli/command/stack/deploy.go:49 +0x23b
github.com/docker/cli/vendor/github.com/spf13/cobra.(*Command).execute(0xc420566c80, 0xc42003a0c0, 0x6, 0x6, 0xc420566c80, 0xc42003a0c0)
        /go/src/github.com/docker/cli/vendor/github.com/spf13/cobra/command.go:762 +0x468
github.com/docker/cli/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc420392f00, 0xc420294330, 0x51389e0, 0xc420294340)
        /go/src/github.com/docker/cli/vendor/github.com/spf13/cobra/command.go:852 +0x30a
github.com/docker/cli/vendor/github.com/spf13/cobra.(*Command).Execute(0xc420392f00, 0xc420392f00, 0x554c4a0)
        /go/src/github.com/docker/cli/vendor/github.com/spf13/cobra/command.go:800 +0x2b
main.main()
        /go/src/github.com/docker/cli/cmd/docker/docker.go:174 +0xd0

vdemeester added status/1-design-review status/needs-vendoring labels Sep 29, 2017

vdemeester requested review from dnephin, cpuguy83 and thaJeztah September 29, 2017 10:13

GordonTheTurtle added the status/0-triage label Sep 29, 2017

vdemeester removed the status/0-triage label Sep 29, 2017

vdemeester force-pushed the compose-multiple-version-mergo branch from 2455685 to 7ff744d Compare September 29, 2017 12:21

ksouf reviewed Sep 29, 2017

View reviewed changes

vdemeester added the area/stack label Sep 29, 2017

thaJeztah mentioned this pull request Sep 30, 2017

Deploy to swarm from multiple compose files moby/moby#30127

Closed

silvin-lubecki reviewed Oct 2, 2017

View reviewed changes

vdemeester force-pushed the compose-multiple-version-mergo branch from 7ff744d to 03c9cdd Compare October 2, 2017 13:06

dnephin reviewed Oct 2, 2017

View reviewed changes

dnephin mentioned this pull request Oct 4, 2017

[Compose] Cast values to expected type after interpolating values #601

Merged

thaJeztah added status/2-code-review and removed status/1-design-review labels Oct 30, 2017

vdemeester force-pushed the compose-multiple-version-mergo branch from 03c9cdd to 6869e3c Compare October 31, 2017 11:03

vdemeester force-pushed the compose-multiple-version-mergo branch 3 times, most recently from 4809114 to a34dd26 Compare November 8, 2017 13:05

vdemeester mentioned this pull request Nov 9, 2017

Add support for "custom" transformer/merges darccio/mergo#49

Merged

dnephin removed the status/needs-vendoring label Nov 10, 2017

dnephin reviewed Nov 10, 2017

View reviewed changes

vdemeester force-pushed the compose-multiple-version-mergo branch from a34dd26 to bcc50a6 Compare January 15, 2018 14:58

shin- mentioned this pull request Jan 22, 2018

Advanced merge for deploy dict in v3 files docker/compose#5595

Merged

thaJeztah added impact/changelog impact/documentation impact/compose-file labels Jan 23, 2018

vdemeester force-pushed the compose-multiple-version-mergo branch 6 times, most recently from c76178c to 8a7db05 Compare January 26, 2018 01:35

vdemeester mentioned this pull request Jan 29, 2018

[compose] Share the compose loading code between swarm and k8s stack deploy #845

Merged

2 tasks

dnephin approved these changes Jan 30, 2018

View reviewed changes

Add support for multiple composefile when deploying

1872bd8

Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester force-pushed the compose-multiple-version-mergo branch from 8a7db05 to 1872bd8 Compare January 30, 2018 01:27

thaJeztah approved these changes Feb 6, 2018

View reviewed changes

thaJeztah added the docs/revisit label Feb 6, 2018

thaJeztah merged commit 25e969c into docker:master Feb 6, 2018

GordonTheTurtle added this to the 18.03.0 milestone Feb 6, 2018

vdemeester deleted the compose-multiple-version-mergo branch February 6, 2018 08:43

vdemeester mentioned this pull request Feb 13, 2018

Add support for extends feature in Compose v3 / docker stack deploy moby/moby#31101

Open

This was referenced Mar 6, 2018

extends not supported in version 3 schema docker/compose#4315

Closed

docker-compose config incorrectly re-formats volume source on Windows hosts. docker/compose#5763

Closed

gbarr01 mentioned this pull request Mar 12, 2018

To doc: Add support for multiple composefile when deploying docker/docs#6200

Closed

MadDaelim mentioned this pull request Aug 30, 2018

New module: add docker_stack module (cloud/docker/docker_stack) ansible/ansible#24588

Merged

thaJeztah mentioned this pull request Jan 31, 2023

docker stack deploy command doesn't support multiple docker-compose.yml #1651

Closed

Add support for multiple composefile when deploying #569

Add support for multiple composefile when deploying #569

Conversation

vdemeester commented Sep 29, 2017 • edited

Choose a reason for hiding this comment

dnephin Oct 2, 2017 • edited

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

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

codecov-io commented Oct 2, 2017 • edited

Codecov Report

dnephin commented Oct 2, 2017

dnephin left a comment

Choose a reason for hiding this comment

thaJeztah commented Oct 30, 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

darccio commented Jan 4, 2018

vdemeester commented Jan 15, 2018

thaJeztah commented Jan 23, 2018 • edited

vdemeester commented Jan 29, 2018

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Feb 6, 2018

vlyubin commented Mar 16, 2018 • edited

MetalArend commented Sep 28, 2018

vdemeester commented Sep 29, 2017 •

edited

dnephin Oct 2, 2017 •

edited

codecov-io commented Oct 2, 2017 •

edited

thaJeztah commented Jan 23, 2018 •

edited

vlyubin commented Mar 16, 2018 •

edited