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

New module: win dsc #24872

Merged
merged 15 commits into from
Jun 1, 2017
Merged

New module: win dsc #24872

merged 15 commits into from
Jun 1, 2017

Conversation

trondhindenes
Copy link
Contributor

SUMMARY

New module: win_dsc

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

win_dsc

ANSIBLE VERSION
2.4.0
ADDITIONAL INFORMATION

This module has been sitting in a personal repo, but as its becoming popular it makes sense to add it to Ansible core.
The module allows execution of DSC resources, and takes a dynamic range of parameter depending on the underlying DSC resource.

@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. test_pull_requests windows Windows community labels May 21, 2017
@ansibot
Copy link
Contributor

ansibot commented May 21, 2017

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/modules/windows/win_dsc.py:30:161: E501 line too long (162 > 160 characters)
lib/ansible/modules/windows/win_dsc.py:32:161: E501 line too long (186 > 160 characters)

The test ansible-test sanity --test validate-modules failed with the following errors:

lib/ansible/modules/windows/win_dsc.py:0:0: E312 No RETURN provided
lib/ansible/modules/windows/win_dsc.py:0:0: E314 No ANSIBLE_METADATA provided

The test ansible-test sanity --test yamllint failed with the following errors:

test/integration/targets/win_dsc/tasks/main.yml:26:1: syntax error: expected <block end>, but found '?'
test/integration/targets/win_dsc/tasks/main.yml:112:1: too many blank lines (2 > 0) (empty-lines)

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels May 21, 2017
@ansibot
Copy link
Contributor

ansibot commented May 21, 2017

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/modules/windows/win_dsc.py:34:104: W291 trailing whitespace
lib/ansible/modules/windows/win_dsc.py:35:95: W291 trailing whitespace
lib/ansible/modules/windows/win_dsc.py:37:122: W291 trailing whitespace
lib/ansible/modules/windows/win_dsc.py:69:12: W291 trailing whitespace

The test ansible-test sanity --test validate-modules failed with the following errors:

lib/ansible/modules/windows/win_dsc.py:0:0: E319 RETURN.DSCAttributes.contains: required key not provided @ data['contains']. Got None
lib/ansible/modules/windows/win_dsc.py:0:0: E319 RETURN.message.returned: expected str for dictionary value @ data['returned']. Got None
lib/ansible/modules/windows/win_dsc.py:0:0: E319 RETURN.message.type: required key not provided @ data['type']. Got None

The test ansible-test sanity --test yamllint failed with the following error:

test/integration/targets/win_dsc/tasks/main.yml:25:1: syntax error: expected <block end>, but found '?'

click here for bot help

@trondhindenes
Copy link
Contributor Author

trondhindenes commented May 21, 2017

Looking closer at the tests, seems we don't have any targets with WMF5.0 or 5.1 on them.

@ansibot
Copy link
Contributor

ansibot commented May 21, 2017

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/modules/windows/win_dsc.py:34:104: W291 trailing whitespace
lib/ansible/modules/windows/win_dsc.py:35:95: W291 trailing whitespace
lib/ansible/modules/windows/win_dsc.py:37:122: W291 trailing whitespace
lib/ansible/modules/windows/win_dsc.py:69:12: W291 trailing whitespace

The test ansible-test sanity --test validate-modules failed with the following errors:

lib/ansible/modules/windows/win_dsc.py:0:0: E319 RETURN.DSCAttributes.contains: required key not provided @ data['contains']. Got None
lib/ansible/modules/windows/win_dsc.py:0:0: E319 RETURN.message.returned: expected str for dictionary value @ data['returned']. Got None
lib/ansible/modules/windows/win_dsc.py:0:0: E319 RETURN.message.type: required key not provided @ data['type']. Got None

click here for bot help

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 21, 2017
@daBONDi
Copy link
Contributor

daBONDi commented May 21, 2017

I like it if it gets put in, use it quite often!
Thx for the Work trondhindenes!

@trondhindenes
Copy link
Contributor Author

Thanks @daBONDi , it was about time to get this cleaned up and PRed in ;-)

#Set-StrictMode -Off

$params = Parse-Args $args -supports_check_mode $true
$result = New-Object psobject
Copy link
Contributor

Choose a reason for hiding this comment

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

We are using normal hashtables these days for the result object.

$result = @{
    changed= $false
}

$CheckMode = $True
}

}
Copy link
Contributor

@dagwieers dagwieers May 21, 2017

Choose a reason for hiding this comment

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

We use this nowadays:

$check_mode = Get-AnsibleParam -obj $params -name "_ansible_check_mode" -type "bool" -default $false

