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: don't add options to my.cnf when they are set to false #339

Closed
wants to merge 1 commit into from
Closed

fix: don't add options to my.cnf when they are set to false #339

wants to merge 1 commit into from

Conversation

spielkind
Copy link

See issue: 338

@igalic
Copy link
Contributor

igalic commented Oct 24, 2013

not sure I entirely agree with this, especially when someone explicitly sets an option to false, they may want it to be explicitly, visibly in the file.

But perhaps I'm not seeing the whole picture here.

@spielkind
Copy link
Author

Even now it is imho not possible to set an value explicitly to false:

value => false

will just result in

value

@apenney
Copy link
Contributor

apenney commented Oct 24, 2013

I wish I could come up with a cleaner data structure or way to build my.cnf that:

a/ Sets sensible defaults.
b/ Lets you override those defaults.
c/ Lets you add arbitrary new entries.
d/ Lets you remove defaults.
e/ Doesn't need a parameter per item.
f/ Cleanly handles allowing statements to be key => false as well as just key.

I feel like we're trapped to a degree as this is really hard to model in Puppet and meet all those requirements. Right now we're failing on d/ and f/. For d/ I considered adding a disable_defaults => true/false that would mean you had to put the entire my.cnf into overrides for "advanced cases". That doesn't help with f/ and the best I can come up with there is either 'UNSET' as a way to leave them as just value, or maybe nil.

How about if this weekend I do some playing around with scrapping the hash, writing a define to set an entry via puppetlabs-inifile (if that works for this use case) or writing an augeas provider, and just writing a whole bunch of default entries in. These can be easily overridden by setting the contents to undef, and new ones can be added in fairly easy.

We can map the existing override_options right into calls to the define so we don't shake up the entire API again, and then additional entries can be added pretty easily. This does mean that the majority of users will need a "wrapper class" around mysql to add configuration, but maybe that's an improvement from trying to funnel everything through a single parameter.

@apenney
Copy link
Contributor

apenney commented Nov 4, 2013

This has been delayed but I'll try to get to this prototype PR today!

@spielkind
Copy link
Author

Any news on the prototype? :)

@apenney
Copy link
Contributor

apenney commented Nov 11, 2013

I've been playing with https://github.com/richardc/puppet-datacat as a way to build this up, it's kind of like concat. So far I've still ran into the problem that it's hard to remove things without hand building a fragment per entry in the $options hash. I've been talking with the author about how he uses it in puppetlabs-mcollective but they have the luxury of assuming '' is unset. I'm now debating between using 'UNSET' or trying to figure out better alternatives (as I either need to use unset or build all the datacat fragments by hand for default options, then work out how to override them in override_options. That's going to end up being a define that checks if the fragment already exists or something horrible.

Basically all my options suck.

@jglenn9k
Copy link
Contributor

Does #364 look like a reasonable fix for this? For me personally, it solves d/, e/ and f/.

@apenney
Copy link
Contributor

apenney commented Nov 13, 2013

I've merged #364 because it's better than waiting, we can always try to improve further from there. I'll get a release out today.

@apenney
Copy link
Contributor

apenney commented Nov 13, 2013

Did a 2.1.0 release for now!

@apenney
Copy link
Contributor

apenney commented Jan 24, 2014

Closing for now, I think 364 handles this well enough for now.

@apenney apenney closed this Jan 24, 2014
@spielkind spielkind deleted the fix/remove-options-which-are-set-to-false-from-config branch March 4, 2014 10:17
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.

None yet

4 participants