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_dsc: DSC improvements #31003

Closed
jborean93 opened this issue Sep 27, 2017 · 12 comments
Closed

win_dsc: DSC improvements #31003

jborean93 opened this issue Sep 27, 2017 · 12 comments
Labels
affects_2.5 This issue/PR affects Ansible v2.5 bug This issue/PR relates to a bug. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community. windows Windows community
Projects
Milestone

Comments

@jborean93
Copy link
Contributor

jborean93 commented Sep 27, 2017

ISSUE TYPE
  • Bug Report
  • Feature Idea
  • Documentation Report
COMPONENT NAME

win_dsc

ANSIBLE VERSION
2.5
CONFIGURATION

default

OS / ENVIRONMENT

Windows

SUMMARY

This is not an issue but a way to documenting some of the improvements we can add to the DSC module. When going through the docs rewrite a few things came up which I thought would be good to do with this module. It should hopefully allow more use of the module in more scenarios and allow more advanced features like definitions in definitions.

Some of the improvements I can see happening are:

  • Create documentation page to demonstrate how it works (In Progress: update Windows docs #30783)
  • Further improve the documentation on the module page
  • Allow nested definitions inside a DSC resource (see below)
  • Document an example that uses [CimInstance[]], are they just hashtables?
  • Allow array types to be defined as a list and not just a comma separated string
  • From Support for UInt64 in win_dsc module #31502 - Automatic type conversion instead of all the if statements.

These may already work and I am just missing something but these are questions I've had when doing the documentation

Nested Definitions

One area that I feel would be really good to have is nested definitions. Using the xWebAdministration example this is how an IIS website can be defined with binding

xWebsite NewWebsite
{
    Ensure          = "Present"
    Name            = $WebSiteName
    State           = "Started"
    PhysicalPath    = $DestinationPath
    BindingInfo     = @(
        MSFT_xWebBindingInformation
        {
            Protocol              = "HTTPS"
            Port                  = 8443
            CertificateThumbprint = "71AD93562316F21F74606F1096B85D66289ED60F"
            CertificateStoreName  = "WebHosting"
        },
        MSFT_xWebBindingInformation
        {
            Protocol              = "HTTPS"
            Port                  = 8444
            CertificateThumbprint = "DEDDD963B28095837F558FE14DA1FDEFB7FA9DA7"
            CertificateStoreName  = "MY"
        }
    )
    DependsOn       = "[File]WebContent"
}

The binding info has 2 definitions in an array for xWebBindingInformation which is another DSC resource. Being able to specify both at the same time will reduce the number of tasks required and help avoid conflicts. The tricky part is how to define it in YAML and convert it across.

@jborean93 jborean93 added this to the 2.5.0 milestone Sep 27, 2017
@ansibot
Copy link
Contributor

ansibot commented Sep 27, 2017

@ansibot ansibot added affects_2.5 This issue/PR affects Ansible v2.5 bug_report module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. windows Windows community labels Sep 27, 2017
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Sep 27, 2017
@jborean93 jborean93 added this to TODO in 2.5 Sep 27, 2017
@jborean93 jborean93 changed the title win_dsc: DSC improvemens win_dsc: DSC improvements Sep 28, 2017
@trondhindenes
Copy link
Contributor

Nice, thanks for keeping improving on dsc! I'm swamped at work so unfortunately not able to do much with Ansible atm. Just wanted to comment on the nested definitions bit: As far as I can see the big hurdle is how we deserialize yaml/json into valid types such as "MSFT_xWebBindingInformation". The codebase already has some fairly ugly parsers to handle the most standard stuff, but we should definetely look at how we could improve on this - fresh set of eyes very welcome.

Maybe we could introduce a scheme where the yaml actually specifies the type of object that it should deserialize to so that we wouldn't have to guess at run-time would be an option. It would certainly be a bit more "advanced" than today's fairly straight-forward usage, but that shouldn't stop us I guess.

@jborean93
Copy link
Contributor Author

Thanks for the comments @trondhindenes much appreciated. I am planning on looking at the module for 2.5 and to see what can be done but it is early days for me. One way I was thinking was to define it like

- win_dsc:
    BindingInfo:
    - xWebInformationBinding:
        Protocol: HTTPS
        Port: 8443
        CertificateThumbprint:  71AD93562316F21F74606F1096B85D66289ED60F
        CertificateStoreName: WebHosting
    - xWebInformationBinding: ...

I would have to test it out and actually see what the requirements are but any other ideas would be good to have.

@daBONDi
Copy link
Contributor

daBONDi commented Sep 29, 2017

Maybe make a new module like win_dsc_template for execution

If someone need to do a heavy lift with an epic dsc doc he can use that and have the features of using jinja2 maybe, like a combination of win_dsc and win_template :-)

And for single DSC Resource Executions win_dsc is enough i think


Because of Type Information maybe win_dsc implement something like

[Type]Property : Value

So there is no guess work anymore, only decend type parsers need to be written to Fetch out [Type] from Property Name and parse its Value

Like in your Example

[MSFT_xWebBindingInformation[]]BindingInfo:
  - Protocol: "HTTPS"
    Port: 8443
    CertificateThumbprint: "71AD93562316F21F74606F1096B85D66289ED60F"
    CertificateStoreName: "WebHosting"
  - Protocol: "HTTPS"
    Port: 8444
    CertificateThumbprint: "DEDDD963B28095837F558FE14DA1FDEFB7FA9DA7"
    CertificateStoreName: "MY"

@ansibot
Copy link
Contributor

ansibot commented Oct 7, 2017

@dagwieers
Copy link
Contributor

dagwieers commented Oct 7, 2017

YAML has already one way to indicate type using !type syntax. That would be my preferred way, but requires support inside Ansible (rather than just working around it by using keywords or values for this).
https://github.com/dlang-community/D-YAML/wiki/Custom-YAML-data-types

@jborean93
Copy link
Contributor Author

@trondhindenes, do you have an example of someone using win_dsc that takes in a CimInstance[]. I'm trying to understand the code and it is a tiny bit confusing how it converts the string.

@jborean93 jborean93 moved this from TO Prioritize to In Progress in 2.5 Oct 12, 2017
@trondhindenes
Copy link
Contributor

trondhindenes commented Oct 17, 2017

Hm, good question. The type parsers got a few PRs while win_dsc was in my personal repo, so I'm afraid not all of the code is mine.
The original CimInstance PR has some example code, does that help?:
trondhindenes/Ansible-win_dsc#2

@jborean93
Copy link
Contributor Author

Thanks it does indeed, very un-yaml like and I don't think we should continue. Wary that the 2.4 module would have this support but will add to the working group agenda to see if we want to break that compatibility considering it isn't documented and can be done an easier way.

@dagwieers
Copy link
Contributor

I am happy to break this if we can backport this as well. The only alternative is that we deprecate the compatibility-layer, which is going to be a drag :-/

@jborean93
Copy link
Contributor Author

jborean93 commented Nov 5, 2017

The PR #31556 has been merged which adds in the majority of these features. PR to update the main documentation page has been raised at #32581

@jborean93 jborean93 moved this from In Progress to Done in 2.5 Nov 9, 2017
@jborean93
Copy link
Contributor Author

Features have been implemented and docs merged in, closing this issue.

@ansibot ansibot added bug This issue/PR relates to a bug. and removed bug_report labels Mar 7, 2018
@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.5 This issue/PR affects Ansible v2.5 bug This issue/PR relates to a bug. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community. windows Windows community
Projects
No open projects
2.5
Done
Development

No branches or pull requests

5 participants