It helps if we can keep the same statements/variable naming for the different modules.


}

$resourcename = Get-Attr -obj $params -name resource_name -failifempty $true -resultobj $result
Copy link
Contributor

Choose a reason for hiding this comment

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

Also use Get-AnsibleParam here.

#Always return the name
set-attr -obj $result -name "resource_name" -value $resourcename
set-attr -obj $result -name "attributes" -value $Attributes
set-attr -obj $result -name "reboot_required" -value $null
Copy link
Contributor

Choose a reason for hiding this comment

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

These could be simple assignments:

$result['resource_name'] = $resourcename
$result['attributes'] = $Attributes
$result['reboot_required'] = $null

the ansible parameters would be cred_username and cred_password.
These will be used to inject a credential object on the fly for the DSC resource.
options:
resource_name:
Copy link
Contributor

@dagwieers dagwieers May 21, 2017

Choose a reason for hiding this comment

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

I would simply call this name, but keep resource or resource_name as alias. (Maybe this is not possible because of conflicts in that namespace ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think "name" is ambigous, as there might also be a "name" param sent into the module (which we rewrite as stated in the docs). I think it's best to avoid "name" altogether on this - and there's a very good chance someone down the line suggests to allow the "name" parameter to be passed in to the underlying dsc resource without rewriting it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok.

path="C:\Temp\zipfile.zip"
destination="C:\Temp\Temp2"
'''

Copy link
Contributor

Choose a reason for hiding this comment

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

The windowsfeature example from the integration tests belongs in this section too. And to be honest, I think we may need a complete section in the documentation related to DSC, real-life examples, etc... This will become the main bulk of what people will be doing with Ansible on Windows going forward.

A full library of common tasks (alternatives to Unix modules) would be very welcome. Something users can easily add examples to, or annotate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I was thinking about that - we'll probably need a separate doc page related to dsc. I'll ask @nitzmahone what he thinks. In the meantime I'll incorporate your suggestions, thanks for reviewing!

Copy link
Contributor

@dagwieers dagwieers May 21, 2017

Choose a reason for hiding this comment

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

It's on the agenda for London, so I am confident that once we have decided about the plan, we can collaborate quickly on the implementation. Exciting times !

@ansibot ansibot removed the community_review In order to be merged, this PR must follow the community review workflow. label May 21, 2017
@dagwieers
Copy link
Contributor

I don't think it works that way, you need 2 shipits from maintainers for it to be merged automatically. And for new modules that's not possible. Unless we add ourselves in advance to the MAINTAINERS document ;-)

@dagwieers
Copy link
Contributor

bot_status

@ansibot
Copy link
Contributor

ansibot commented May 31, 2017

waiting_on: maintainer
module: win_dsc
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
mergeable_state: clean
shippable_status: success
maintainer_shipits: 0
community_shipits: 2
ansible_shipits: 0
shipit_actors: dagwieers jborean93

click here for bot help

@trondhindenes
Copy link
Contributor Author

Just reading about the process on https://github.com/ansible/community/blob/master/PR-FLOW.md, but it doesn't completely line up with how the bot labels things. Not sure if I should ping @nitzmahone , just merge it myself (having received two shipits) or something else. Can't wait to squash this madness :-)

@dagwieers
Copy link
Contributor

@trondhindenes I've been adding some ideas to the AnsibleFest London EtherPad wrt. contributions and automerging, if you have ideas of improving the workflow as well, share them :-)

2000 open issues and almost 1200 PRs is not looking good :-(

@nitzmahone nitzmahone merged commit 055fd6f into ansible:devel Jun 1, 2017
@nitzmahone
Copy link
Member

Yeah, I think we're at the point that we need to just get it out there for folks to start playing with- I'm sure there will be things to change down the line, but hopefully this can solve some pain for folks that have access to DSC resources that do things Ansible doesn't.

@trondhindenes
Copy link
Contributor Author

sweet, thanks guys! Big step!

@nitzmahone
Copy link
Member

Thank YOU! Stoked to get it shipped!

@dagwieers
Copy link
Contributor

If we finish everything we planned to do at AnsibleFest in London, we can drink MORE beer !

@jhawkesworth
Copy link
Contributor

Delighted this is in! Now need big excuse to start using it.

@wernerb
Copy link

wernerb commented Jun 2, 2017

I am seeing an error with a basic Dsc Resource "File":

- name: blabla
  win_dsc5:
    resource_name: File
    ensure: Present
    destinationpath: |-
      C:\example.txt
    Contents: |
      My content

Output:

Friday 02 June 2017  14:12:45 +0200 (0:00:03.155)       0:00:03.224 ***********
Using module file /Users/werner/projects/nn/aws-platform/ansible/main/playbooks/library/win_dsc5.ps1
<127.0.0.1> ESTABLISH WINRM CONNECTION FOR USER: Administrator on PORT 5986 TO 127.0.0.1
EXEC (via pipeline wrapper)
fatal: [default]: FAILED! => {
    "Attributes": [
        {
            "Key": "ensure",
            "Name": "ensure",
            "Value": "Present"
        },
        {
            "Key": "Contents",
            "Name": "Contents",
            "Value": "My content\n"
        },
        {
            "Key": "destinationpath",
            "Name": "destinationpath",
            "Value": "C:\\example.txt"
        }
    ],
    "Contents": "My content\n",
    "DSCAttributes": {
        "Contents": "My content\n",
        "destinationpath": "C:\\example.txt",
        "ensure": "Present"
    },
    "changed": false,
    "destinationpath": "C:\\example.txt",
    "ensure": "Present",
    "failed": true,
    "resource_name": "File"
}

MSG:

Cannot validate argument on parameter 'ModuleName'. The argument is null or empty. Provide an argument that is not null or empty, and then try the command again.

The problem seems to be that there are DscResources that have no module. For File it is empty?

@trondhindenes
Copy link
Contributor Author

File is weird, it has no module. Wont fix. Ill add it the documentation

@dagwieers
Copy link
Contributor

I guess that if we're going to add a DSC section to the windows documentation, it's time to start splitting up the Windows docs. It's already one huge document. I'll add it to the WWG agenda.

@wernerb
Copy link

wernerb commented Jun 2, 2017

@trondhindenes I have a patch that allows you to optionally setmodule_name to override. A different case that this also fixes is if you have a resourcename that exists in multiple modules. In both situations you can then supply the correct module name. I will make a pull request in your original repository, and is it welcome here?

An example:

- name: Configure IIS/Telnet Client
  win_dsc5:
    resource_name: WindowsFeature
    ensure: Present
    item_name: "Web-server"

- name: blabla
  win_dsc5:
    module_name: PSDesiredStateConfiguration
    resource_name: File
    ensure: Present
    destinationpath: |-
      C:\example.txt
    Contents: |
      My content

@trondhindenes
Copy link
Contributor Author

Good point wernerb. Please submit pr to this repo, I won't maintain the "private" version any longer (i'll add some text regarding that in that repo)

@cwegener
Copy link

cwegener commented Jun 3, 2017

I just got here via Trond's message on the old repo. :-)

So glad that this is now merged into ansible. I'm quite a heavy user of this module.

I'll have some time to test all my playbooks with this merged version of the module.

I hope I won't have to open too many issues. ;-) 2000 open issues is quite a lot.

@dagwieers
Copy link
Contributor

@cwegener Can we use your expertise for helping with the DSC documentation ?

We would like to have a section, either in the docs or on a wiki, with real-world use-cases, examples and tips-and-tricks to make this attractive to Windows admins. Good quality roles we can point to would be an asset too.

@trondhindenes trondhindenes deleted the newmodule_win_dsc branch June 9, 2017 22:42
@cwegener
Copy link

@dagwieers I'd be happy to help with documentation. I'm not sure what the timeframe for the 2.4 release is though (assuming that the DSC documentation should be ready for the release). Depending on what the timeframe is, I can commit to writing up some use cases and examples. I do have a couple of SQL Server playbooks for ansible+dsc that I should be able to share as an example.

@jborean93
Copy link
Contributor

@cwegener I'm not sure what @trondhindenes had planned with this but it may be useful to add some of your examples or whatever you think is relevant to https://github.com/ansible/community/wiki/Windows%3A-documentation. That way we can collate it all at the end or add it to the eventual PR that will be there.

@jhawkesworth
Copy link
Contributor

@cwegener
Copy link

cwegener commented Aug 3, 2017

@jborean93 Thanks for the link to the wiki. That sounds like a good starting point. I'm pretty new at contributing documentation to ansible. I will need to figure out how to use Github Wikis though. I've never used them before.
@jhawkesworth Looking at the dates for 2.4, I will certainly be able to make some time in my schedule prior to release to document some win_dsc examples. I will start with sharing my ideas/suggestions on the wiki page.

@trondhindenes
Copy link
Contributor Author

I have a ton of examples from our production code aswell, happy to contribute.

@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.4 This issue/PR affects Ansible v2.4 module This issue/PR relates to a module. new_module This PR includes a new module. new_plugin This PR includes a new plugin. shipit This PR is ready to be merged by Core windows Windows community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants