override_options broken since refactoring

[liwo]

According to README, one can override single options in my.cnf like this:

class { '::mysql::globals':
  override_options => { 'mysqld' => { 'max_connections' => '1024' } }
}

Unfortunately, this does not work as expected; merge() is not recursive.

[jburnham]

Yea, it seems stdlib doesn't have/support recursive hash merging. I had to switch to our own function that does do this. I'm wondering if upstream stdlib will want a function (or fix merge to do recursive) that does this (seems like they should since it's a valid thing to do).

[apenney]

If this isn't recursive then this has probably tanked my entire theory behind the rewrite. 👎

I think we would accept a recursive hash merging function. I'd accept a recursive hash merging function and I can "make that happen" by harrassing the maintainer of stdlib. Failing that, you willing to share yours and I'll just add it to this module specifically in the meantime.

In the past I had to force deep_merge to be installed by that's not exactly appropriate for something like this so if you have something functional that saves me reinventing the wheel I'd appreciate it. :)

If that doesn't work out then I will back out this to what's in the 1.x branch and rethink from scratch.

[jburnham]

Do we want to replace merge and make it recursive or should it be a new function like rmerge (for recursive merge)?

[apenney]

While I would prefer to replace merge() with a recursive one I think that might be surprising behavior so we should start by adding rmerge() I think.

[jburnham]

My thoughts as well, working on a pr to stdlib.

[jburnham]

@apenney Would it be prudent to go ahead with adding a separate 'private' deep_merge equivalent function here so we can proceed with fixing this module? Since stdlib is a pretty core module I'm sure there will be some delay in a release that has this new function and I would imagine deep_merge should be backported to versions compatible to puppet 2.7.x (which we use still). If you agree, what would you call such a temporary function?

[apenney]

Yeah, i'm definitely OK with that. How about we just give it the terrible mysql_deepmerge() or something for now, making it clear what it does and that it's mysql specific. If you're OK with that I guess I'm asking you to fire in a PR to add that function if you're OK with doing so.

[apenney]

Closing for now as we'll be shipping 2.x with mysql_deepmerge() and not waiting on stdlib.