New Windows module to chanage CDROM drive letter and unmount virt. cdroms

[RusoSova]

SUMMARY

New module for Windows to have cdroms remapped to the list of desired drives. The list of drive is provided in comma separated fashion. Module will ensure that all CDROMs are mapped to one of the letters provided. In case that are less letters provided than CDROMs, only number of CDROMs=number of drive letters will be changed.
Module can also dismount Windows virtual CDROMs as part of drive letter change or as a stand alone action.

ISSUE TYPE

• New Module Pull Request

COMPONENT NAME

win_cdrom_letter

ADDITIONAL INFORMATION

It's useful to know what your CDROM(s) are mapped as. I was tiered to rely on win_shell and win_command to invoke commands on my hosts. Does not look clean. I hope this will be useful for more people as well.

[ansibot]

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

lib/ansible/modules/windows/win_cdrom_letter.py:4:31: trailing-whitespace Trailing whitespace

The test ansible-test sanity --test future-import-boilerplate [explain] failed with 1 error:

lib/ansible/modules/windows/win_cdrom_letter.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

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

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

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

lib/ansible/modules/windows/win_cdrom_letter.py:0:0: missing: __metaclass__ = type

The test ansible-test sanity --test pep8 [explain] failed with 4 errors:

lib/ansible/modules/windows/win_cdrom_letter.py:4:32: W291 trailing whitespace
lib/ansible/modules/windows/win_cdrom_letter.py:41:161: E501 line too long (174 > 160 characters)
lib/ansible/modules/windows/win_cdrom_letter.py:92:19: W291 trailing whitespace
lib/ansible/modules/windows/win_cdrom_letter.py:129:4: W292 no newline at end of file

The test ansible-test sanity --test pslint [explain] failed with 12 errors:

lib/ansible/modules/windows/win_cdrom_letter.ps1:3:32: PSAvoidTrailingWhitespace Line has trailing whitespace
lib/ansible/modules/windows/win_cdrom_letter.ps1:55:24: PSAvoidUsingWMICmdlet File 'win_cdrom_letter.ps1' uses WMI cmdlet. For PowerShell 3.0 and above, use CIM cmdlet which perform the same tasks as the WMI cmdlets. The CIM cmdlets comply with WS-Management (WSMan) standards and with the Common Information Model (CIM) standard, which enables the cmdlets to use the same techniques to manage Windows computers and those running other operating systems.
lib/ansible/modules/windows/win_cdrom_letter.ps1:68:19: PSAvoidUsingWMICmdlet File 'win_cdrom_letter.ps1' uses WMI cmdlet. For PowerShell 3.0 and above, use CIM cmdlet which perform the same tasks as the WMI cmdlets. The CIM cmdlets comply with WS-Management (WSMan) standards and with the Common Information Model (CIM) standard, which enables the cmdlets to use the same techniques to manage Windows computers and those running other operating systems.
lib/ansible/modules/windows/win_cdrom_letter.ps1:124:1: PSAvoidTrailingWhitespace Line has trailing whitespace
lib/ansible/modules/windows/win_cdrom_letter.ps1:126:1: PSAvoidTrailingWhitespace Line has trailing whitespace
lib/ansible/modules/windows/win_cdrom_letter.ps1:135:44: PSAvoidTrailingWhitespace Line has trailing whitespace
lib/ansible/modules/windows/win_cdrom_letter.ps1:137:1: PSAvoidTrailingWhitespace Line has trailing whitespace
lib/ansible/modules/windows/win_cdrom_letter.ps1:141:1: PSAvoidTrailingWhitespace Line has trailing whitespace
lib/ansible/modules/windows/win_cdrom_letter.ps1:143:60: PSAvoidTrailingWhitespace Line has trailing whitespace
lib/ansible/modules/windows/win_cdrom_letter.ps1:151:28: PSAvoidUsingWMICmdlet File 'win_cdrom_letter.ps1' uses WMI cmdlet. For PowerShell 3.0 and above, use CIM cmdlet which perform the same tasks as the WMI cmdlets. The CIM cmdlets comply with WS-Management (WSMan) standards and with the Common Information Model (CIM) standard, which enables the cmdlets to use the same techniques to manage Windows computers and those running other operating systems.
lib/ansible/modules/windows/win_cdrom_letter.ps1:162:18: PSAvoidTrailingWhitespace Line has trailing whitespace
lib/ansible/modules/windows/win_cdrom_letter.ps1:170:61: PSAvoidTrailingWhitespace Line has trailing whitespace

The test ansible-test sanity --test validate-modules [explain] failed with 5 errors:

lib/ansible/modules/windows/win_cdrom_letter.py:0:0: E305 DOCUMENTATION.author: Invalid author for dictionary value @ data['author']. Got ['RusoSova']
lib/ansible/modules/windows/win_cdrom_letter.py:0:0: E305 DOCUMENTATION.options.drive_letter.example: extra keys not allowed @ data['options']['drive_letter']['example']. Got 'O,S,U,R'
lib/ansible/modules/windows/win_cdrom_letter.py:0:0: E305 DOCUMENTATION.version_added: required key not provided @ data['version_added']. Got None
lib/ansible/modules/windows/win_cdrom_letter.py:0:0: E307 version_added should be '2.9'. Currently None
lib/ansible/modules/windows/win_cdrom_letter.py:106:1: E313 RETURN is not valid YAML

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

lib/ansible/modules/windows/win_cdrom_letter.py:106:1: error RETURN: syntax error: could not find expected ':'

click here for bot help

[ansibot]

cc @ShachafGoldstein @if-meaton @jborean93 @jhawkesworth @nitzmahone
click here for bot help

[RusoSova]

What's wrong with the Author field? :/

lib/ansible/modules/windows/win_cdrom_letter.py:0:0: E305 DOCUMENTATION.author: Invalid author for dictionary value @ data['author']. Got ['RusoSova']

"Got ['RusoSova']" - looks like the script got it right.

[ansibot]

@Daniel-Sanchez-Fabregas @LiranNis @SamLiu79 @ShachafGoldstein @timothyvandenbrande @andrewsaraceni @ar7z1 @blakfeld @brianlloyd @cchurch @chopraaa @chrishoffman @crossan007 @daBONDi @defionscode @dlazz @elventear @equelin @henrikwallstrom @it-praktyk @itigoag @jiuka @johnboy2 @joshludwig @ksubileau @marqelme @mcassaniti @mhunsber @michaeldeaton @nwchandler @nwsparks @petemounce @ptemplier @rcanderson23 @richardcs @riponbanik @sbaerlocher @schwartzmx @smadam813 @themiwi @tksarah @tmmruk

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 shipit if you would like to see it merged.

click here for bot help

[RusoSova]

Thanks to agaffney @ IRC for pointing out that author should be Name (@githubaccount)

[jhawkesworth]

I like the idea of having a module to manage which letter a cd/dvd drive is mounted as. (I too, have some win_shell powershell to manage this problem and its ugly!)

I do have some reservations about this implementation though.
My first is that the module's interface makes its feel like its issuing a command, rather than describing the state you want once the module has completed. While this might sound like its a fussy thing to care about, I think the 'command' style might be making the module more complex than perhaps it needs to be. Its is definitely worthwhile getting the 'interface' of the module right - you can change the implementation later if necessary but once people start using modules its much harder to change the parameters they accept.

You've tackled some edge cases such as multiple cd/dvd drives and virtual drives, which is great, but I wonder if multiple drives could simply be handled by passing a list of cd/dvd drives to ensure are mounted and perhaps use 'state' module parameter to handle whether a drive is mounted or dismounted, and perhaps whether virtual drives should be dismounted. I'm not sure what the interface would be exactly, but I think it might be possible to make the module parameters a description of how you want the drives arranged once the module has completed.

This example bothers me a bit:

- name: Change CDROM letter to Z or Y if Z is in use or to K if Z and Y are in use
  win_cdrom_letter:
   drive_letter: Z,Y,K
  register: result

It seems the idea here is to just get the cd/dvd drive out of the way as the end state could be that it is mapped to Z, Y or K. Personally this is not how I want to arrange things. I would much rather know that by the time the module has run the Z drive is allways the cd/dvd drive on all machines that it has run against (or the task fails because that couldn't be achieved). I appreciate you may well be trying to bring order to lots of machines which users have mapped drives on to suit their own needs. In which case I'd hope that you could use 'win_disk_facts' (or something similar if that doesn't return information about cd/dvd drives) so you can detect any machine where other drives are already mapped to the destination drive letter you want to use.

Also if the drive_letter parameter needs to have more than one letter I think it should take a list, in which case a proper yaml list of letters would be preferable in the examples rather than a comma separated list.

I hope the above is of some use - I'd definitely think about having a state: present / absent module parameter.

Happy to chat more if you want clarification, but I hope the above is helpful.

[RusoSova]

Thank you very much for the feedback. This module started as a simple remap of a single CDROM drive, usually there is a single CDROM drive on a system, but no always. And virtual CDROMs can make this issues even less straight forward.

Unfortunately, win_disk_facts does not return information about CDROMs, at least from my testing. I also find win_disk_facts fairly cumbersome to use to pass the information to the subsequent tasks, way to much logic to build in the .yml file. Albeit, I might not be using it “correctly”.

The idea was to create simple interface to tackle as many corner cases as possible. There is no way to find the number of CDROMs and their letters via Ansible modules atm.
For example, if you want to make sure your “physical” CDROM is mounted always as Z on all the machines in the inventory you can use just one letter nomenclature. This will only make change to a single CDROM, which will be good in 95%+ of the cases. And if Z is already in use by non-cdrom, the task will fail

- name: Change CDROM letter to Z
  win_cdrom_letter:
   drive_letter: Z
   dismount_virtual: yes
  register: result

But if the goal is to make sure no CDROM is taking the “prime” drive letters like D:, E:, F: or any other of your reserved letters, the multi-letter approach will be best.

- name: Change CDROM letter to Z or Y if Z is in use or to K if Z and Y are in use
  win_cdrom_letter:
   drive_letter: Z,Y,K
  register: result

The above task also guarantees that, if possible, the CDROM will be mapped to Z before it will try to mount it to Y.
I 100% agree that the multiple letters should be passed as a list and not in comma separated fashion. I need to update this.
The one scenario I was debating over is: what to do in case the CDROM is already mapped to one of the letters on the list. For example, there are 2 CDROMs mapped to Z: and Y: respectively and this is the task in question:

- win_cdrom_letter:
   drive_letter: K,Z,Y
  register: result

Should the CDROMs be remapped to K: and Z: if possible or assume that all the letters are of equal cost? I decided on the latter as I found this approach more inline with my goal. The end state is when the CDROMs comply with either K, Z, or Y.

From your comments, I get the idea that perhaps separating this module into two separate ones is a preferred approach and more inline with Ansible way.

win_cdrom_info
win_cdrom_letter

My initial thought on win_cdrom_info return would be something very simple like this:

number_of_virtual: int
virtual_letters: [list of letters]
number_of_physical: int
physical_letters: [list of letters]

This approach with simplified win_cdrom_letter does not give you the ability to know which letters are free to use though. Sometimes “get the cd/dvd drive out of the way” is what might be needed and the other times you might need to be strict. I tried to encompass both possibilities in the current win_cdrom_letter implementation.

My personal use cases are:

- name: Change CDROM letter to Z or if in use make it Y
  win_cdrom_letter:
   drive_letter: Z,Y
   dismount_virtual: yes
  register: result

- name: Dismount virtual cdroms
  win_cdrom_letter:
   dismount_only: yes
  register: result

Disabling and enabling physical CDROMs is out of scope for this module. I believe CIM’s method to disable PnP devices is Win10/Server2016+ so it won’t be compatible with server 2012.

So, as it stands, I am not sure what direction this should be heading. I would like to hear more opinions about this before I start doing any changes or abandon it all together. I will try to create a simple win_cdrom_info module, it can be useful by itself.
Thank you again for the feedback.

[jborean93]

I agree with what @jhawkesworth is saying here and this module is a lot more run this operation (rename) than be this. I see you've attempted to make things idempotent but in the end it just makes things a bit messy and hard to understand. I appreciate the work you've put into this so far but this isn't really what we are looking for.

[ansibot]

@RusoSova this PR contains more than one new module.

Please submit only one new module per pull request. For a detailed explanation, please read the grouped modules documentation

click here for bot help

[ansibot]

@RusoSova this PR contains the following merge commits:

• e1bef79

Please rebase your branch to remove these commits.

click here for bot help

[ansibot]

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

lib/ansible/modules/windows/win_cdrom_letter.py:0:0: module-incorrect-version-added: version_added should be '2.10'. Currently 2.9
lib/ansible/modules/windows/win_description.py:0:0: invalid-metadata-type: ANSIBLE_METADATA.metadata_version: not a valid value for dictionary value @ data['metadata_version']. Got '1.0'
lib/ansible/modules/windows/win_description.py:0:0: module-incorrect-version-added: version_added should be '2.10'. Currently 2.9

The test ansible-test sanity --test pslint [explain] failed with 2 errors:

lib/ansible/modules/windows/win_description.ps1:66:21: PSCustomUseLiteralPath: Use the explicit -LiteralPath parameter name instead of -Path
lib/ansible/modules/windows/win_description.ps1:75:13: PSCustomUseLiteralPath: Use the explicit -LiteralPath parameter name instead of -Path

click here for bot help

[gundalow]

@RusoSova If you are having issue with your branch you may wish to create a fresh branch and PR

[gundalow]

Hi!

Thanks very much for your submission to Ansible. It sincerely means a lot to us that you've taken time to contribute.

You've clearly put a lot of work and thought into this. There has been some great discussion as part of this PR

Unfortunately, we're not sure if we want this feature in the program, and I don't want this to seem confrontational. Our reasons for this are:

• the module's interface makes its feel like its issuing a command, rather than describing the state you want once the module has completed. This is often a good indicator that something doesn't fit well with "The Ansible Way" New Windows module to chanage CDROM drive letter and unmount virt. cdroms #59872 (comment)
• There has been a good attempted to make things idempotent, however in the end it just makes things a bit messy and hard to understand New Windows module to chanage CDROM drive letter and unmount virt. cdroms #59872 (comment)

However, we're absolutely always up for discussion. Since this is a really busy project, we don't always see comments on closed tickets, but want to encourage
open dialog. You can stop by the development list, and we'd be glad to talk about it - and we might even be persuaded otherwise!

• https://groups.google.com/forum/#!forum/ansible-devel
• If you use IRC join us in #ansible-windows on Freenode

In the future, sometimes starting a discussion on the development list prior to implementing a feature can make getting things included a little easier, but it's not always necessary.

Thank you once again for this and your interest in Ansible!