Add support for REQUIRE SSL|X509 option #888
Conversation
| @@ -107,6 +107,8 @@ def self.cmd_options(options) | |||
| options.each do |opt| | |||
| if opt == 'GRANT' | |||
| option_string << ' WITH GRANT OPTION' | |||
| elsif op = opt.match(/^REQUIRE\s(SSL|X509)$/) | |||
There was a problem hiding this comment.
well, we already know that if SSL or X509 is given, it's a REQUIRE. Why not take a similar approach than 'GRANT' and just use 'SSL' or 'X509'; Those will be looked for here and subsequently the option_string will fill in the REQUIRE. So something like:
elsif opt == 'SSL' or opt == 'X509'
option_string << "REQUIRE #{opt}"
| options = [] | ||
| options << 'GRANT' if rest.match(/WITH\sGRANT\sOPTION/) | ||
| req_opt = rest.match(/REQUIRE\s(SSL|X509)/) | ||
| options << req_opt[0] if req_opt |
There was a problem hiding this comment.
If the above recommendation is taken, this also needs to be changed,
edestecd
force-pushed
the
master
branch
5 times, most recently
from
f03a744 to
f4961e9
Compare
|
Added spec tests and Doc to the README. |
|
I prefer the verbosity of REQUIRE, b/c its more clear what you are doing when glancing the code. http://dev.mysql.com/doc/refman/5.7/en/grant.html So there is no chance for a single word there. I have not added support for these as of yet, but hope to in the future... I'm not entirely married to the REQUIRE, however. If others feel the same way, I will change it. |
|
@edestecd using REQUIRE is fine for me. and SSL and X509 support is exactly what I need for the stuff I'm doing. Thanks! |
|
Code looks good to me, also nice testing coverage. Let's go! |
Basic support for REQUIRE SSL or REQUIRE X509 only through the options param on the mysql_grant type. This seems like an option we should support.
It would be nice to support any option in the options array, but I'm not sure about the code to check if the options are set in the instances method. Seems like you need to look for something explicitly in the rest, the way it is set up currently.