{"action":"created","comment":{"url":"https://api.github.com/repos/ansible/ansible-modules-core/pulls/comments/37104726","id":37104726,"diff_hunk":"@@ -151,69 +145,87 @@ def split_entry(entry):\n     else:\n         t = None\n \n-    p = normalize_permissions(p)\n+    return [d, t, e, p]\n \n-    return [d,t,e,p]\n \n-def get_acls(module,path,follow):\n+def build_entry(etype, entity, permissions=None):\n+    '''Builds and returns an entry string. Does not include the permissions bit if they are not provided.'''\n+    return etype + ':' + entity + (':' + permissions if permissions else '')\n \n-    cmd = [ module.get_bin_path('getfacl', True) ]\n-    if not follow:\n-        cmd.append('-h')\n-    # prevents absolute path warnings and removes headers\n-    cmd.append('--omit-header')\n-    cmd.append('--absolute-names')\n-    cmd.append(path)\n \n-    return _run_acl(module,cmd)\n+def build_command(module, mode, path, follow, default, recursive, entry=''):\n+    '''Builds and returns agetfacl/setfacl command.'''\n+    if mode == 'set':\n+        cmd = [module.get_bin_path('setfacl', True)]\n+        cmd.append('-m \"%s\"' % entry)\n+    elif mode == 'rm':\n+        cmd = [module.get_bin_path('setfacl', True)]\n+        cmd.append('-x \"%s\"' % entry)\n+    else:  # mode == 'get'\n+        cmd = [module.get_bin_path('getfacl', True)]\n+        cmd.append('--omit-header')","path":"files/acl.py","position":81,"original_position":81,"commit_id":"ae1126c217bc48847e35311785ede1011d5e544f","original_commit_id":"ae1126c217bc48847e35311785ede1011d5e544f","user":{"login":"puiterwijk","id":967561,"avatar_url":"https://avatars.githubusercontent.com/u/967561?v=3","gravatar_id":"","url":"https://api.github.com/users/puiterwijk","html_url":"https://github.com/puiterwijk","followers_url":"https://api.github.com/users/puiterwijk/followers","following_url":"https://api.github.com/users/puiterwijk/following{/other_user}","gists_url":"https://api.github.com/users/puiterwijk/gists{/gist_id}","starred_url":"https://api.github.com/users/puiterwijk/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/puiterwijk/subscriptions","organizations_url":"https://api.github.com/users/puiterwijk/orgs","repos_url":"https://api.github.com/users/puiterwijk/repos","events_url":"https://api.github.com/users/puiterwijk/events{/privacy}","received_events_url":"https://api.github.com/users/puiterwijk/received_events","type":"User","site_admin":false},"body":"You might want to re-add the warning: # prevents absolute path warnings and removes headers","created_at":"2015-08-14T18:24:36Z","updated_at":"2015-08-14T18:24:36Z","html_url":"https://github.com/ansible/ansible-modules-core/pull/150#discussion_r37104726","pull_request_url":"https://api.github.com/repos/ansible/ansible-modules-core/pulls/150","_links":{"self":{"href":"https://api.github.com/repos/ansible/ansible-modules-core/pulls/comments/37104726"},"html":{"href":"https://github.com/ansible/ansible-modules-core/pull/150#discussion_r37104726"},"pull_request":{"href":"https://api.github.com/repos/ansible/ansible-modules-core/pulls/150"}}},"pull_request":{"url":"https://api.github.com/repos/ansible/ansible-modules-core/pulls/150","id":22329211,"html_url":"https://github.com/ansible/ansible-modules-core/pull/150","diff_url":"https://github.com/ansible/ansible-modules-core/pull/150.diff","patch_url":"https://github.com/ansible/ansible-modules-core/pull/150.patch","issue_url":"https://api.github.com/repos/ansible/ansible-modules-core/issues/150","number":150,"state":"open","locked":false,"title":"Add recursive support for ACL module","user":{"login":"astorije","id":113730,"avatar_url":"https://avatars.githubusercontent.com/u/113730?v=3","gravatar_id":"","url":"https://api.github.com/users/astorije","html_url":"https://github.com/astorije","followers_url":"https://api.github.com/users/astorije/followers","following_url":"https://api.github.com/users/astorije/following{/other_user}","gists_url":"https://api.github.com/users/astorije/gists{/gist_id}","starred_url":"https://api.github.com/users/astorije/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/astorije/subscriptions","organizations_url":"https://api.github.com/users/astorije/orgs","repos_url":"https://api.github.com/users/astorije/repos","events_url":"https://api.github.com/users/astorije/events{/privacy}","received_events_url":"https://api.github.com/users/astorije/received_events","type":"User","site_admin":false},"body":"This PR enables the support for recursive ACLs. Some refactoring was needed to make this work. It is the new repo structure augmented clone of ansible/ansible#8380.\r\n\r\nIt fixes ansible/ansible#5053, ansible/ansible#5550 and fix ansible/ansible#7276) as well as a few things along the way:\r\n\r\n- PEP8 compliance(-ish)\r\n- Fix erroneous entry param `etype` to `type`\r\n- Param checking messages use RFC 2119 wording)\r\n- Add checks for incompatible states\r\n\r\nI did not specify the version in the documentation bit. I will add it when this is merged to have consistent docs.\r\n\r\n## Advantages of this PR\r\n\r\n- Permissions do not need to be normalized anymore using the `--test` checking system\r\n\r\n- `--test` does not check using `getfacl`, so the code is less verbose and less likely to introduce a parsing error (since there is actually - almost - no parsing)\r\n\r\n- `--test` makes `--recursive` possible at no performance cost, and checking the `changed` status is neither overly complicated or slow\r\n\r\n- `default` can now be used on acl queries as well\r\n\r\n- [`X` (`capital-x` / *Special execute*)](https://en.wikipedia.org/wiki/Chmod#Symbolic_modes) is now supported in the permissions with no extra code\r\n\r\n## Inconvenients of this PR\r\n\r\n- `--test` does not seem to be supported by BSD's `setfacl`. However, this can be solved in the `acl_changed` function only without affecting the rest of the file. Suggestions below.\r\n\r\n- `recursive=yes` and `state=query` are currently incompatible because that needs further discussion to ensure backward compatibility. This should be done in another issue/PR though. I will be more than happy to work on it, but one step at a time :)\r\n\r\n## General notes (and motivations) to help for the review\r\n\r\n- `normalize_permissions` did not support `X` (`capital-x`) and its support would cause problem with the former checking system (since `X` itself is not stateless)\r\n\r\n- `_run_acl` was making the usage of `--test` impossible because for the last line returned when using `--test` is relevant and not empty. It now makes sure that the last line is empty before stripping it.\r\n\r\n- 2 new builder helpers `build_entry` and `build_command` now factorize some recurring code\r\n\r\n- `acl_changed` is a function meant to test if a command changes ACLs. The checking can be extended there rather than in the `main` function\r\n\r\n- I removed the `get_acls`, `set_acl` and `rm_acl` wrappers for the following reasons:\r\n    - They were used only once each\r\n    - They only contained one call to `build_command` and one call to `run_acl` in the end\r\n    - The command is already built when it is `--test`ed before so I reuse the command rather than rebuilding it\r\n\r\n- `_run_acl` is now `run_acl` because it is not a hidden/private function anymore, it is used in the `main` function multiple times\r\n\r\n## Suggestions for Linux / BSD support\r\n\r\nMy PR breaks support for BSD because of its lack of `--test`, but adds support for `--recursive` on Linux. I suggest that when `--test` is not available, `changed` is **always** true when using `recursive=yes`. When not using `recursive=yes`, we can either do the same or replicate the old code for checking state. What I described earlier (notably the `X` bit) will still be a problem. I'll talk with @bcoca on IRC about that.\r\n\r\nThe documentation should then mention the support differences if there are  any (for example no support for `recursive` on BSD).","created_at":"2014-10-07T03:35:16Z","updated_at":"2015-08-14T18:24:36Z","closed_at":null,"merged_at":null,"merge_commit_sha":"15a9c981976d2cb085d8e8c04126ecef956e4040","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/ansible/ansible-modules-core/pulls/150/commits","review_comments_url":"https://api.github.com/repos/ansible/ansible-modules-core/pulls/150/comments","review_comment_url":"https://api.github.com/repos/ansible/ansible-modules-core/pulls/comments{/number}","comments_url":"https://api.github.com/repos/ansible/ansible-modules-core/issues/150/comments","statuses_url":"https://api.github.com/repos/ansible/ansible-modules-core/statuses/ae1126c217bc48847e35311785ede1011d5e544f","head":{"label":"astorije:recursive_acl","ref":"recursive_acl","sha":"ae1126c217bc48847e35311785ede1011d5e544f","user":{"login":"astorije","id":113730,"avatar_url":"https://avatars.githubusercontent.com/u/113730?v=3","gravatar_id":"","url":"https://api.github.com/users/astorije","html_url":"https://github.com/astorije","followers_url":"https://api.github.com/users/astorije/followers","following_url":"https://api.github.com/users/astorije/following{/other_user}","gists_url":"https://api.github.com/users/astorije/gists{/gist_id}","starred_url":"https://api.github.com/users/astorije/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/astorije/subscriptions","organizations_url":"https://api.github.com/users/astorije/orgs","repos_url":"https://api.github.com/users/astorije/repos","events_url":"https://api.github.com/users/astorije/events{/privacy}","received_events_url":"https://api.github.com/users/astorije/received_events","type":"User","site_admin":false},"repo":{"id":24663240,"name":"ansible-modules-core","full_name":"astorije/ansible-modules-core","owner":{"login":"astorije","id":113730,"avatar_url":"https://avatars.githubusercontent.com/u/113730?v=3","gravatar_id":"","url":"https://api.github.com/users/astorije","html_url":"https://github.com/astorije","followers_url":"https://api.github.com/users/astorije/followers","following_url":"https://api.github.com/users/astorije/following{/other_user}","gists_url":"https://api.github.com/users/astorije/gists{/gist_id}","starred_url":"https://api.github.com/users/astorije/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/astorije/subscriptions","organizations_url":"https://api.github.com/users/astorije/orgs","repos_url":"https://api.github.com/users/astorije/repos","events_url":"https://api.github.com/users/astorije/events{/privacy}","received_events_url":"https://api.github.com/users/astorije/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/astorije/ansible-modules-core","description":"Ansible modules - these modules ship with ansible","fork":true,"url":"https://api.github.com/repos/astorije/ansible-modules-core","forks_url":"https://api.github.com/repos/astorije/ansible-modules-core/forks","keys_url":"https://api.github.com/repos/astorije/ansible-modules-core/keys{/key_id}","collaborators_url":"https://api.github.com/repos/astorije/ansible-modules-core/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/astorije/ansible-modules-core/teams","hooks_url":"https://api.github.com/repos/astorije/ansible-modules-core/hooks","issue_events_url":"https://api.github.com/repos/astorije/ansible-modules-core/issues/events{/number}","events_url":"https://api.github.com/repos/astorije/ansible-modules-core/events","assignees_url":"https://api.github.com/repos/astorije/ansible-modules-core/assignees{/user}","branches_url":"https://api.github.com/repos/astorije/ansible-modules-core/branches{/branch}","tags_url":"https://api.github.com/repos/astorije/ansible-modules-core/tags","blobs_url":"https://api.github.com/repos/astorije/ansible-modules-core/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/astorije/ansible-modules-core/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/astorije/ansible-modules-core/git/refs{/sha}","trees_url":"https://api.github.com/repos/astorije/ansible-modules-core/git/trees{/sha}","statuses_url":"https://api.github.com/repos/astorije/ansible-modules-core/statuses/{sha}","languages_url":"https://api.github.com/repos/astorije/ansible-modules-core/languages","stargazers_url":"https://api.github.com/repos/astorije/ansible-modules-core/stargazers","contributors_url":"https://api.github.com/repos/astorije/ansible-modules-core/contributors","subscribers_url":"https://api.github.com/repos/astorije/ansible-modules-core/subscribers","subscription_url":"https://api.github.com/repos/astorije/ansible-modules-core/subscription","commits_url":"https://api.github.com/repos/astorije/ansible-modules-core/commits{/sha}","git_commits_url":"https://api.github.com/repos/astorije/ansible-modules-core/git/commits{/sha}","comments_url":"https://api.github.com/repos/astorije/ansible-modules-core/comments{/number}","issue_comment_url":"https://api.github.com/repos/astorije/ansible-modules-core/issues/comments{/number}","contents_url":"https://api.github.com/repos/astorije/ansible-modules-core/contents/{+path}","compare_url":"https://api.github.com/repos/astorije/ansible-modules-core/compare/{base}...{head}","merges_url":"https://api.github.com/repos/astorije/ansible-modules-core/merges","archive_url":"https://api.github.com/repos/astorije/ansible-modules-core/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/astorije/ansible-modules-core/downloads","issues_url":"https://api.github.com/repos/astorije/ansible-modules-core/issues{/number}","pulls_url":"https://api.github.com/repos/astorije/ansible-modules-core/pulls{/number}","milestones_url":"https://api.github.com/repos/astorije/ansible-modules-core/milestones{/number}","notifications_url":"https://api.github.com/repos/astorije/ansible-modules-core/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/astorije/ansible-modules-core/labels{/name}","releases_url":"https://api.github.com/repos/astorije/ansible-modules-core/releases{/id}","created_at":"2014-10-01T02:32:10Z","updated_at":"2015-07-04T19:59:23Z","pushed_at":"2015-07-29T22:34:42Z","git_url":"git://github.com/astorije/ansible-modules-core.git","ssh_url":"git@github.com:astorije/ansible-modules-core.git","clone_url":"https://github.com/astorije/ansible-modules-core.git","svn_url":"https://github.com/astorije/ansible-modules-core","homepage":"","size":7601,"stargazers_count":0,"watchers_count":0,"language":"Python","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"devel"}},"base":{"label":"ansible:devel","ref":"devel","sha":"981f0ad48166757fcd78e276777d6d7d21f5fc08","user":{"login":"ansible","id":1507452,"avatar_url":"https://avatars.githubusercontent.com/u/1507452?v=3","gravatar_id":"","url":"https://api.github.com/users/ansible","html_url":"https://github.com/ansible","followers_url":"https://api.github.com/users/ansible/followers","following_url":"https://api.github.com/users/ansible/following{/other_user}","gists_url":"https://api.github.com/users/ansible/gists{/gist_id}","starred_url":"https://api.github.com/users/ansible/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ansible/subscriptions","organizations_url":"https://api.github.com/users/ansible/orgs","repos_url":"https://api.github.com/users/ansible/repos","events_url":"https://api.github.com/users/ansible/events{/privacy}","received_events_url":"https://api.github.com/users/ansible/received_events","type":"Organization","site_admin":false},"repo":{"id":24474126,"name":"ansible-modules-core","full_name":"ansible/ansible-modules-core","owner":{"login":"ansible","id":1507452,"avatar_url":"https://avatars.githubusercontent.com/u/1507452?v=3","gravatar_id":"","url":"https://api.github.com/users/ansible","html_url":"https://github.com/ansible","followers_url":"https://api.github.com/users/ansible/followers","following_url":"https://api.github.com/users/ansible/following{/other_user}","gists_url":"https://api.github.com/users/ansible/gists{/gist_id}","starred_url":"https://api.github.com/users/ansible/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ansible/subscriptions","organizations_url":"https://api.github.com/users/ansible/orgs","repos_url":"https://api.github.com/users/ansible/repos","events_url":"https://api.github.com/users/ansible/events{/privacy}","received_events_url":"https://api.github.com/users/ansible/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/ansible/ansible-modules-core","description":"Ansible modules - these modules ship with ansible","fork":false,"url":"https://api.github.com/repos/ansible/ansible-modules-core","forks_url":"https://api.github.com/repos/ansible/ansible-modules-core/forks","keys_url":"https://api.github.com/repos/ansible/ansible-modules-core/keys{/key_id}","collaborators_url":"https://api.github.com/repos/ansible/ansible-modules-core/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/ansible/ansible-modules-core/teams","hooks_url":"https://api.github.com/repos/ansible/ansible-modules-core/hooks","issue_events_url":"https://api.github.com/repos/ansible/ansible-modules-core/issues/events{/number}","events_url":"https://api.github.com/repos/ansible/ansible-modules-core/events","assignees_url":"https://api.github.com/repos/ansible/ansible-modules-core/assignees{/user}","branches_url":"https://api.github.com/repos/ansible/ansible-modules-core/branches{/branch}","tags_url":"https://api.github.com/repos/ansible/ansible-modules-core/tags","blobs_url":"https://api.github.com/repos/ansible/ansible-modules-core/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/ansible/ansible-modules-core/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/ansible/ansible-modules-core/git/refs{/sha}","trees_url":"https://api.github.com/repos/ansible/ansible-modules-core/git/trees{/sha}","statuses_url":"https://api.github.com/repos/ansible/ansible-modules-core/statuses/{sha}","languages_url":"https://api.github.com/repos/ansible/ansible-modules-core/languages","stargazers_url":"https://api.github.com/repos/ansible/ansible-modules-core/stargazers","contributors_url":"https://api.github.com/repos/ansible/ansible-modules-core/contributors","subscribers_url":"https://api.github.com/repos/ansible/ansible-modules-core/subscribers","subscription_url":"https://api.github.com/repos/ansible/ansible-modules-core/subscription","commits_url":"https://api.github.com/repos/ansible/ansible-modules-core/commits{/sha}","git_commits_url":"https://api.github.com/repos/ansible/ansible-modules-core/git/commits{/sha}","comments_url":"https://api.github.com/repos/ansible/ansible-modules-core/comments{/number}","issue_comment_url":"https://api.github.com/repos/ansible/ansible-modules-core/issues/comments{/number}","contents_url":"https://api.github.com/repos/ansible/ansible-modules-core/contents/{+path}","compare_url":"https://api.github.com/repos/ansible/ansible-modules-core/compare/{base}...{head}","merges_url":"https://api.github.com/repos/ansible/ansible-modules-core/merges","archive_url":"https://api.github.com/repos/ansible/ansible-modules-core/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/ansible/ansible-modules-core/downloads","issues_url":"https://api.github.com/repos/ansible/ansible-modules-core/issues{/number}","pulls_url":"https://api.github.com/repos/ansible/ansible-modules-core/pulls{/number}","milestones_url":"https://api.github.com/repos/ansible/ansible-modules-core/milestones{/number}","notifications_url":"https://api.github.com/repos/ansible/ansible-modules-core/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/ansible/ansible-modules-core/labels{/name}","releases_url":"https://api.github.com/repos/ansible/ansible-modules-core/releases{/id}","created_at":"2014-09-25T20:28:25Z","updated_at":"2015-08-13T07:24:12Z","pushed_at":"2015-08-14T18:01:57Z","git_url":"git://github.com/ansible/ansible-modules-core.git","ssh_url":"git@github.com:ansible/ansible-modules-core.git","clone_url":"https://github.com/ansible/ansible-modules-core.git","svn_url":"https://github.com/ansible/ansible-modules-core","homepage":"","size":47765,"stargazers_count":389,"watchers_count":389,"language":"Python","has_issues":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":942,"mirror_url":null,"open_issues_count":579,"forks":942,"open_issues":579,"watchers":389,"default_branch":"devel"}},"_links":{"self":{"href":"https://api.github.com/repos/ansible/ansible-modules-core/pulls/150"},"html":{"href":"https://github.com/ansible/ansible-modules-core/pull/150"},"issue":{"href":"https://api.github.com/repos/ansible/ansible-modules-core/issues/150"},"comments":{"href":"https://api.github.com/repos/ansible/ansible-modules-core/issues/150/comments"},"review_comments":{"href":"https://api.github.com/repos/ansible/ansible-modules-core/pulls/150/comments"},"review_comment":{"href":"https://api.github.com/repos/ansible/ansible-modules-core/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/ansible/ansible-modules-core/pulls/150/commits"},"statuses":{"href":"https://api.github.com/repos/ansible/ansible-modules-core/statuses/ae1126c217bc48847e35311785ede1011d5e544f"}}}}