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

Workaround mergo merging panic for volumes #106

Merged
merged 1 commit into from Mar 5, 2021
Merged

Conversation

ulyssessouza
Copy link
Contributor

@ulyssessouza ulyssessouza commented Feb 17, 2021

This should fix docker/compose-cli#1261 and docker/cli#2916

The process should probably be applied to other maps, like networks, secrets, configs and extensions

@ulyssessouza ulyssessouza changed the title Workaround mergo merging Workaround mergo merging panic for volumes Feb 17, 2021
@ulyssessouza ulyssessouza force-pushed the fix-panic-merge branch 3 times, most recently from 64250b0 to 5427c29 Compare February 17, 2021 20:37
@ndeloof
Copy link
Collaborator

ndeloof commented Feb 17, 2021

If we adopt the "pointer" approach, then we need to do the same on all map[]foo types

@ulyssessouza
Copy link
Contributor Author

@ndeloof You are right!
This is just a "Draft" PR to check if there is something missing before I do the rest of the types

@ndeloof
Copy link
Collaborator

ndeloof commented Feb 17, 2021

Reading darccio/mergo#90 (comment) it seems this would not be the sivler bullet you expected: non-empty source would be fully replaced by override, not merged.

@ndeloof
Copy link
Collaborator

ndeloof commented Feb 18, 2021

I ran my own debug session investigating docker/compose-cli#1261 ...
surprisingly, mapstruct is able to set a map field (see https://github.com/mitchellh/mapstructure/blob/master/mapstructure.go#L846) while mergo fails doing the same https://github.com/imdario/mergo/blob/cd55aeac72a19c6d5826061d5125d72b252a12eb/merge.go#L104
but that's all voodoo to me (ㆆ _ ㆆ)

I noticed (trying to get a minimal test set) that issue happens as compose override file defines driver_opts while the source has this map initial value set to nil
i.e adding an empty map in the yaml doc fix this issue:

volumes:
  data:
    driver_opts: {}

so a possible fix, without digging into mergo internal reflect voodoo is to enforce all maps to be set non-nil.
mapstructure's ZeroFields could help us here, but unfortunately this only applies to keys in source, and other fields are skipped (https://github.com/mitchellh/mapstructure/blob/master/mapstructure.go#L1350-L1353)
Need more investigations :P

Copy link
Collaborator

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

tested with

volumes:
  data:
  • override:
volumes:
  data:
    driver: azure_file
    driver_opts:
      share_name: test-share
      storage_account_name: my-storage-account

as reported in initial issue

=> panic persists

by my own investigation, root cause is DriverOpts is a nil map and mergo fails to set a value.
Can't find a satisfying fix so far.

@ulyssessouza
Copy link
Contributor Author

@ndeloof Just waiting on merge of darccio/mergo#177 to bump mergo to fix the nil issue

@ndeloof
Copy link
Collaborator

ndeloof commented Mar 4, 2021

@ulyssessouza darccio/mergo#177 has been merged

@ulyssessouza
Copy link
Contributor Author

@ndeloof Yep! And I've just bumped mergo in this PR too

@ndeloof
Copy link
Collaborator

ndeloof commented Mar 4, 2021

ack, but with the fix you should not need to use *types.VolumeConfig anymore

@ulyssessouza ulyssessouza force-pushed the fix-panic-merge branch 2 times, most recently from 81cf330 to ab9a86c Compare March 5, 2021 03:35
Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
@ndeloof ndeloof merged commit 876e630 into master Mar 5, 2021
@ndeloof ndeloof deleted the fix-panic-merge branch March 5, 2021 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker compose panics with volume override
2 participants