-
Notifications
You must be signed in to change notification settings - Fork 682
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
Allow mysql resource to accept socket path #1933
Conversation
Signed-off-by: Richard Shade <rshade@rightscale.com>
Signed-off-by: Richard Shade <rshade@rightscale.com>
Signed-off-by: Richard Shade <rshade@rightscale.com>
@rshade thanks for your PR! Unfortunately, we cannot accept this change as written. The I verified this by creating a Vagrant machine, installing MySQL, configuring it to listen on 13306 (and forwarded that port), and then tried to connect to it using both
One potential solution would be to set the default
That would still allow you to define a socket in your tests without breaking existing users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unable to accept as-written, would break existing known use.
Signed-off-by: Richard Shade <rshade@rightscale.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty great to me, @rshade, thanks!
If you have time to level-up this change, I would break out the code the builds the CLI command string into a separate method (called mysql_command
or something similar), and then that will allow us to test that logic easily and independently. If you don't have time for that, lemme know and I can follow-up with another PR to add that as well.
I will try and level up a little beginning of next week. Would it be cool if I open issues on a few of the items and we can work from there? |
That works for me! |
Signed-off-by: Richard Shade <rshade@rightscale.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @rshade for this improvement
@@ -16,10 +16,12 @@ class MysqlSession < Inspec.resource(1) | |||
end | |||
" | |||
|
|||
def initialize(user = nil, pass = nil, host = 'localhost') | |||
def initialize(user = nil, pass = nil, host = 'localhost', port = nil, socket = nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future we probably want to use opts here to avoid passing nil values
command += " -h #{@host}" | ||
end | ||
command += " --port #{@port}" unless @port.nil? | ||
command += " #{db} -s -S #{@socket} -e \"#{escaped_query}\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The -S #{@socket}
argument is redundant with -h localhost
and leads to MySQL error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the current version breaks for all queries without socket connections because of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is fixed in newer versions.
We can not use this by default against mysql installations via the mysql cookbook.