Replaced 'DROP USER' with 'DROP USER IF EXISTS' #942
Conversation
|
@libertamohamed this looks like a sensible change, however could you look at the failing tests that this new functionality has introduced. |
|
@tphoney the test is failing because of the MySQL version. It's running on 5.5. |
|
@libertamohamed can you pin the tests around this version ? |
|
shouldn't we add version checking, rather than pinning?? |
|
even though english is my first language. my usage resembles this http://vignette2.wikia.nocookie.net/familyguy/images/e/e4/Wacky_Waving_Inflatable_Arm_Flailing_Tube_Man.gif/revision/latest?cb=20120930090058 apologies for that. @igalic is correct. |
|
mysql has internal version checking, or you can just use our |
|
@libertamohamed Is there any movement on this PR? |
|
@libertamohamed I apologize but due to the lack of movement on this PR and your lack of response when prompted I feel that we must know close this PR. I apologize if this is inconvenient for you and if in the future you wish to reopen it then we will be more than happy to once again review it. |
|
Sorry for my lack of response. I was on a sabbatical and didn't hand over this PR to one of my colleagues. Just created a to-do in our team for this. Will start a new PR if it's ready. |
|
@libertamohamed I apologize for jumping the gun then. I can reopen this PR now if you want? |
|
@david22swan, sure if you can re- open it will be great. It's a small fix, but need to make time for it. More pressure is good, haha! |
|
@libertamohamed Here you go, once again sorry for jumping the gun on closing it. |
| @@ -96,7 +96,9 @@ def create | |||
|
|
|||
| def destroy | |||
| merged_name = @resource[:name].sub('@', "'@'") | |||
| self.class.mysql_caller("DROP USER '#{merged_name}'", 'system') | |||
| if_exists = mysqld_version >= '5.7' ? 'IF EXISTS' : '' | |||
There was a problem hiding this comment.
please use Puppet::Util::Package.versioncmp() for comparing version numbers. see L17 for an example usage.
| provider.class.expects(:mysql_caller).with("DROP USER 'joe'@'localhost'", 'system') | ||
| if_exists = mysqld_version >= '5.7' ? 'IF EXISTS' : '' | ||
|
|
||
| provider.class.expects(:mysql_caller).with("DROP USER #{if_exists} 'joe'@'localhost'", 'system') |
There was a problem hiding this comment.
we'll still want to test the original query, so adding another test case would be most helpful here. there is also a pattern near the top of this file for setting the version number in the class.
e.g.
it 'removes a user using IF EXISTS' do
{set version to 5.7.1 or something}
provider.class.expects(:mysql_caller).with("DROP USER IF EXISTS 'joe'@'localhost'", 'system')
...
end
|
could it be that this whole thing is in really bad need of a rebase? |
|
I did a rebase before commiting. Made some changes based on the review of @eputnam. Tests are passing. Just waiting to be accepted. |
|
@libertamohamed It has 216 commits currently from many different authors. An easy way to tell if a rebase worked is if the PR only has a few commits (or even only one commit) and all from you. The diff also shows ~5,000 lines being changed in 87 files, so checking that is another way to check if the changes being made by the PR are the ones you expected. |
|
Sorry for the late response. I did a new rebase now. Could you check it? |
| @@ -96,7 +96,9 @@ def create | |||
|
|
|||
| def destroy | |||
| merged_name = @resource[:name].sub('@', "'@'") | |||
| self.class.mysql_caller("DROP USER '#{merged_name}'", 'system') | |||
| if_exists = (Puppet::Util::Package.versioncmp(mysqld_version, '5.7') >= 0) ? 'IF EXISTS ' : '' | |||
There was a problem hiding this comment.
This needs a mariadb version check as well. Might as well throw in percona, too. e.g. https://github.com/xelmedia/puppetlabs-mysql/blob/b88fe42bb0302c4e4cd048a2b82d55c2c7e5f525/lib/puppet/provider/mysql_user/mysql.rb#L86-L87
No description provided.