-
Notifications
You must be signed in to change notification settings - Fork 681
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
wmi resource: properly escape quotes in WMI query #2342
Conversation
Signed-off-by: David Alexander <[email protected]>
Signed-off-by: David Alexander <[email protected]>
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.
You know more about Windows than I do, but this seems like a reasonable change. Thanks, @TheLonelyGhost!
@@ -14,7 +14,7 @@ | |||
# Use win32_service with filter, it returns a single service object | |||
describe wmi({ | |||
class: 'win32_service', | |||
filter: "name like '%winrm%'" | |||
filter: 'name like "%winrm%"' |
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.
Isn't this breaking existing users that used the old syntax?
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.
I'm not sure I understand what you mean. The original test checks if a nested string works with single-quotes ('
) whereas this test checks against nested double-quotes ("
).
This small change makes it so the same test covers 2 concepts instead of just one.
Since the wmi resource implementation executes powershell with nested "
's using a string composed with "
's, it makes sense to check that the string given here uses "
's and that it escapes them properly.
Additionally, this line of the test checks that the filter
option actually works as expected.
What's the question here? What concern is there about a breaking change?
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.
Seems like it was late during my review. All good, thank you for catching this one!
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 for catching this issue @TheLonelyGhost
@@ -14,7 +14,7 @@ | |||
# Use win32_service with filter, it returns a single service object | |||
describe wmi({ | |||
class: 'win32_service', | |||
filter: "name like '%winrm%'" | |||
filter: 'name like "%winrm%"' |
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.
Seems like it was late during my review. All good, thank you for catching this one!
Fixes #2260.
This modifies an existing test to include that scenario and fixes it as 2 separate commits.