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

Select a reference Windows module for the documentation #20505

Closed
dagwieers opened this issue Jan 20, 2017 · 8 comments · Fixed by #30588
Closed

Select a reference Windows module for the documentation #20505

dagwieers opened this issue Jan 20, 2017 · 8 comments · Fixed by #30588
Labels
affects_2.3 This issue/PR affects Ansible v2.3 docs This issue/PR relates to or includes documentation. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community. windows Windows community

Comments

@dagwieers
Copy link
Contributor

dagwieers commented Jan 20, 2017

ISSUE TYPE
  • Documentation Report
COMPONENT NAME

win_environment

ANSIBLE VERSION

v2.4

SUMMARY

As discussed on IRC, let's declare one module the reference module for Windows and use it in the documentation for new module authors.

The module should have all these features:

  • must be small in business logic
  • must be idempotent
  • must support check-mode
  • must include -validateset
  • must include -failifempty
  • must include -default
  • must include a bool-type parameter
  • should include a path-type parameter
  • should include a string-type parameter

The win_service module was our first candidate, but it has turned into a more complex example. It does meet all requirements. So we decided to use win_environment as the reference module.

The rewrite of this module and the expected bikeshedding will be done during the review-process in the PR. The PR will also include:

  • Include plenty of annotations to guide the new contributor
  • Ensure the module has a consistent CodingStyle that we prefer most (but we do not enforce it) @jborean93
  • We update this module every time we improve our standards, so that new and existing maintainers can track new standards
@dagwieers
Copy link
Contributor Author

@jhawkesworth @nitzmahone @dav1x @jctanner As discussed on IRC.

@ansibot ansibot added affects_2.3 This issue/PR affects Ansible v2.3 docs_report needs_triage Needs a first human triage before being processed. labels Jan 20, 2017
@dagwieers dagwieers changed the title Use a reference Windows module for the documentation Select a reference Windows module for the documentation Jan 20, 2017
@jhawkesworth
Copy link
Contributor

mention @ilpianista who also mentioned on IRC

some other (unsorted) thoughts

what to put for metadata?
link to jimic's presentation(s) on writing modules?
a note about | out-null so you don't put unintended objects into the output pipelines (therefore putting cruft into the generated json.
avoid feeding trailing slashes on strings into ConvertTo-Json 'cos it makes it fail
link to how to run existing integration tests against your test windows machines/vms

@jctanner jctanner removed the needs_triage Needs a first human triage before being processed. label Jan 20, 2017
@gundalow
Copy link
Contributor

Keep me in the loop, will help with the developing_modules.html rewrite that's going on.

@jhawkesworth
Copy link
Contributor

jhawkesworth commented Jan 20, 2017

more thoughts...

need to support powershell 3 and above
links to powershell references

document return values
conventions for var names
conventions for return values
check with linux equivalents to make consistent names where possible, but only if it makes sense

advanced topics:

example of calling .net class
example of embedding c# code

dagwieers added a commit to dagwieers/ansible that referenced this issue Jan 28, 2017
As discussed in ansible#20505 we would like to point from the documentation to
a module we choose as the reference module for Windows.

I picked win_service and cleaned it up to the best of my abilities.
I also added check-mode support as an example.

Let the bikeshedding begin !
dagwieers added a commit to dagwieers/ansible that referenced this issue Feb 20, 2017
As discussed in ansible#20505 we would like to point from the documentation to
a module we choose as the reference module for Windows.

I picked win_service and cleaned it up to the best of my abilities.
I also added check-mode support as an example.

Let the bikeshedding begin !
@dagwieers
Copy link
Contributor Author

So we have 3 candidates, and win_service is no longer on the table :-)

[dag@moria ansible.git]$ grep -l -- '-type "str"' $(grep -l -- '-type "path"' $(grep -l -- '-type "bool"' $(grep -l default $(grep -l failifempty $(grep -l 'validateset' $(grep -rl '$check_mode' lib/ansible/modules/windows/))))))
lib/ansible/modules/windows/win_lineinfile.ps1
lib/ansible/modules/windows/win_owner.ps1
lib/ansible/modules/windows/win_uri.ps1

Of these, win_uri is clearly the most simple example, although it does have some things that require cleaning up.

  • headers should be a dict (implement a dict-type !)
  • fix the idempotency issue when writing to a file
  • make it somewhat more feature complete with the uri module
  • reorganize result
  • add integration tests
  • update documentation

So quite some work before we can consider it first-of-class.

@dagwieers
Copy link
Contributor Author

It has been decided to use win_environment as it has the most simple implementation of an idempotent module supporting check-mode and diff support.

The upside is that we don't need to do a lot of work ;-)

@ansibot ansibot added module This issue/PR relates to a module. windows Windows community labels Jun 6, 2017
@ansibot ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label Jun 29, 2017
@jhawkesworth
Copy link
Contributor

At 19:00 UTC on Tuesday 19 September 2017 this ticket will be processed during the second Windows Sprint,
https://github.com/ansible/community/wiki/Windows:-sprints
we would appreciate if you could join the Sprint to help out with understanding/fixing/closing this issue.

@dagwieers
Copy link
Contributor Author

dagwieers commented Sep 19, 2017

We looked into this issue today, and two things are required moving forward:

  1. We need to update the win_environment module to follow the latest conventions done
  2. We need to update the docs to point at this module as the reference module done

@ansibot ansibot added docs This issue/PR relates to or includes documentation. and removed docs_report labels Mar 1, 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.3 This issue/PR affects Ansible v2.3 docs This issue/PR relates to or includes documentation. 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
None yet
Development

Successfully merging a pull request may close this issue.

5 participants