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

Provide feature flag for overriding config.*.yaml instead of merging #4100

Closed
1 task done
miromichalicka opened this issue Aug 9, 2022 · 9 comments
Closed
1 task done

Comments

@miromichalicka
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe

In our setup, we're overriding hooks with a limited set when we're running certain operations - e.g. we're not installing drupal. Since #3983 changes behaviour, we're no longer able to do it.

Describe the solution you'd like

I would like to set up a (project) flag in config.*.yaml file that would indicate if I want to merge multiple config files or if those should be overriden.

Describe alternatives you've considered

One alternative would be to use yq or similar scripting solution, requiring additional tooling on the server and user machines.

Additional context

No response

@miromichalicka
Copy link
Contributor Author

Relates to #4079 and #4099

@mkalkbrenner
Copy link

mkalkbrenner commented Aug 9, 2022

I would really appreciate this feature!

Let me describe our current issue. We run a drupal application that uses language negotiation based on the hostname.
Therefore we use additional_hostnames in config.yaml.

A big advantage of DDEV was that you could run different feature branches of a project in parallel. Just do another checkout and provide a config.local.yaml like this:

name: feature-4711
additional_hostnames: []

or

name: feature-123
additional_hostnames:
  - en.feature-123
  - de.feature-123
  - fr.feature-123

If you now start such a feature branch you break the ddev-router because the additional hostnames from config.yaml get applied and lead to dupplicates and errors.

@rfay
Copy link
Member

rfay commented Aug 9, 2022

This seems like a great approach! Thanks for the good thinking.

@rfay
Copy link
Member

rfay commented Aug 13, 2022

I'd love it if you could all review and test the PR for this, #4118 (comment)

@HeineDeelstra
Copy link

I did not yet test the pull request, but the per file override is really nifty.

Do I understand correctly that in order to override some settings set to a non-nil value in an "earlier" config.yaml one has to add override_config: true to the file in order to eg disable mutagen? And that this would also be asymmetrical because one could use a merge (override_config: false) to override nil values from such an earlier config.yaml?

@miromichalicka
Copy link
Contributor Author

@oliverhabara can you please test this on our use case? Thanks

@rfay
Copy link
Member

rfay commented Aug 15, 2022

@HeineDeelstra overriding any non-nil value would require a config.*.yaml with override_config: true in it.

override_config: false is the default, and is essentially irrelevant. override_config: true applies only to the config.*.yaml in which it appears.

I hope that answers your question.

I pretty much hope that people don't put mutagen_enabled in any config.yaml or config.*.yaml, but rather control it with global config, because it's really not something a project would ever config.

@HeineDeelstra
Copy link

I find the requirement for override_config: true to set scalar config to nil, but not to a non-nil value rather surprising.

I would naively expect the default merge to:

  • override any scalar to any later value, even if this later value is a nil value
  • combine arrays ['a', 'b', 'c'] + ['x', 'y', 'z'] -> ['a', 'b', 'c', 'x', 'y', 'z']
  • object members with non identical names to be merged {one:1} + {two: 2} -> {one:1, two:2}
  • object members with identical names to be merged for arrays or object {one:[1]} + {one:[2]} -> {one:[1,2]}
  • object members with identical names to be overwritten for scalars: {one: 1} + {one:2} -> {one:2}

@rfay
Copy link
Member

rfay commented Aug 16, 2022

@HeineDeelstra I understand why you would expect the default merge (v1.20.0) would not be what it is, but this is discussed in #4079

  • The default merge in v1.20.0 does in fact override any scalar to the later value, but not if the later value is nil. The reason it can't do nil is that all the values in a struct that don't have non-nil values are nil. Therefore, when you merge, if you overwrote scalar values with nil values (by default) then lots of things you didn't want to be nil are suddenly nil. The problem is that we're merging structs. They're not maps or some other structure. It might be possible to write code for each member of the struct that behaves the way you hope, but it would be impossible to maintain.
  • The default merge does in fact combine arrays ['a', 'b', 'c'] + ['x', 'y', 'z'] -> ['a', 'b', 'c', 'x', 'y', 'z']; but with override_config set, it would be ['x', 'y', 'z'].
  • The default merge in v1.20 does what you suggest, object members with non identical names to be merged {one:1} + {two: 2} -> {one:1, two:2}
  • The default merge in v1.20.0 does what you suggest, object members with identical names to be merged for arrays or object {one:[1]} + {one:[2]} -> {one:[1,2]}
  • I'm not sure about object members with identical names to be overwritten for scalars: {one: 1} + {one:2} -> {one:2}; those may end up being duplicates and fail.

You can see examples of expectations behavior in https://github.com/drud/ddev/tree/master/pkg/ddevapp/testdata/TestConfigMerge

This PR adds a "fulloverride" section which shows the behavior with override_config.

I do hope you'll try out the PR (you don't mention that you tried it).

You can easily experiment with various values and files; just ddev debug configyaml will show you results of the merge in all cases; ddev debug configyaml | yq is a little easier to read.

@rfay rfay closed this as completed in 2972c6a Aug 17, 2022
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

No branches or pull requests

4 participants