-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Conversation
…should not be more complex
e3f9790
to
7c8e3f4
Compare
abd961c
to
7978afe
Compare
Greetings, @h0nIg -- Thanks for submitting this new module to Ansible Extras! This module is now in community review, a process that is open to all Ansible users. In order for this module to be approved, it must gain the following votes, which can be contributed by anyone other than the author of the new module: “works_for_me”: If you have tested the module thoroughly, including testing of all of the module’s options, and if the module works for you, please add “works_for_me” in the comments. “passes_guidelines”: If you have gone through the module guidelines and the module meets all of the requirements, please add “passes_guidelines” in the comments. Guidelines are available here: http://docs.ansible.com/developing_modules.html#module-checklist “needs_revision”: If the module fails to work for you, or if it doesn’t meet guidelines, please add “needs_revision” in the comments with details about what needs to be fixed. When a module has both “works_for_me” and “passes_guidelines” tags, we will promote the module for inclusion in Ansible Extras. At this point, you will be expected to maintain the module by fixing bugs and evaluating pull requests in a timely manner. More information about the review process can be seen here: Thanks again for submitting your Ansible module! |
As with #1119 -- Pinging @schwartzmx & @trondhindenes for feedback as the win_acl module owners, who may have feedback here. And pinging the rest of our Windows crew for thoughts as well: @petemounce @elventear @smadam813 @jhawkesworth @angstwad @sivel @chrishoffman @cchurch |
|
||
$path = Get-Attr $params "path" -failifempty $true | ||
$user = Get-Attr $params "user" -failifempty $true | ||
$recurse = Get-Attr $params "recurse" "no" -validateSet "no","yes" -resultobj $result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to use ConvertTo-Bool
so that the value of $recurse
is a bool
4aadd63
to
45249fb
Compare
Thanks @h0nIg -- pinging windows crew for re-review. @trondhindenes @petemounce @elventear @smadam813 @jhawkesworth @angstwad @sivel @chrishoffman @cchurch |
I think Windows file ownerships and acls are too complex to handle in a single module. passes_guidelines |
…ined variables fixed variable capitalization
fe5e4e5
to
862fb71
Compare
862fb71
to
d5ab52e
Compare
i would appreciate the inclusion. works_for_me ping: @trondhindenes @petemounce @elventear @smadam813 @jhawkesworth @angstwad @sivel @chrishoffman @cchurch |
@h0nIg See if you can squash the commits into one. Looks like this already has 1 works_for_me and 1 passes_guidelines, so should be marked for inclusion. @robynbergeron ^^ |
@Jmainguy checkout the "Files changed" tab at the top. there you can see the full changeset |
|
||
$path = Get-Attr $params "path" -failifempty $true | ||
$user = Get-Attr $params "user" -failifempty $true | ||
$recurse = Get-Attr $params "recurse" "no" -validateSet "no","yes" -resultobj $result | ConvertTo-Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue here as with win_acl_inheritance- the ConvertTo-Bool eats the validation error if you pass something other than yes/no
@h0nIg looks good- just that one little issue and we're good to go. For future, check-mode support would be a good addition too. Thanks! |
@h0nIg A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. |
added suggestion by @nitzmahone . this should be ready for inclusion :) |
@h0nIg - I have been looking at the PR review robot code here: https://github.com/ansible/ansibullbot/blob/master/prbot.py and I think you need to put 'ready_for_review' in the comments to get this progressing through the PR inclusion process described here: https://github.com/ansible/ansible-modules-extras/blob/devel/REVIEWERS.md). I think only the PR submitter can do this. |
ready_for_review |
Thanks @h0nIg for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion. [This message brought to you by your friendly Ansibull-bot.] |
@gregdek this pull request already received two passes_guidlines and two works_for_me. just @nitzmahone asked for a minor improvement related to documentation / input variables. could you please merge this, since people are not reacting and its hard to get "shipit" again (which was equal to passes_guidlines / works_for_me before "Ansibull-bot") |
Thanks for the updates- sold! |
added separate module to change owner, since win_acl is ACL only and should not be more complex