Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error with merge composefile with networks… #983

Merged
merged 1 commit into from
Apr 5, 2018

Conversation

vdemeester
Copy link
Collaborator

@vdemeester vdemeester commented Apr 3, 2018

… and other cases too. Updating mergo fixes the bugs (but introduced a
slight behaviour change that had to be fixed too)

Updates mergo : darccio/mergo@ea74e01...cfcc6d7

Fixes #972

Needs darccio/mergo#79 to get in upstream 👼

cc @dnephin

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

@silvin-lubecki
Copy link
Contributor

@vdemeester

  • linter is 😠 cli/compose/loader/merge_test.go:1::warning: file is not gofmted with -s (gofmt)
  • so is the vendoring
scripts/validate/check-git-diff vendor

These files were changed:

 M vendor/github.com/imdario/mergo/merge.go

… and other cases too. Updating mergo fixes the bugs (but introduced a
slight behaviour change that had to be fixed too)

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@vdemeester
Copy link
Collaborator Author

Upstream got merge (thx @imdario 😻), so this is now ready for review 😉

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@MetalArend
Copy link

@thaJeztah Sweet. The same issue is present when defining volumes, is this solving that as well?

@thaJeztah
Copy link
Member

@MetalArend volumes should also be addressed by this change, but perhaps if you have a minimal example to reproduce/test, that would be appreciated

@MetalArend
Copy link

Small example:

docker-compose.yml

version: '3.4'
volumes:
  app:
services:
  bash:
    image: bash
    command: cat docker-compose.override.yml
    volumes:
      - app:/app
    working_dir: /app

docker-compose.override.yml

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

docker stack deploy --compose-file docker-compose.yml --compose-file docker-compose.override.yml test

@LaurentTrk
Copy link

@thaJeztah Sweet. The same issue is present when defining volumes, is this solving that as well?

Hi @MetalArend , is the issue solved from your side ? Having the same issue with volume merging on 18.09.7...
Thansk for your help

@thaJeztah
Copy link
Member

@LaurentTrk feel free to open a new issue with steps to reproduce if it's not fixed (or if there's a regression)

@LaurentTrk
Copy link

Thanks @thaJeztah , here it is : #1981

@thaJeztah
Copy link
Member

Thanks! I must admit that I don't recall if I tested @MetalArend's example at the time #983 (comment), so also possible it wasn't fixed for Volumes with this PR; having a new issue helps preventing it getting lost 🤗

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reflect.Value.Set using unaddressable value
6 participants