Skip to content
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

win_service: quoted path fix #32469

Merged
merged 2 commits into from
Nov 2, 2017
Merged

Conversation

jborean93
Copy link
Contributor

SUMMARY

In 2.4, the powershell type of path has an explicit check as to whether it is a valid path or not. The win_service module used this for the path check but it is a valid scenario to quote this value which fails in 2.4. This change removes the path type for path but it will still expand the environment variables like before. Includes an integration test to ensure it doesn't regress again in the future.

Fixes #32368

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

win_service

ANSIBLE VERSION
2.4

@jborean93 jborean93 changed the title Win service path fix win_service: quoted path fix Nov 1, 2017
@ansibot
Copy link
Contributor

ansibot commented Nov 1, 2017

@ansibot ansibot added affects_2.5 This issue/PR affects Ansible v2.5 bugfix_pull_request core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. windows Windows community labels Nov 1, 2017
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Nov 1, 2017
@jhawkesworth
Copy link
Contributor

Code looks good to me, but wondering whether we should have a different name instead of 'path' as it seems to mean 'path to command including arguments'. Perhaps just alias 'path' as 'command_line'?

Probably not worth doing unless it is a distinction and a convention we want to adopt across modules.

Having a type for paths is nice as that's something you can check for correctness - command lines aren't really something we would ever be able to validate though.

@jborean93
Copy link
Contributor Author

From @dssdss at #32368 (comment)

Now, it works! Thank you very much.

@jborean93
Copy link
Contributor Author

@jhawkesworth we can add an alias, as the path type was just added so we can expand environment variables in line. It was later extended to validate paths. At the time I added the path option to win_service I didn't think of the fact it should take in arguments and just used a common option name. I'm not opposed to adding in the alias but it should probably be in another PR and discussed in a wider group.

@jborean93 jborean93 merged commit 5b1db00 into ansible:devel Nov 2, 2017
@jborean93 jborean93 deleted the win_service-path-fix branch November 2, 2017 23:55
jborean93 added a commit that referenced this pull request Nov 2, 2017
* win_service: fix for path in quotes

* Added tests to verify behaviour doesn't regress

(cherry picked from commit 5b1db00)
kiorky pushed a commit to corpusops/ansible that referenced this pull request Nov 6, 2017
* win_service: fix for path in quotes

* Added tests to verify behaviour doesn't regress

(cherry picked from commit 5b1db00)
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.5 This issue/PR affects Ansible v2.5 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. windows Windows community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

win_service quotes in path to handle spaces
3 participants