win_chocolatey_feature: new module #42848
Conversation
ansibot
added
affects_2.7
|
@Daniel-Sanchez-Fabregas @LiranNis @SamLiu79 @timothyvandenbrande @andrewsaraceni @ar7z1 @blakfeld @brianlloyd @chrishoffman @daBONDi @elventear @erwanquelin @henrikwallstrom @if-meaton @joshludwig @marqelme @nwchandler @nwsparks @petemounce @ptemplier @riponbanik @rndmh3ro @schwartzmx @smadam813 @themiwi @tksarah As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add |
| @@ -0,0 +1,75 @@ | |||
| #!powershell | |||
| # This file is part of Ansible | |||
There was a problem hiding this comment.
I know this is a minor nitpick, but I am looking at making the Windows modules more consistent. Does the addition of # This file is part of Ansible has any useful meaning ?
Also, we tend to have # -*- coding: utf-8 -*- as the second line.
There was a problem hiding this comment.
BTW I see you are doing this for the .py file.
There was a problem hiding this comment.
The # This file is part of Ansible isn't needed IMO, I'm just copying pasting from another module and happy to have it removed. I don't think we should have # -*- coding: utf-8 -* as that is not a Windows/PowerShell-ism. There's no benefit for having that in the PowerShell modules.
There was a problem hiding this comment.
Alright, I always asumed # -*- coding: utf-8 -*- was an editor/IDE feature, but it does seem to be tight to Python specifically. Let me get rid of those then.
| #!powershell | ||
| # This file is part of Ansible | ||
|
|
||
| # Copyright (c) 2018 Ansible Project |
There was a problem hiding this comment.
Another minor nitpick is that the syntax for copyright lines used to be:
# Copyright: (c) 2018, Ansible ProjectI know people have been adding other stuff since, but this was originally documented and automated by @abadger and I have been updating at least my modules like this.
There was a problem hiding this comment.
BTW I see you are doing this for the .py file.
ansibot
removed
the
needs_triage
| } else { | ||
| $state_string = "disable" | ||
| } | ||
| $res = Run-Command -command "`"$($choco_app.Path)`" feature $state_string --name `"$name`"" |
There was a problem hiding this comment.
The recommended command here would be:
choco feature enable --name="'<name>'"
There was a problem hiding this comment.
While the command you have will work, it is not best practice.
There was a problem hiding this comment.
I have a few questions
- Why the double quoting
" and 'of the value, surely having separate arguments (that are correctly escaped) is a better option - Is it a preference from Chocolatey from a docs and ensuring user's escape correctly or is the parser expecting this format
This is calling CreateProcessW directly so cmd shell quotes don't take effect here, only " is used in this layer when being parsed by Windows.
While in this module it isn't as important, other modules we have a special function that takes in an array of args and correctly escapes them so the program receive those args as written (without the user escaping them themselves). The Chocolatey preferred format --key="'value'" won't work well with the function we have while the --key value format is a lot easier.
There was a problem hiding this comment.
Due to the myriad of escaping, and quote removal, and potential for spaces in paths/names, etc. that happens when interacting between cmd and powershell, etc. our guidance/best practice is always to use the equivalent of this:
choco feature enable --name="'<featureName>'"
for each Chocolatey command. We have found that this is the most resilient way to correctly pass the intended parameters into Chocolatey, and have them be recognised.
Arguably, this isn't needed here, since feature names will never have spaces, etc. However, if someone were to look at this command, and how it is doing something, and then apply the same to another Chocolatey command, it might not work as intended.
There was a problem hiding this comment.
Cool, so sounds like it's more getting users to escape their args correctly. From a maintainability perspective I feel like keeping the existing format is better for us. If a user copies and paste what we are doing then the onus is on them to figure out what is going wrong then just blindly copying it. There are plenty of docs in Chocolatey to cover these scenarios as well.
ansibot
added
needs_revision
SUMMARY
In continuing with the work of completing https://chocolatey.org/docs/features-infrastructure-automation#chocolatey-integration-implementation-with-common-configuration-managers
Manages Chocolatey features https://chocolatey.org/docs/commands-feature.
ISSUE TYPE
COMPONENT NAME
win_chocolatey_feature
ANSIBLE VERSION