Windows: win_regedit; Add support for managing binary registry data to win_regedit

[jhawkesworth]

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

win_regedit

ANSIBLE VERSION

ansible 2.1.0 (win_regedit_binary_tests cca084c89d) last updated 2016/04/07 18:24:39 (GMT +100)
  lib/ansible/modules/core: (detached HEAD cf01087a30) last updated 2016/04/05 08:30:23 (GMT +100)
  lib/ansible/modules/extras: (add_binary_to_regedit 7a8d3cf392) last updated 2016/04/14 21:23:07 (GMT +100)
  config file = 
  configured module search path = Default w/o overrides

SUMMARY

The current implementation of win_regedit will not let you manage binary data.
This is because it is expecting strings but powershell sees the binary data as a byte array.
I added logic to handle converting hex strings in module parameters into byte arrays, and a function
to test if the current registry data value matched the supplied registry data value.
If you export binary data using regedit, you get a string representation of the hex values with a 'hex:' prefix and comma separated values, so this is the preferred form for expressing binary data in module parameters. However the conversion function allows the 'hex:' prefix to be omitted and the hex values to be separated by commas or colons.

I also added data_changed and data_type_changed boolean return values and documented these.

Not aware of an existing bug report for this, but fixes an issue I encountered while using Ansible.

I have a PR which adds integration tests for win_regedit which I will submit shortly.

Example: Running the following:

- name: remove registry key used for testing
  win_regedit:
    key: 'HKCU:\SOFTWARE\Cow Corp'
    state: absent

Before:

changed: [WIN-TEST] => {"changed": true}

After:

changed: [WIN-TEST] => {"changed": true, "data_changed": false, "data_type_changed": false}

[jhawkesworth]

ping @nitzmahone @trondhindenes - as promised, sorry for the delay

[jhawkesworth]

Oops looks like I need to fix my documentation. Will tackle asap.

[gregdek]

Thanks @jhawkesworth for this PR. This PR requires revisions, either because it fails to build or by reviewer request. Please make the suggested revisions. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

[This message brought to you by your friendly Ansibull-bot.]

[jhawkesworth]

ready_for_review

[gregdek]

Thanks @jhawkesworth. To the current maintainers, @joshludwig, @smadam813 please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit' or 'needs_revision' as appropriate.

[This message brought to you by your friendly Ansibull-bot.]

[jhawkesworth]

@nitzmahone Thanks for merging.

[dreamcat4]

@jhawkesworth Can you please take a look? Just not familiar enough to understand your code here / know how to fix it. Many thanks.

[jhawkesworth]

Hmm.i did change how the current reg data and reg data in the module params are compared so I may have made a bug there.

I will investigate as soon as I can.

[jhawkesworth]

Hi,

I have documented how to set binary values now here: https://github.com/jhawkesworth/ansible-modules-extras/blob/2ecb1a37dcfb0e432c87a8610321c08659e771e1/windows/win_regedit.py

The doc probably won't be visible until the next time the site is rebuilt.

The idea was to make it easy to use previously-exported binary values - I never managed to guess the syntax you are using when I was trying to get it to work with a binary value, hence this pull request.

I will dig further but wanted to ask if you could try using the format as described above please?

[dreamcat4]

OK thanks @jhawkesworth I can see that now :)

https://github.com/jhawkesworth/ansible-modules-extras/blob/2ecb1a37dcfb0e432c87a8610321c08659e771e1/windows/win_regedit.py#L97-L104

Yeah. That is a more preffered method because like you say - its in same format as whats spit out by Regedit / Export and similar windows operation(s). So no, I don't mind converting / switching over to be using that approach now that it is possible with your patch.

BTW the format which had been working up until now was YAML's generic / native data representation method for an array of bytes [0x01,0x02]. So that's where that came from :)

other breakage(s).

Can we maintain the capability for setting a registry value to an empty string? Like "" or ''. I think this may be important, because when come across weird / special cases in windows registry. Whereby certain registry entr(ies) can only be made to work in desired fashion by setting the value to an empty string. For example this is needed when either:

a) the key in question is locked down with ACLs, cannot be deleted.
b) the program reading the key will automatically try to re-create it and set to some new default value if its missing. (but we want to make a default setting which is to clear it, and not have anything in future set to that key's value).

SO anyway in your code here it seems like the operation errors out because in your new code, whe way that this powershell ifelse statement is written, it cannot compare 2 "" values. So perhaps some extra checking is needed there? Like perhaps break it down into pieces, instead of 1 long line. What do you think?

[jhawkesworth]

Thanks for this. Wish I'd realized there was a way to specify a byte array in yaml.
I feel that supplying a yaml byte array ought to be supported as well as the registry string representation thing.

However fixing the empty string comparision definitely needs doing and I am working on it now.

I think its actually the parameter validation on the Compare-RegistryData function

I think it should look this this:

    Param (
        [parameter(Mandatory=$true)]
        [AllowEmptyString()]$ReferenceData,
        [parameter(Mandatory=$true)]
        [AllowEmptyString()]$DifferenceData
        )

I am going to create an integration test to handle this case to test it out and then create a PR assuming the above actually fixes.

Sorry for inconvenience, will fix the string compare as soon as possible and then tackle allowing yaml int array after.

[jhawkesworth]

That seems to fix the string compare problem, preparing pull requests now.

[dreamcat4]

thanks man! look forward to trying it out.

[jhawkesworth]

if you update to latest devel for extras the change is now in place.

[jhawkesworth]

@dreamcat4 for info, updated integration tests PR is here: ansible/ansible#15570

[dreamcat4]

OK @jhawkesworth. I can independantly confirm that the fix works for data: "" (empty string) values. I really appreciate this - thank you.

Just remaining the old binary format [0x01,0x02, ...] still broken.

[jhawkesworth]

Thanks for testing so quickly

I have raised #2090 to cover the yaml format no longer working.

I will attempt to get a fix in for this but not yet sure what the fix might actually be, and 2.1 is on the verge of code freeze.

[dreamcat4]

OK cool no problem. Have now already switched to new hex: format. For all my tasks. Won't be adversely affected if it misses the freeze. Subscribed to track on other issue.