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

Windows module: win_optional_feature for Windows workstations #53709

Merged
merged 20 commits into from Mar 20, 2019

Conversation

Projects
None yet
4 participants
@rcanderson23
Copy link
Contributor

rcanderson23 commented Mar 12, 2019

SUMMARY

This module is used to install or remove windows features Windows workstation versions such as Windows 10. The current module win_feature does this for Server versions but I felt the cmdlets were different enough that it justified a separate module.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

win_optional_feature

ADDITIONAL INFORMATION

Module allows for installation and removal of Windows features such as NetFx3 and Windows Subsystem for Linux

ansible 2.8.0.dev0
  config file = None
  configured module search path = ['/Users/randerson/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/randerson/projects/ansible/lib/ansible
  executable location = /Users/randerson/projects/ansible/bin/ansible
  python version = 3.7.2 (default, Feb 12 2019, 08:15:36) [Clang 10.0.0 (clang-1000.11.45.5)]```
@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Mar 12, 2019

The test ansible-test sanity --test line-endings [explain] failed with 1 error:

lib/ansible/modules/windows/win_optional_feature.ps1:0:0: use "\n" for line endings instead of "\r\n"

The test ansible-test sanity --test shebang [explain] failed with 1 error:

lib/ansible/modules/windows/win_optional_feature.ps1:0:0: module should not be executable

click here for bot help

@ansibot

This comment has been minimized.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 12, 2019

@dagwieers
Copy link
Member

dagwieers left a comment

LGTM, well done.

I am torn between having this as a separate module (keep it simple) and one single win_feature module (user convenience). It would help if Microsoft has a clear vision on how this would evolve in the future (wrt. win_package, win_capability, etc...). Less is more IMO.

@ansibot ansibot removed the needs_triage label Mar 13, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 13, 2019

@rcanderson23 this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 19, 2019

The test ansible-test sanity --test pslint [explain] failed with 1 error:

lib/ansible/modules/windows/win_optional_feature.ps1:34:1: PSUseDeclaredVarsMoreThanAssignments The variable 'feature_result' is assigned but never used.

click here for bot help

@ansibot ansibot added ci_verified and removed ci_verified labels Mar 19, 2019

@jborean93
Copy link
Contributor

jborean93 left a comment

Looks nearly complete just a few more comments and still needs the tests enabled.


$spec = @{
options = @{
name = @{ type = "str"; required = $true }

This comment has been minimized.

@jborean93

jborean93 Mar 19, 2019

Contributor

You don't have to do it now but accepting a list makes this module a whole lot more useful and faster than just using with_items/loop.

Show resolved Hide resolved lib/ansible/modules/windows/win_optional_feature.ps1 Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_optional_feature.ps1
Show resolved Hide resolved lib/ansible/modules/windows/win_optional_feature.ps1 Outdated
@jborean93

This comment has been minimized.

Copy link
Contributor

jborean93 commented Mar 19, 2019

Just an FYI the DISM module in PowerShell is present in Windows 8 and Server 2012 and newer so we should be able to run the tests on these hosts https://peter.hahndorf.eu/blog/WindowsFeatureViaCmd.html.

@jborean93

This comment has been minimized.

Copy link
Contributor

jborean93 commented Mar 19, 2019

Not sure why the tests are failing but it's potentially because .NET 3.5 requires Windows Update to download the feature files and that's by default not allowed in WinRM. I would change the test feature name to TelnetClient or something as equally small in the tests.

The only remaining issue apart from the test failures is around the indentation, it's mostly ok but there are a few sections that are indented more than normal. Make sure you keep things aligned, for example https://github.com/ansible/ansible/pull/53709/files#diff-a8868ba1722b2acb0e6d9d733eb893b1R52 is indented more than https://github.com/ansible/ansible/pull/53709/files#diff-a8868ba1722b2acb0e6d9d733eb893b1R45 and so on. It looks like both spaces and tabs are being used, make sure you keep the indentation style the same and I believe spaces are preferred in this project.

image

@rcanderson23

This comment has been minimized.

Copy link
Contributor Author

rcanderson23 commented Mar 19, 2019

Thank you for pointing the tabs. I didn't realize I hadn't configured my editor for powershell correctly.

@jborean93

This comment has been minimized.

Copy link
Contributor

jborean93 commented Mar 20, 2019

@rcanderson23 looking good, I've just made a minor tweak to the tests so if they were to run on an older host they won't bomb out. The aliases is just an optimisation for CI so it won't create a new host, we still need to skip the tests manually as well.

@jborean93

This comment has been minimized.

Copy link
Contributor

jborean93 commented Mar 20, 2019

I am torn between having this as a separate module (keep it simple) and one single win_feature module (user convenience). It would help if Microsoft has a clear vision on how this would evolve in the future (wrt. win_package, win_capability, etc...). Less is more IMO.

@dagwieers I wholeheartily agree with you here but unfortunately most docs I've seen from Microsoft use one or the other. I see a more prevailing trend to use the optional features through DISM but that's not exclusive. From what I've gathered the DISM and ServerManager cmdlets are developed by separate teams in Microsoft and they show no signs of giving way to each other. This page has a nice overview on the differences https://peter.hahndorf.eu/blog/WindowsFeatureViaCmd.html. What makes this even more fun is the addition of capabilities which I believe are also controlled by the DISM cmdlets.

@jborean93 jborean93 merged commit 31ceba7 into ansible:devel Mar 20, 2019

1 check passed

Shippable Run 114864 status is SUCCESS.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.