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_chocolatey_feature: new module #42848

Merged
merged 2 commits into from
Jul 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
74 changes: 74 additions & 0 deletions lib/ansible/modules/windows/win_chocolatey_feature.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
#!powershell

# Copyright: (c), 2018 Ansible Project
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)

#Requires -Module Ansible.ModuleUtils.CommandUtil
#Requires -Module Ansible.ModuleUtils.Legacy

$ErrorActionPreference = "Stop"

$params = Parse-Args -arguments $args -supports_check_mode $true
$check_mode = Get-AnsibleParam -obj $params -name "_ansible_check_mode" -type "bool" -default $false

$name = Get-AnsibleParam -obj $params -name "name" -type "str" -failifempty $true
$state = Get-AnsibleParam -obj $params -name "state" -type "str" -default "enabled" -validateset "disabled", "enabled"

$result = @{
changed = $false
}

Function Get-ChocolateyFeatures {
param($choco_app)

$res = Run-Command -command "`"$($choco_app.Path)`" feature list -r"
if ($res.rc -ne 0) {
Fail-Json -obj $result -message "Failed to list Chocolatey features: $($res.stderr)"
}
$feature_info = @{}
$res.stdout -split "`r`n" | Where-Object { $_ -ne "" } | ForEach-Object {
$feature_split = $_ -split "\|"
$feature_info."$($feature_split[0])" = $feature_split[1] -eq "Enabled"
}

return ,$feature_info
}

Function Set-ChocolateyFeature {
param(
$choco_app,
$name,
$enabled
)

if ($enabled) {
$state_string = "enable"
} else {
$state_string = "disable"
}
$res = Run-Command -command "`"$($choco_app.Path)`" feature $state_string --name `"$name`""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recommended command here would be:

choco feature enable --name="'<name>'"

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the command you have will work, it is not best practice.

Copy link
Contributor Author

@jborean93 jborean93 Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ($res.rc -ne 0) {
Fail-Json -obj $result -message "Failed to set Chocolatey feature $name to $($state_string): $($res.stderr)"
}
}

$choco_app = Get-Command -Name choco.exe -CommandType Application -ErrorAction SilentlyContinue
if (-not $choco_app) {
Fail-Json -obj $result -message "Failed to find Chocolatey installation, make sure choco.exe is in the PATH env value"
}

$feature_info = Get-ChocolateyFeatures -choco_app $choco_app
if ($name -notin $feature_info.keys) {
Fail-Json -obj $result -message "Invalid feature name '$name' specified, valid features are: $($feature_info.keys -join ', ')"
}

$expected_status = $state -eq "enabled"
$feature_status = $feature_info.$name
if ($feature_status -ne $expected_status) {
if (-not $check_mode) {
Set-ChocolateyFeature -choco_app $choco_app -name $name -enabled $expected_status
}
$result.changed = $true
}

Exit-Json -obj $result
50 changes: 50 additions & 0 deletions lib/ansible/modules/windows/win_chocolatey_feature.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
#!/usr/bin/python
# -*- coding: utf-8 -*-

# Copyright: (c) 2018, Ansible Project
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)

ANSIBLE_METADATA = {'metadata_version': '1.1',
'status': ['preview'],
'supported_by': 'community'}

DOCUMENTATION = r'''
---
module: win_chocolatey_feature
version_added: '2.7'
short_description: Manages Chocolatey features
description:
- Used to enable or disable features in Chocolatey.
options:
name:
description:
- The name of the feature to manage.
- Run C(choco.exe feature list) to get a list of features that can be
managed.
required: yes
state:
description:
- When C(disabled) then the feature will be disabled.
- When C(enabled) then the feature will be enabled.
choices:
- disabled
- enabled
default: enabled
author:
- Jordan Borean (@jborean93)
'''

EXAMPLES = r'''
- name: disable file checksum matching
win_chocolatey_feature:
name: checksumFiles
state: disabled

- name: stop Chocolatey on the first package failure
win_chocolatey_feature:
name: stopOnFirstPackageFailure
state: enabled
'''

RETURN = r'''
'''
1 change: 1 addition & 0 deletions test/integration/targets/win_chocolatey_feature/aliases
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
windows/ci/group1
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
from __future__ import (absolute_import, division, print_function)
__metaclass__ = type


def choco_checksum_state(value):
return [i for i in value if i.startswith("checksumFiles|")][0].split("|")[1] == "Enabled"


class FilterModule(object):

def filters(self):
return {
'choco_checksum_state': choco_checksum_state
}
20 changes: 20 additions & 0 deletions test/integration/targets/win_chocolatey_feature/tasks/main.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
- name: ensure Chocolatey is installed
win_chocolatey:
name: chocolatey
state: present

- name: ensure we start from a baseline for test feature
win_chocolatey_feature:
name: checksumFiles
state: disabled

- block:
- name: run tests
include_tasks: tests.yml

always:
- name: set feature back to enabled
win_chocolatey_feature:
name: checksumFiles
state: enabled
95 changes: 95 additions & 0 deletions test/integration/targets/win_chocolatey_feature/tasks/tests.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
---
- name: fail on invalid feature
win_chocolatey_feature:
name: failFeature
state: enabled
register: fail_res
failed_when: '"Invalid feature name ''failFeature'' specified, valid features are: " not in fail_res.msg'

- name: enable disabled feature (check mode)
win_chocolatey_feature:
name: checksumFiles
state: enabled
check_mode: yes
register: enable_check

- name: get actual state of feature (check mode)
win_command: choco.exe feature list -r
register: enable_actual_check

- name: assert enable disabled feature (check mode)
assert:
that:
- enable_check is changed
- enable_actual_check.stdout_lines|choco_checksum_state == False

- name: enable disabled feature
win_chocolatey_feature:
name: checksumFiles
state: enabled
register: enable

- name: get actual state of feature
win_command: choco.exe feature list -r
register: enable_actual

- name: assert enable disabled feature
assert:
that:
- enable is changed
- enable_actual.stdout_lines|choco_checksum_state == True

- name: enable disabled feature (idempotent)
win_chocolatey_feature:
name: checksumFiles
state: enabled
register: enable_again

- name: assert enable disabled feature (idempotent)
assert:
that:
- not enable_again is changed

- name: disable enabled feature (check mode)
win_chocolatey_feature:
name: checksumFiles
state: disabled
check_mode: yes
register: disable_check

- name: get actual state of feature (check mode)
win_command: choco.exe feature list -r
register: disable_actual_check

- name: assert disable enabled feature (check mode)
assert:
that:
- disable_check is changed
- disable_actual_check.stdout_lines|choco_checksum_state == True

- name: disable enabled feature
win_chocolatey_feature:
name: checksumFiles
state: disabled
register: disable

- name: get actual state of feature
win_command: choco.exe feature list -r
register: disable_actual

- name: assert disable enabled feature
assert:
that:
- disable is changed
- disable_actual.stdout_lines|choco_checksum_state == False

- name: disable enabled feature (idempotent)
win_chocolatey_feature:
name: checksumFiles
state: disabled
register: disable_again

- name: assert disable enabled feature (idempotent)
assert:
that:
- not disable_again is changed