Added options for including/excluding triggers and routines to the mysql::server::backup module #705
Conversation
|
@stevesaliman do you wanna fix-up this commit's email address, or add it to your github profile, before i hit the merge button? (this way it'll be associated with your account, and you get magic internet points.) |
…d a permission problem that was preventing triggers from being backed up
stevesaliman
force-pushed
the
backup-triggers-routines
branch
from
4ef0ba4
to
ec14b87
Compare
|
I didn't know email addresses were important that way. I think I rebased this correctly, and that it is good to go. Now I have a bunch of other commits to rebase for the rest of my projects :-) |
|
Looking at it again, It seems that GitHub is still seeing this with my original address, although gitk on my local machine sees the new address. What address do you see? |
|
looks good to me! |
| is_expected.to contain_file('mysqlbackup.sh').with( | ||
| :content => /bzcat -zc/ | ||
| is_expected.to contain_file('mysqlbackup.sh').with_content( | ||
| /bzcat -zc/ |
There was a problem hiding this comment.
is this part of your change, or is this some merge/rebase issue?
There was a problem hiding this comment.
I notice that some of the other tests were moving from with to with_content. For example the should be able to backup events table test on line 123 was already using with_content, but should skip backing up events table by default on line 51 wasn't, so I changed it to be consistent with other tests.
I hope that wasn't overstepping...
… not we are actually backing up triggers
| table => '*.*', | ||
| privileges => [ 'SELECT', 'RELOAD', 'LOCK TABLES', 'SHOW VIEW', 'PROCESS' ], | ||
| require => Mysql_user["${backupuser}@localhost"], | ||
| if $include_triggers { |
There was a problem hiding this comment.
please just put the variable assignment into an if/else.
There was a problem hiding this comment.
I'd have to say, I think I agree with "someone else" :-)
I think a select is good for times when you need to set a variable based on a number or string which could have a lot of different values - especially when you want to use a regular expression. But it feels a little clunky for a boolean. I've never seen a switch statement used on a boolean in other languages.
If this really needs to be a selector to be merged, so be it, but I'm not sure the code really benefits from the change in this case.
|
I've just pushed a version that uses a variable for the privilege assignment |
| } else { | ||
| $privs = [ 'SELECT', 'RELOAD', 'LOCK TABLES', 'SHOW VIEW', 'PROCESS' ] | ||
| } | ||
|
|
There was a problem hiding this comment.
as always, i would prefer you use selectors here, although, someone else here might disagree ;)
Added options for including/excluding triggers and routines to the mysql::server::backup module
|
thank you @stevesaliman! |
|
Thank you for including this in your module. Out of curiosity, how often do changes get pushed to puppet forge? I want to use the |
|
that's a very sensitive subject, according to @mhaskel |
|
@stevesaliman @igalic mysql doesn't have a release scheduled at this time and we're already booked up for releases for next week, but we can probably do the week after that. |
|
@stevesaliman also we are working on a fix for a bug this introduces for older versions of mysql. Triggers were not introduced as a privilege until 5.1.6 so it fails with a syntax error. #708 has the fix |
The mysqldump command issued by the module-generated mysqlbackup.sh script does not include triggers or routines when it does a file-per-database backup. This means that if users try to restore a database using one of theses backup files, they will not be restoring the full structure of their database, just the data, which can leave users with broken applications due to missing routines. I think that it is important to support triggers and routines in a backup, especially if users are only backing up selected database, in which case the triggers and routines will not get backed up anywhere else.
To resolve this issue, I’ve added support for 2 new mysql::server::backup parameters (mysql::backup is marked as deprecated, so I didn’t modify it) . The first is called include_triggers and the second is called include_routines. When these parameters are true, the appropriate options are added to the mysqldump command. These flags have no effect when we are doing a full backup to a single file, since the triggers and routines will be backed up with the information_schema.
The default value for triggers is
true, and the default forroutinesis false, which mirrors the default behavior of themysqldumpcommand (and gives backwards compatibility with previous versions of this module).I'm not sure how the other two backup technologies treat triggers and routines, if the other technologies typically include routines by default, or if you think the default behavior should be to include everything in a backup, then we should change the the default for routines.
Either way, I think the ability to support the backup of triggers and routines in a file-per-database mode is important because it is the only way for users to generate a backup file for a single database that can be used to fully restore a single named database to its original state.
I've also addressed a privilege problem with the backup user that was preventing triggers from being backed up regardless of the options chosen.
This Pull Request replaces #549, which was done against a pretty old branch.